Closed Bug 1101905 Opened 5 years ago Closed 5 years ago

Make bytecode contain a notion of strictness

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: efaust, Assigned: efaust)

References

Details

Attachments

(11 files)

1.01 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
5.12 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
16.80 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
14.28 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
17.11 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
16.25 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
11.10 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.68 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
6.25 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
24.89 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1.57 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
Part 1 of bug 1101903, with more description there.

I have a prototype of this; patches forthcoming.
Blocks: 1101903
We used to have a notion where we could change JSOP_DELNAME to JSOP_FALSE if we saw the right conditions, but that's no longer the case. Remove some machinery that looks for it.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8525615 - Flags: review?(jwalden+bmo)
Attachment #8525617 - Flags: review?(jwalden+bmo)
Attachment #8525619 - Flags: review?(jwalden+bmo)
Attachment #8525621 - Flags: review?(jwalden+bmo)
Attachment #8525622 - Flags: review?(jwalden+bmo)
Attachment #8525624 - Flags: review?(jwalden+bmo)
Attachment #8525627 - Flags: review?(jwalden+bmo)
Attachment #8525628 - Flags: review?(jwalden+bmo)
Still not stricified:
JSOP_THIS
JSOP_ARGUMENTS
JSOP_DEFFUN

The first two clearly rely on the actual strictness of the script, rather than the site of the op, so they remain as they were.

The last is also arguably in this category, as it only uses the strictness to bind the function. Perhaps it should also be done?
Comment on attachment 8525615 [details] [diff] [review]
Part -1: Cleanup emitting JSOP_DELNAME

Review of attachment 8525615 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5891,5 @@
>        {
>          if (!BindNameToSlot(cx, bce, pn2))
>              return false;
> +        if (!EmitAtomOp(cx, pn2, pn2->getOp(), bce))
> +            return false;

Woo woo glad to see this dead.
Attachment #8525615 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8525616 [details] [diff] [review]
Part 0: Add general machinery for making strict opcodes

Review of attachment 8525616 [details] [diff] [review]:
-----------------------------------------------------------------

Legit.
Attachment #8525616 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8525617 [details] [diff] [review]
Part 1: Strictify JSOP_DELPROP and JSOP_DELELEM

Review of attachment 8525617 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5915,2 @@
>        {
> +        JSOp strictOp = bce->sc->strict ? JSOP_STRICTDELPROP : JSOP_DELPROP;

delOp?  strictOp suggests it's always JSOP_STRICT* to me.  Same below.

::: js/src/jit/BaselineCompiler.cpp
@@ +2112,5 @@
>      pushArg(ImmGCPtr(script->getName(pc)));
>      pushArg(R0);
>  
> +    bool strict = JSOp(*pc) == JSOP_STRICTDELPROP;
> +    if (!callVM(strict ? DeletePropertyStrictInfo : DeletePropertyNonStrictInfo))

So in the strict case, we could push a constant true as the result.  This is simpler for now, so let's roll with this, and file a (good first?) bug to be more optimal for the strict-mode case (for both strictdel* ops).

::: js/src/jit/CodeGenerator.cpp
@@ -8600,5 @@
>  {
>      pushArg(ImmGCPtr(lir->mir()->name()));
>      pushArg(ToValue(lir, LCallDeleteProperty::Value));
>  
> -    if (lir->mir()->block()->info().script()->strict())

Holy dereferencing Batman!

::: js/src/jit/MIR.h
@@ +10158,5 @@
>    public:
>      INSTRUCTION_HEADER(DeleteElement)
>  
> +    static MDeleteElement *New(TempAllocator &alloc, MDefinition *value, MDefinition *index,
> +                               bool strict) 

WS

::: js/src/vm/Interpreter.cpp
@@ +2267,5 @@
>  }
>  END_CASE(JSOP_DELNAME)
>  
>  CASE(JSOP_DELPROP)
> +CASE(JSOP_STRICTDELPROP)

I might be inclined to duplicate the code for these, myself, but it doesn't much matter.

@@ +2280,5 @@
>  
>      bool succeeded;
>      if (!JSObject::deleteGeneric(cx, obj, id, &succeeded))
>          goto error;
> +    if (!succeeded && (JSOp(*REGS.pc) == JSOP_STRICTDELPROP)) {

No need to overparenthesize here.

@@ +2307,5 @@
>      if (!ValueToId<CanGC>(cx, propval, &id))
>          goto error;
>      if (!JSObject::deleteGeneric(cx, obj, id, &succeeded))
>          goto error;
> +    if (!succeeded && (JSOp(*REGS.pc) == JSOP_STRICTDELELEM)) {

Again overparen.

::: js/src/vm/Opcodes.h
@@ +426,5 @@
>      macro(JSOP_SYMBOL,    45, "symbol",     NULL,         2,  0,  1,  JOF_UINT8) \
>      \
> +    /*
> +     * Pops the top of stack value, deletes property from it, pushes 'true' onto
> +     * the stack if succeeded, 'false' if not. (Strict mode)

Pops the top stack value and attempts to delete the given property from it.  Pushes 'true' on success, else throws a TypeError per strict mode property-deletion requirements.

@@ +436,5 @@
> +    macro(JSOP_STRICTDELPROP,   46, "strict-delprop",    NULL,         5,  1,  1, JOF_ATOM|JOF_PROP|JOF_CHECKSTRICT) \
> +    /*
> +     * Pops the top two values on the stack as 'propval' and 'obj',
> +     * deletes 'propval' property from 'obj', pushes 'true'  onto the stack if
> +     * succeeded, 'false' if not. (Strict mode)

Pops 'obj' and 'propval' and attempts to delete the 'propval' property from 'obj'.  Pushes 'true' on success, else throws a TypeError per strict mode property-deletion requirements.

These ops (notwithstanding their current state) should probably be in the Special Operators category.  Fix these new ones and the existing ones, please.
Attachment #8525617 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8525619 [details] [diff] [review]
Part 2: Strictify JSOP_SETPROP

Review of attachment 8525619 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2422,5 @@
>          if (Emit1(cx, bce, JSOP_SWAP) < 0)                  // N? OBJ N+1
>              return false;
>      }
>  
> +    JSOp setOp = bce->sc->strict ? JSOP_STRICTSETPROP : JSOP_SETPROP;

setOp, like the delOp I just suggested.  Nice!

::: js/src/jit/BaselineCompiler.cpp
@@ +2052,5 @@
>  
>  bool
> +BaselineCompiler::emit_JSOP_STRICTSETPROP()
> +{
> +    return emit_JSOP_SETPROP();

Hmm, this spooky strictness-checking at a distance is sadmaking.  Bleh.

::: js/src/jit/BaselineIC.cpp
@@ +7893,5 @@
>      JSOp op = JSOp(*pc);
>      FallbackICSpew(cx, stub, "SetProp(%s)", js_CodeName[op]);
>  
>      MOZ_ASSERT(op == JSOP_SETPROP ||
> +               op == JSOP_STRICTSETPROP ||

So fixing fallback is certainly necessary, but I kind of doubt it's sufficient for caches to work.  :-\  That said, brief investigation couldn't find anything that the IonBuilder.cpp changes wouldn't have kept working, so....

::: js/src/jit/IonBuilder.cpp
@@ +10010,5 @@
>  
>      // Always use a call if we are doing the definite properties analysis and
>      // not actually emitting code, to simplify later analysis.
>      if (info().executionModeIsAnalysis()) {
> +        // XXX: Dropped later in stack, but for now other ops rely on the old mechanic.

wat

@@ +10393,5 @@
>  {
>      MOZ_ASSERT(*emitted == false);
>  
> +    // XXX Dropped later in stack, but while others rely on it, we must retain the check.
> +    bool strict = IsStrictSetPC(pc) || script()->strict();

**2

::: js/src/jsopcode.h
@@ +667,5 @@
>  IsSetPropPC(jsbytecode *pc)
>  {
>      JSOp op = JSOp(*pc);
> +    return op == JSOP_SETPROP || op == JSOP_SETNAME || op == JSOP_SETGNAME ||
> +           op == JSOP_STRICTSETPROP;

Mild preference for setprops adjacent.

::: js/src/vm/Opcodes.h
@@ +445,5 @@
>       */ \
>      macro(JSOP_STRICTDELELEM,   47, "strict-delelem",    NULL,         1,  2,  1, JOF_BYTE|JOF_ELEM|JOF_CHECKSTRICT) \
> +    /*
> +     * Pops the top two values on the stack as 'val' and 'obj', sets property of
> +     * 'obj' as 'val', pushes 'obj' onto the stack. (Strict Mode)

Pops the top two values on the stack as 'val' and 'obj' and performs 'obj.prop = val', pushing 'val' back onto the stack.  Throws a TypeError if the set-operation failed (per strict mode semantics).

@@ +469,3 @@
>      /*
>       * Pops the top two values on the stack as 'val' and 'obj', sets property of
>       * 'obj' as 'val', pushes 'obj' onto the stack.

Same comment as before, minus the strict-mode bit.
Attachment #8525619 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8525621 [details] [diff] [review]
Part 3: Strictify JSOP_SETNAME

Review of attachment 8525621 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1517,5 @@
> +StrictifySetNameOp(JSOp op, BytecodeEmitter *bce)
> +{
> +    switch (op) {
> +        case JSOP_SETNAME:
> +            if (bce->sc->strict) op = JSOP_STRICTSETNAME;

Two lines, please.  And ixnay on the crazy indentation.

@@ +1525,5 @@
> +    return op;
> +}
> +
> +static void
> +StrictifySetNameOp(ParseNode *pn, BytecodeEmitter *bce)

Prefer a different name for this to overloading: StrictifySetNameNode or so.

::: js/src/jit/BaselineCompiler.cpp
@@ +2064,5 @@
>  
>  bool
> +BaselineCompiler::emit_JSOP_STRICTSETNAME()
> +{
> +    return emit_JSOP_SETPROP();

I guess this means this patch and the previous one require folding, for the SetProp fallback to be consistent with JSOP_*SETNAME at all times in this patch-stack.

::: js/src/jsopcode.h
@@ +668,5 @@
>  IsSetPropPC(jsbytecode *pc)
>  {
>      JSOp op = JSOp(*pc);
>      return op == JSOP_SETPROP || op == JSOP_SETNAME || op == JSOP_SETGNAME ||
> +           op == JSOP_STRICTSETPROP || op == JSOP_STRICTSETNAME;

Adjacency.

::: js/src/vm/Interpreter-inl.h
@@ +311,4 @@
>      MOZ_ASSERT_IF(*pc == JSOP_SETGNAME, scope == cx->global());
>  
> +    // XXX: Removed later in stack. SETGNAME still relies on script.
> +    bool strict = *pc == JSOP_STRICTSETNAME || script->strict();

watwat

::: js/src/vm/Interpreter.cpp
@@ +2386,3 @@
>  {
> +    static_assert(JSOP_SETNAME_LENGTH == JSOP_STRICTSETNAME_LENGTH,
> +                  "setname and strict-setname must be the same size");

Retroactive nitpick that s/-// in these messages -- there's no dash/underscore/etc. in the name, so don't put one here either.

::: js/src/vm/Opcodes.h
@@ +454,5 @@
>       */ \
>      macro(JSOP_STRICTSETPROP,   48, "strict-setprop",    NULL,         5,  2,  1, JOF_ATOM|JOF_PROP|JOF_SET|JOF_DETECTING|JOF_CHECKSTRICT) \
> +    /*
> +     * Pops a scope and value from the stack, assigns value to the given name,
> +     * and pushes the value back on the stack. (Strict mode)

s/\(Strict mode\)/If the set failed, then throw a TypeError per usual strict mode semantics./
Attachment #8525621 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8525622 [details] [diff] [review]
Part 4: Strictify JSOP_SETGNAME

Review of attachment 8525622 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1520,5 @@
>          case JSOP_SETNAME:
>              if (bce->sc->strict) op = JSOP_STRICTSETNAME;
>              break;
> +        case JSOP_SETGNAME:
> +            if (bce->sc->strict) op = JSOP_STRICTSETGNAME;

Two lines.

@@ +1658,5 @@
>  
>      JSOp op;
>      switch (pn->getOp()) {
>        case JSOP_NAME:     op = JSOP_GETGNAME; break;
> +      case JSOP_SETNAME:  op = StrictifySetNameOp(JSOP_SETGNAME, bce); break;

These really should be multiple lines as well.

::: js/src/jit/BaselineIC.cpp
@@ +7942,5 @@
>          }
>      } else if (op == JSOP_SETNAME ||
>                 op == JSOP_STRICTSETNAME ||
> +               op == JSOP_SETGNAME ||
> +               op == JSOP_STRICTSETGNAME)

...I guess this has to be folded with the others, too, for DoSetPropFallback consistency with the actual ops.  Weeeeeeeeeeeee.

::: js/src/jit/IonBuilder.cpp
@@ +1684,4 @@
>        {
>          PropertyName *name = info().getAtom(pc)->asPropertyName();
>          JSObject *obj = &script()->global();
>          return setStaticName(obj, name);

So setStaticName also appears to depend on jsop_setprop behavior, so it too needs folding into all of this.  La la la...

You would win massive bonus points if you could point out tests that fail at these intermediate stages due to this fallback-behavior inconsistency, or alternatively wrote tests that demonstrated the bugginess.

::: js/src/jit/VMFunctions.cpp
@@ +507,5 @@
>      if (MOZ_LIKELY(!obj->getOps()->setProperty)) {
>          return baseops::SetPropertyHelper<SequentialExecution>(
>              cx, obj.as<NativeObject>(), obj.as<NativeObject>(), id,
>              (op == JSOP_SETNAME || op == JSOP_SETGNAME ||
> +             op == JSOP_STRICTSETNAME || op == JSOP_STRICTSETGNAME)

Mild preference for setname || strictsetname || setgname || strictsetgname ordering to have adjacency.

::: js/src/vm/Interpreter.cpp
@@ +2386,5 @@
>  {
>      static_assert(JSOP_SETNAME_LENGTH == JSOP_STRICTSETNAME_LENGTH,
>                    "setname and strict-setname must be the same size");
> +    static_assert(JSOP_SETGNAME_LENGTH == JSOP_STRICTSETGNAME_LENGTH,
> +                  "setgname and strict-setgname must be the same size");

s/-//

::: js/src/vm/Opcodes.h
@@ +1443,4 @@
>      \
> +    /*
> +     * Pops the top two values on the stack as 'val' and 'scope', sets property
> +     * of 'scope' as 'val' and pushes 'val' back on the stack. (Strict mode)

Same sort of "If the set fails, throw a TypeError per strict mode semantics." thing.
Attachment #8525622 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8525624 [details] [diff] [review]
Part 5: Strictify JSOP_SETELEM

Review of attachment 8525624 [details] [diff] [review]:
-----------------------------------------------------------------

So the IonBuilder comment is pretty not-standards-conforming, but it's an existing problem, so good enough as long as we promptly fix it in more patches.

::: js/src/jit/IonBuilder.cpp
@@ +8480,5 @@
>          current->add(MPostWriteBarrier::New(alloc(), object, value));
>  
>      // Emit SetElementCache.
> +    bool strict = JSOp(*pc) == JSOP_STRICTSETELEM;
> +    MInstruction *ins = MSetElementCache::New(alloc(), object, index, value, strict, guardHoles);

Shouldn't strictness, instead of being part of this instruction, be part of MSetElementInstruction, just as it's part of MSetPropertyInstruction?  The consequence is that MCallSetElement has no sense of strictness at all, and lowering/codegen just pass a constant 0 for strictness.  That's gotta be buggy.  Pre-existingly, but we need this fixed, with a testcase that would fail.  Probably shouldn't hold up *this* particular patch, but it should be fixt pronto.

::: js/src/vm/Opcodes.h
@@ +504,5 @@
> +    macro(JSOP_SETELEM,   56, "setelem",    NULL,         1,  3,  1, JOF_BYTE |JOF_ELEM|JOF_SET|JOF_DETECTING|JOF_CHECKSLOPPY) \
> +    /*
> +     * Pops the top three values on the stack as 'val', 'propval' and 'obj',
> +     * sets 'propval' property of 'obj' as 'val', pushes 'obj' onto the
> +     * stack.

Same "Throws a TypeError if the set fails, per strict mode semantics." thing.
Attachment #8525624 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8525625 [details] [diff] [review]
Part 6: Cleanup remaining uses of script->strict() in property set paths

Review of attachment 8525625 [details] [diff] [review]:
-----------------------------------------------------------------

This is pretty stampy, would rather do this after removal of script->strict() somehow, but this is probably far enough to assume it's okay (in concert with prior patch-reading).  If not, lolfuzzing.
Attachment #8525625 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8525626 [details] [diff] [review]
Part 7: Use options.strictOption to pass strictness into Eval script compilation, rather than the strictness of the calling script

Review of attachment 8525626 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Eval.cpp
@@ +394,5 @@
>                 .setForEval(true)
>                 .setNoScriptRval(false)
>                 .setMutedErrors(mutedErrors)
> +               .setIntroductionInfo(introducerFilename, "eval", lineno, maybeScript, pcOffset)
> +               .maybeMakeStrictMode(callerScript && callerScript->strict());

I...think you can assert |callerScript| here, maybe?

::: js/src/vm/Debugger.cpp
@@ +5602,5 @@
>             .setNoScriptRval(false)
>             .setFileAndLine(filename, lineno)
>             .setCanLazilyParse(false)
> +           .setIntroductionType("debugger eval")
> +           .maybeMakeStrictMode(frame ? frame.script()->strict() : false);

Oh, I guess JSScript::strict() doesn't quite die.  Blah.  I remembered that, yessir I did.
Attachment #8525626 - Flags: review?(jwalden+bmo) → review+
Attachment #8525628 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8525627 [details] [diff] [review]
Part 8: Strictify JSOP_EVAL and JSOP_SPREADEVAL

Review of attachment 8525627 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Eval.cpp
@@ +203,1 @@
>          return EvalJSON_NotJSON;

Rather than introduce EvalJSONType and further split things, instead make the argument EvalType directly.  Now that ES6 allows duplicated properties in an object literal, this restriction is no longer necessary.  There might be a test or two requiring adjustment for this, tho I guess with no semantic difference now, maybe not.

@@ +292,5 @@
>  
>      RootedScript callerScript(cx, caller ? caller.script() : nullptr);
> +    EvalJSONType jsonType = evalType == INDIRECT_EVAL ? JSON_INDIRECT_EVAL :
> +                                IsStrictEvalPC(pc) ? JSON_STRICT_DIRECT_EVAL : JSON_DIRECT_EVAL;
> +    EvalJSONResult ejr = TryEvalJSON(cx, jsonType, flatStr, args.rval());

You can just pass |evalType| unmodified given the ES6 change.

@@ +371,5 @@
>      if (!flatStr)
>          return false;
>  
> +    EvalJSONType ejt = IsStrictEvalPC(pc) ? JSON_STRICT_DIRECT_EVAL : JSON_DIRECT_EVAL;
> +    EvalJSONResult ejr = TryEvalJSON(cx, ejt, flatStr, vp);

And just DIRECT_EVAL here.

::: js/src/jit/BaselineIC.cpp
@@ +8654,5 @@
>  TryAttachCallStub(JSContext *cx, ICCall_Fallback *stub, HandleScript script, jsbytecode *pc,
>                    JSOp op, uint32_t argc, Value *vp, bool constructing, bool isSpread,
>                    bool useNewType)
>  {
> +    if (useNewType || op == JSOP_EVAL || op == JSOP_STRICTEVAL)

Dilatory, but should the spreadeval ops be here, too?  Seems like we don't want subs for them, either.

::: js/src/jsscript.cpp
@@ +2936,5 @@
> +        MOZ_ASSERT(JSOp(*pc) == JSOP_EVAL || JSOp(*pc) == JSOP_STRICTEVAL ||
> +                   JSOp(*pc) == JSOP_SPREADEVAL || JSOp(*pc) == JSOP_STRICTSPREADEVAL);
> +        mozilla::DebugOnly<bool> isSpread = JSOp(*pc) == JSOP_SPREADEVAL ||
> +                                            JSOp(*pc) == JSOP_STRICTSPREADEVAL;
> +        MOZ_ASSERT(*(pc + (isSpread ? JSOP_SPREADEVAL_LENGTH : JSOP_EVAL_LENGTH)) == JSOP_LINENO);

Before this:

static_assert(JSOP_SPREADEVAL_LENGTH == JSOP_STRICTSPREADEVAL_LENGTH,
              "next op after a spread must be at consistent offset");
static_assert(JSOP_EVAL_LENGTH == JSOP_STRICTEVAL_LENGTH,
              "next op after a direct eval must be at consistent offset");

(Sure, we have some of this elsewhere, but really you want such things directly by each call site.)

::: js/src/vm/Interpreter.cpp
@@ +2454,3 @@
>  {
> +    static_assert(JSOP_EVAL_LENGTH == JSOP_STRICTEVAL_LENGTH,
> +                  "eval and strict-eval must be the same size");

s/-// and below

@@ +4040,2 @@
>          if (cx->global()->valueIsEval(args.calleev())) {
>              if (!DirectEval(cx, args))

Hm, we could/should make this pass in strictness at some point, maybe.

::: js/src/vm/Opcodes.h
@@ +464,5 @@
>      macro(JSOP_STRICTSETNAME,   49, "strict-setname",    NULL,         5,  2,  1,  JOF_ATOM|JOF_NAME|JOF_SET|JOF_DETECTING|JOF_CHECKSTRICT) \
> +    /*
> +     * spreadcall variant of JSOP_EVAL
> +     *
> +     * Invokes 'eval' with 'args' and pushes the return value onto the stack. (Strict mode)

s/\..*/, performing a direct eval if 'eval' is the current realm's eval function./ perhaps.  And same futzing for JSOP_SPREADEVAL.

@@ +1179,4 @@
>      \
> +    /* ECMA-compliant call to eval op. */ \
> +    /*
> +     * Invokes 'eval' with 'args' and pushes return value onto the stack. (Strict Mode)

Same sort of futzing as above, for both.
Attachment #8525627 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8525616 [details] [diff] [review]
Part 0: Add general machinery for making strict opcodes

Review of attachment 8525616 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +287,1 @@
>      /* These should filter through EmitVarOp. */

Put a blank line before this comment.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> Comment on attachment 8525624 [details] [diff] [review]
> Part 5: Strictify JSOP_SETELEM
> 
> Review of attachment 8525624 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So the IonBuilder comment is pretty not-standards-conforming, but it's an
> existing problem, so good enough as long as we promptly fix it in more
> patches.
> 
> ::: js/src/jit/IonBuilder.cpp
> @@ +8480,5 @@
> >          current->add(MPostWriteBarrier::New(alloc(), object, value));
> >  
> >      // Emit SetElementCache.
> > +    bool strict = JSOp(*pc) == JSOP_STRICTSETELEM;
> > +    MInstruction *ins = MSetElementCache::New(alloc(), object, index, value, strict, guardHoles);
> 
> Shouldn't strictness, instead of being part of this instruction, be part of
> MSetElementInstruction, just as it's part of MSetPropertyInstruction?  The
> consequence is that MCallSetElement has no sense of strictness at all, and
> lowering/codegen just pass a constant 0 for strictness.  That's gotta be
> buggy.  Pre-existingly, but we need this fixed, with a testcase that would
> fail.  Probably shouldn't hold up *this* particular patch, but it should be
> fixt pronto.

Filed bug 1105498.
Target Milestone: mozilla37 → mozilla36
Depends on: 1106141
Blocks: 1313064
See Also: → 1313064
You need to log in before you can comment on or make changes to this bug.