Clean up staticLevel and UpvarCookie handling

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 630449 [details] [diff] [review]
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.
Attachment #630449 - Flags: review?(jorendorff)
(Assignee)

Comment 2

5 years ago
Created attachment 630450 [details] [diff] [review]
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.
Attachment #630450 - Flags: review?(jorendorff)
(Assignee)

Comment 3

5 years ago
Created attachment 630451 [details] [diff] [review]
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().
Attachment #630451 - Flags: review?(jorendorff)
(Assignee)

Comment 4

5 years ago
Created attachment 630453 [details] [diff] [review]
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.
Attachment #630453 - Flags: review?(jorendorff)
(Assignee)

Comment 5

5 years ago
Created attachment 630764 [details] [diff] [review]
Patch 3b: do level check in UpvarCookie::set()

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

Updated

5 years ago
Blocks: 750279
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+
(Assignee)

Comment 7

5 years ago
> 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
(Assignee)

Comment 8

5 years ago
> 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.
(Assignee)

Comment 9

5 years ago
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
Target Milestone: --- → mozilla16
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?
(Assignee)

Comment 11

5 years ago
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.
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)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.