761914 - Clean up staticLevel and UpvarCookie handling

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Patch 1: remove UpvarCookie::UPVAR_LEVEL_LIMIT.

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.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Patch 2: Simplify UpvarCookie

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.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Patch 3: do level check in UpvarCookie::set()

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().

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Patch 4: set SharedContext::staticLevel in the constructor

This patch sets SharedContext::staticLevel via the constructor, rather than
via assignments afterwards.  This allows it to be const, which is nice.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Patch 3b: do level check in UpvarCookie::set()

This version of patch 3 adds an overflow check to the assignment of script->staticLevel.

Comment 6

[Jason Orendorff [:jorendorff]]

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.

Comment 7

[Nicholas Nethercote [inactive]]

> 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.

Comment 8

[Nicholas Nethercote [inactive]]

> 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 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8936c42edb7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/3acaeed330a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/558aad1afbab
https://hg.mozilla.org/integration/mozilla-inbound/rev/13ef4ab18b45

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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?

Comment 11

[Nicholas Nethercote [inactive]]

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.

Comment 12

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/8936c42edb7f
https://hg.mozilla.org/mozilla-central/rev/3acaeed330a4
https://hg.mozilla.org/mozilla-central/rev/558aad1afbab
https://hg.mozilla.org/mozilla-central/rev/13ef4ab18b45

(Merged by Ed Morley)