Closed Bug 761914 Opened 12 years ago Closed 12 years ago

Clean up staticLevel and UpvarCookie handling

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [js:t])

Attachments

(4 files, 1 obsolete file)

UpvarCookie and staticLevel are a bit of a mess and they've been getting in my way with the lazy bytecode work. This bug will clean them up.
This patch removes UpvarCookie::UPVAR_LEVEL_LIMIT. The comment just above its solitary use is clearly no longer correct. I tried replacing that use with 0 but got assertion failures because isDirectEvalFrame() isn't expecting a zero staticLevel. So I tried using 1 instead and that passes all the tests.
Attachment #630449 - Flags: review?(jorendorff)
This patch simplifies UpvarCookie in two ways. - First, since UpvarCookie holds two uint16_t values, I converted the existing uint32_t |value| which encodes both to two uint16_t fields. Much better. - Second, we need to encode a "free" value, but that's currently done by setting the level to 0x3fff or higher. This is an odd encoding, and there's no reason why we can't use a level of 0xffff to represent the "free" value and leave everything below that for normal levels. This allowed the removal of JSFB_LEVEL_BITS; sizeof(FunctionBox) is unchanged.
Attachment #630450 - Flags: review?(jorendorff)
SetStaticLevel() looks like this: bool frontend::SetStaticLevel(SharedContext *sc, unsigned staticLevel) { /* * This is a lot simpler than error-checking every UpvarCookie::set, and * practically speaking it leaves more than enough room for upvars. */ if (UpvarCookie::isLevelReserved(staticLevel)) { JS_ReportErrorNumber(sc->context, js_GetErrorMessage, NULL, JSMSG_TOO_DEEP, js_function_str); return false; } sc->staticLevel = staticLevel; return true; } The comment is false -- it's not that hard to check in UpvarCookie::set(). In fact, it makes a lot more sense to check it there, because UpvarCookie::level is 16 bits, whereas SharedContext::staticLevel is 32 bits. So this patch moves the checking from SetStaticLevel into UpvarCookie::set(). (In fact, the checking is moot because we'll blow the stack long before we've processed 64K nested functions, but I've kept it to be conservative.) I also de-JSBoolified BindNameToSlot().
Attachment #630451 - Flags: review?(jorendorff)
This patch sets SharedContext::staticLevel via the constructor, rather than via assignments afterwards. This allows it to be const, which is nice.
Attachment #630453 - Flags: review?(jorendorff)
This version of patch 3 adds an overflow check to the assignment of script->staticLevel.
Attachment #630451 - Attachment is obsolete: true
Attachment #630451 - Flags: review?(jorendorff)
Attachment #630764 - Flags: review?(jorendorff)
Attachment #630449 - Flags: review?(jorendorff) → review+
Attachment #630450 - Flags: review?(jorendorff) → review+
Attachment #630453 - Flags: review?(jorendorff) → review+
Comment on attachment 630764 [details] [diff] [review] Patch 3b: do level check in UpvarCookie::set() I agree the comment is false, but why change this? diffstat shows this is -2 lines of code net, not necessarily worth worrying about; and I would have thought checking the staticLevel each time it changes would be saner than checking later downstream uses, regardless of the number of bits in any given member variable. Of course as you observe it is all moot either way since we can't trigger these errors. Granting review, since it seems harmless enough, but a bit puzzled.
Attachment #630764 - Flags: review?(jorendorff) → review+
> I agree the comment is false, but why change this? diffstat shows this is -2 > lines of code net, not necessarily worth worrying about; and I would have > thought checking the staticLevel each time it changes would be saner than > checking later downstream uses, regardless of the number of bits in any > given member variable. The motivation is bug 758509, which will pass |staticLevel| into SharedContext's constructor. Fallible constructors are a pain -- I didn't want to create a SharedContext::init() function if I could avoid it, esp. since I want SharedContext::staticLevel to become const, which the init() approach doesn't allow. Sorry I didn't explain this. Besides, the staticLevel is |unsigned| in most places, but in two places we restrict it to 16 bits. It felt more natural and less error-prone to me to do the range-checking when we actually do the shrink-to-16-bits assignment, not ahead of time.
Blocks: 758509
> The motivation is bug 758509, which will pass |staticLevel| into > SharedContext's constructor. Oh wait, I actually did that in patch 4 in this bug.
Comment on attachment 630764 [details] [diff] [review] Patch 3b: do level check in UpvarCookie::set() Review of attachment 630764 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscript.cpp @@ +1265,5 @@ > } > script->nslots = script->nfixed + bce->maxStackDepth; > + > + // This is an unsigned-to-uint16_t conversion, test for too-high values. > + if (bce->sc->staticLevel > UINT_MAX) { Not UINT16_MAX?
Well spotted :) I just fixed it in a follow-up patch in bug 759509. In practice it doesn't matter because we run out of stack space *well* before hitting that limit, so there's no rush to fix it.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: