912303 - Differential Testing: Different output message involving __noSuchMethod__

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

try {
    Object.defineProperty(this, "y", {
        get: function() {
            return ([x, x])
        }
    });
    f = true;
    Object.defineProperty(this, "x", {
        get: function() {
            return this.f()
        }
    });
    __proto__["__noSuchMethod__"] = print;
    y;
} catch (e) {}

shows the following on js opt shell on m-c changeset 8c5a94ba1096 without any CLI arguments:

f
f

However it shows only the following with --ion-eager:

f


More fun when pasted in the shell:

$ ./js-opt-32-dm-ts-windows-8c5a94ba1096.exe
js> try {
    Object.defineProperty(this, "y", {
        get: function() {
            return ([x, x])
        }
    });
    f = true;
    Object.defineProperty(this, "x", {
        get: function() {
            return this.f()
        }
    });
    __proto__["__noSuchMethod__"] = print;
    y;
} catch (e) {}
f
f
[(void 0), (void 0)]
js>

$ ./js-opt-32-dm-ts-windows-8c5a94ba1096.exe --ion-eager
js> try {
    Object.defineProperty(this, "y", {
        get: function() {
            return ([x, x])
        }
    });
    f = true;
    Object.defineProperty(this, "x", {
        get: function() {
            return this.f()
        }
    });
    __proto__["__noSuchMethod__"] = print;
    y;
} catch (e) {}
f
function print() {
    [native code]
}
js>

My configure flags are:

 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-threadsafe <more NSPR compilation flags>

Comment 1

[Jan de Mooij [:jandem]]

Kannan, can you take this? It looks like the Baseline GetProp IC attaches a stub even if we hit the __noSuchMethod__ case.

What's worse, the GetProp_Native stubs should probably also guard against this in the CALLPROP case, because it's possible to change a property's value from object to primitive without changing the shape :( JM and Ion probably have the same issue... I don't know if fixing that is worth it..

Maybe we can even remove __noSuchMethod__ now, bug 683218?

Comment 2

[Jan de Mooij [:jandem]]

Same applies to CALLELEM. I'm afraid it will be a lot of work to make this work with the JITs in all cases, especially for indexed properties...

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: baseline-nosuchmethod-handling.patch

Modifies baseline stub generation for CALLPROP and CALLELEM to ensure that noSuchMethod handling is correct.

In some cases, this is handled by modifying the stubs to explicitly check the result for undefined, and if so, invoke a noSuchMethod handler.  In other cases, this is handled by not generating optimized stubs when the op is CALLPROP or CALLELEM.

Comment 4

[Kannan Vijayan [:djvj]]

Comment on attachment 821810 [details] [diff] [review]
baseline-nosuchmethod-handling.patch

decoder.. can I get this patch fuzzed, in particular exercising code that uses __noSuchMethod___ in various scenarios?

This patch is green on try for linux, linux64, and android.. but since __noSuchMethod__ is super heavily exercised in the test suites, I think a good round of fuzzer coverage would be useful.

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 821810 [details] [diff] [review]
baseline-nosuchmethod-handling.patch

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

Looks good. We should make sure that we aren't making a perf gaff with these disablings, but I agree they should be rare. There are a few comments below, and we discussed some of them already on IRC. r=me

::: js/src/jit/BaselineIC.cpp
@@ +3809,5 @@
>          !stub->hasStub(ICStub::GetElem_String))
>      {
> +        // Don't attach this stub for CALLPROP ops which might need noSuchMethod handling.
> +#if JS_HAS_NO_SUCH_METHOD
> +        if (isCallElem)

This is fine, but conservative, as discussed. Not only does a CALLELEM in this case make no semantic sense, but it will never invoke __nSM__ in practice, since the incoming string is not an object to begin with. In general, we should aim to be consistent between GETELEM and GETPROP, so if we are going to leave GetProp_String without a check of this kind, then we should with GETELEM as well.

@@ +4274,5 @@
> +        Address valAddr(scratchReg, 0);
> +
> +        // Check if __noSuchMethod__ needs to be called.
> +#if JS_HAS_NO_SUCH_METHOD
> +        entersStubFrame_ = true;

This looks like it's already set above, regardless of which stub is actually be generated. Line 4148, locally.

@@ +4296,5 @@
> +                return false;
> +            
> +            leaveStubFrame(masm);
> +            // Result is already in R0
> +            masm.jump(&afterNoSuchMethod); 

nit: whitespace at end of line and on line before leaveStubFrame()

@@ +4460,5 @@
> +        regs.takeUnchecked(obj);
> +        regs.take(R1);
> +
> +        masm.pushValue(R1);
> +        masm.loadValue(element, R1);

The first time we check the address, and this time we check the value after we've loaded it. Is there a reason for the difference in style? Perhaps GETPROP vs. GETELEM?

@@ +4474,5 @@
> +            return false;
> +        
> +        leaveStubFrame(masm);
> +        // Result is already in R0
> +        masm.jump(&afterNoSuchMethod); 

Your whitespace betrays your copypasta :P

@@ +4663,3 @@
>      ValueOperand tempVal = regs.takeAnyValue();
>      masm.loadValue(BaseIndex(argData, idxReg, ScaleFromElemWidth(sizeof(Value))), tempVal);
> +    regs.add(argData);

Why is this readded? I understand why it used to be, as we used the register set again later to get a bunch of spare registers. But now, we have line 4678, which starts with a new register set. Am I missing something?

@@ +4689,5 @@
> +            return false;
> +        
> +        leaveStubFrame(masm);
> +        // Result is already in R0
> +        masm.jump(&afterNoSuchMethod); 

whitespace

@@ +6033,5 @@
>      if (cacheableCall && isScripted && !isDOMProxy) {
> +#if JS_HAS_NO_SUCH_METHOD
> +        // It's hard to keep the original object alive through a call, and it's unlikely
> +        // that a getter will be used in callprop locations.  Just don't attach
> +        // stubs in that case.

Can we add a bit to this comment about using getters to generate functions being rare?

@@ +6106,5 @@
>      // If it's a shadowed listbase proxy property, attach stub to call Proxy::get instead.
>      if (isDOMProxy && domProxyShadowsResult == Shadows) {
>          JS_ASSERT(obj == holder);
> +#if JS_HAS_NO_SUCH_METHOD
> +        if (isCallProp)

This one is slightly dubious in it's potential rarity. We should talk to bz, and probably perform some amount of performance testing to ensure that this is not a problem.

@@ +6475,5 @@
> +        masm.pushValue(R1);
> +        masm.push(objReg);
> +        if (!callVM(LookupNoSuchMethodHandlerInfo, masm))
> +            return false;
> +        

nit: whitespace on previous line.

::: js/src/jit/BaselineIC.h
@@ +3136,5 @@
> +#if JS_HAS_NO_SUCH_METHOD
> +        return static_cast<int32_t>(kind) |
> +               (static_cast<int32_t>(isCallElem_) << 16) |
> +               (static_cast<int32_t>(needsAtomize_) << 17) |
> +               (static_cast<int32_t>(acctype_) << 18);

Is there a good reason to make isCallElem_ the first thing here? Why not just stick it at the end, and then we don't need to have a whole different cascade behind the ifdef, just a single binary or. We want it to line up with other stubs, or something?

It makes me really uncomfortable that we have different bit meanings based on an ifdef. How much more churn are we likely to have here? If not much, can we just bubble it to the end in all these stubs?

::: js/src/jit/arm/BaselineHelpers-arm.h
@@ +195,5 @@
>      }
>  }
>  
>  inline void
> +EmitUnstowICValues(MacroAssembler &masm, int values, bool discard=false)

Are these changes used anywhere? I don't see any other references to EmitUnstowICValues in the patch...

I don't particularly object ot them, but we shouldn't add them needlessly.

Comment 6

[Kannan Vijayan [:djvj]]

(In reply to Eric Faust [:efaust] from comment #5)
> @@ +4460,5 @@
> > +        regs.takeUnchecked(obj);
> > +        regs.take(R1);
> > +
> > +        masm.pushValue(R1);
> > +        masm.loadValue(element, R1);
> 
> The first time we check the address, and this time we check the value after
> we've loaded it. Is there a reason for the difference in style? Perhaps
> GETPROP vs. GETELEM?

It's an |Address| vs. |BaseIndex| thing.  It's possible to type-check an Address-referenced value directly from memory.  On x86 we just add 4 to the offset and read a 32-bit value from there.  For BaseIndex (which are scaled register offset addresses), that's not possible.


> 
> @@ +6106,5 @@
> >      // If it's a shadowed listbase proxy property, attach stub to call Proxy::get instead.
> >      if (isDOMProxy && domProxyShadowsResult == Shadows) {
> >          JS_ASSERT(obj == holder);
> > +#if JS_HAS_NO_SUCH_METHOD
> > +        if (isCallProp)
> 
> This one is slightly dubious in it's potential rarity. We should talk to bz,
> and probably perform some amount of performance testing to ensure that this
> is not a problem.

Agreed.


> ::: js/src/jit/BaselineIC.h
> @@ +3136,5 @@
> > +#if JS_HAS_NO_SUCH_METHOD
> > +        return static_cast<int32_t>(kind) |
> > +               (static_cast<int32_t>(isCallElem_) << 16) |
> > +               (static_cast<int32_t>(needsAtomize_) << 17) |
> > +               (static_cast<int32_t>(acctype_) << 18);
> 
> Is there a good reason to make isCallElem_ the first thing here? Why not
> just stick it at the end, and then we don't need to have a whole different
> cascade behind the ifdef, just a single binary or. We want it to line up
> with other stubs, or something?
> 
> It makes me really uncomfortable that we have different bit meanings based
> on an ifdef. How much more churn are we likely to have here? If not much,
> can we just bubble it to the end in all these stubs?

Couple of reasons:
1. isCallElem seems more 'related' to the kind - a statement about what kind of operation is being run.

2. I find it syntactically ugly to leave a statement unfinished, except an #ifdeffed block finishes the statement, and the #else block of it contains a single semicolon that finishes the statement.  IMHO it messes with readability.  In this case, it's very clear that two different things are happening, in which cases they're happening, and what's happening in each case.

It's unlikely that there will be much churn in these cases.  If you feel strongly about this I can change it to the way you suggest, but I honestly feel that this is more readable.

Comment 7

[Eric Faust [:efaust]]

(In reply to Kannan Vijayan [:djvj] from comment #6)

> It's unlikely that there will be much churn in these cases.  If you feel
> strongly about this I can change it to the way you suggest, but I honestly
> feel that this is more readable.

Nah, you're right. I didn't consider the semicolon. That sounds really ugly.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 821810 [details] [diff] [review]
baseline-nosuchmethod-handling.patch

Not sure if I'll have time for testing this patch anytime soon, but let's see if I can get this on my radar.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Kannan Vijayan [:djvj] from comment #3)
> Created attachment 821810 [details] [diff] [review]
> baseline-nosuchmethod-handling.patch

fwiw, this patch fails to apply on tip today, so testing using a m-c rev from Oct 28, rev f9fe2c148aa8 (which seems to work).

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Actually, it'll be best to get a rebase on tip - that rev wasn't particularly stable.

Comment 11

[Kannan Vijayan [:djvj]]

I just tried to apply to mozilla-inbound tip and it applies fine.  Just tried on recent pull of m-c and it applies cleanly with some offsets.

What rev are you trying to apply it to?  Have you tried the latest tip as of today?

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(on m-c tip rev a07aebef20e7)

$ patch -p1 --dry-run < ~/Desktop/912303-c3.diff 
patching file js/src/jit/BaselineIC.cpp
Hunk #1 succeeded at 3483 (offset -13 lines).
Hunk #2 succeeded at 3675 (offset -13 lines).
Hunk #3 FAILED at 3719.
Hunk #4 succeeded at 3752 (offset -13 lines).
Hunk #5 succeeded at 3777 (offset -13 lines).
Hunk #6 succeeded at 3830 (offset -13 lines).
Hunk #7 succeeded at 3865 (offset -13 lines).
Hunk #8 succeeded at 4237 (offset -13 lines).
Hunk #9 succeeded at 4424 (offset -13 lines).
Hunk #10 succeeded at 4506 (offset -13 lines).
Hunk #11 succeeded at 4635 (offset -13 lines).
Hunk #12 FAILED at 5985.
Hunk #13 succeeded at 6021 (offset -15 lines).
Hunk #14 succeeded at 6068 (offset -15 lines).
Hunk #15 succeeded at 6089 (offset -15 lines).
Hunk #16 succeeded at 6200 (offset -15 lines).
Hunk #17 succeeded at 6365 (offset -15 lines).
Hunk #18 succeeded at 6387 (offset -15 lines).
Hunk #19 succeeded at 6407 (offset -15 lines).
Hunk #20 succeeded at 6553 (offset -15 lines).
Hunk #21 succeeded at 6626 (offset -15 lines).
Hunk #22 succeeded at 6718 (offset -15 lines).
Hunk #23 succeeded at 6871 (offset -15 lines).
2 out of 23 hunks FAILED -- saving rejects to file js/src/jit/BaselineIC.cpp.rej
patching file js/src/jit/BaselineIC.h
patching file js/src/jit/arm/BaselineHelpers-arm.h
patching file js/src/jit/arm/MacroAssembler-arm.cpp
patching file js/src/jit/arm/MacroAssembler-arm.h
patching file js/src/jit/x64/BaselineHelpers-x64.h
patching file js/src/jit/x64/MacroAssembler-x64.h
Hunk #1 succeeded at 351 (offset 1 line).
Hunk #2 succeeded at 806 (offset 2 lines).
patching file js/src/jit/x86/BaselineHelpers-x86.h
patching file js/src/jit/x86/MacroAssembler-x86.h
Hunk #1 succeeded at 352 (offset 1 line).

Comment 13

[Christian Holler (:decoder)]

Comment on attachment 821810 [details] [diff] [review]
baseline-nosuchmethod-handling.patch

Haven't spotted anything going wrong here.

Comment 14

[Kannan Vijayan [:djvj]]

(In reply to Gary Kwong [:gkw] [:nth10sd] (yes, still catching up on bugmail) from comment #12)
> (on m-c tip rev a07aebef20e7)
> 
> $ patch -p1 --dry-run < ~/Desktop/912303-c3.diff 
> patching file js/src/jit/BaselineIC.cpp
> Hunk #1 succeeded at 3483 (offset -13 lines).
...

This is really weird.  Are you sure you have the right diff?

Regardless, I'm going to try to land the bug now and see how it goes.  It's coming up green on try and seems OK with :decoder's fuzzing, so it should be fine.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 821810 [details] [diff] [review]
baseline-nosuchmethod-handling.patch

Strange that the patch application failed for me. Your suggested way of moving forward works, too. I'll be sure to file new bugs as they come along.

Comment 16

[Kannan Vijayan [:djvj]]

That'll be appreciated, thanks Gary!  If you see noSuchMethod crashes going forward it might be worthwhile to test this commit for regression.

https://hg.mozilla.org/integration/mozilla-inbound/rev/58605e9a6ea1

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/58605e9a6ea1