Last Comment Bug 761914 - Clean up staticLevel and UpvarCookie handling
: Clean up staticLevel and UpvarCookie handling
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks: UntangleFrontEnd 758509
  Show dependency treegraph
 
Reported: 2012-06-05 22:32 PDT by Nicholas Nethercote [:njn]
Modified: 2012-06-08 04:17 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1: remove UpvarCookie::UPVAR_LEVEL_LIMIT. (2.82 KB, patch)
2012-06-05 22:33 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Patch 2: Simplify UpvarCookie (6.95 KB, patch)
2012-06-05 22:34 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Patch 3: do level check in UpvarCookie::set() (19.34 KB, patch)
2012-06-05 22:37 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Review
Patch 4: set SharedContext::staticLevel in the constructor (10.55 KB, patch)
2012-06-05 22:40 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Patch 3b: do level check in UpvarCookie::set() (20.43 KB, patch)
2012-06-06 16:33 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2012-06-05 22:32:34 PDT
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 [:njn] 2012-06-05 22:33:33 PDT
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.
Comment 2 Nicholas Nethercote [:njn] 2012-06-05 22:34:59 PDT
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.
Comment 3 Nicholas Nethercote [:njn] 2012-06-05 22:37:00 PDT
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().
Comment 4 Nicholas Nethercote [:njn] 2012-06-05 22:40:50 PDT
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.
Comment 5 Nicholas Nethercote [:njn] 2012-06-06 16:33:13 PDT
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.
Comment 6 Jason Orendorff [:jorendorff] 2012-06-07 17:31:20 PDT
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 [:njn] 2012-06-07 18:35:06 PDT
> 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 [:njn] 2012-06-07 18:41:38 PDT
> 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 10 :Ms2ger 2012-06-08 00:50:01 PDT
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 [:njn] 2012-06-08 00:56:18 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.