1293575 - Differential Testing: Different output message involving arguments

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

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

for (let w in (function(y) {
        var y = 0;
        for (var x = 0; x < 9; ++x) {
            z = arguments.callee.arguments;
        }
    })(makeFinalizeObserver('nursery')));
print(uneval(z));


$ ./js-dbg-64-dm-clang-darwin-720b5d2c84d5 --fuzzing-safe --no-threads --ion-eager testcase.js
({0:0})

$ ./js-dbg-64-dm-clang-darwin-720b5d2c84d5 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
({0:{}})

Tested this on m-c rev 720b5d2c84d5.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin14.5.0 --disable-jemalloc --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 720b5d2c84d5

This seems to go back prior to m-c rev dc4b163f7db7 (Nov 2014), so setting needinfo? from Jan as a start.

Comment 1

[Jan de Mooij [:jandem]]

Below is a simpler test. It's probably caused by Ion not getting along well with f.arguments. Maybe we should print to stderr in more-deterministic mode when f.arguments is used...

function f(y) {
    var y = 0;
    for (var x = 0; x < 9; ++x) {
        z = arguments.callee.arguments;
    }
}
f(Math);
print(uneval(z));

Comment 2

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

Jan, what's next here then?

Comment 3

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

We (Jan/me/nbp at SF All Hands) agreed that :nbp should be the right person to needinfo? here.

Comment 4

[Nicolas B. Pierron [:nbp]]

Gary mentions that this is blocking differential testing involving arguments.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Skip update of frame arguments when the Arguments object aliases the formal arguments.

The problem is that the frame arguments were being restored during the bailout
based on the snapshot slots for arguments.

This patch removes the update of the frame arguments when the Arguments object
aliases the formals. In this cases both Baseline and IonMonkey update the
argument object on JSOP_SETARG, and the frame arguments are left untouched.
(except on bailout, which was the error)

Comment 6

[Jan de Mooij [:jandem]]

I wanted to review this before taking PTO the rest of today and maybe tomorrow, but this code is subtle. Will get back to it soon.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8996367 [details] [diff] [review]
Skip update of frame arguments when the Arguments object aliases the formal arguments.

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

I think this makes sense, but it's really hard to wrap my head around all the cases :) Nice to have this bug fixed finally!

::: js/src/jit-test/tests/basic/bug1293575.js
@@ +1,3 @@
> +
> +function f(y) {
> +    var y = 123456;

Nit: remove the "var", y is an argument

::: js/src/jit/BaselineBailouts.cpp
@@ +792,5 @@
>          MOZ_ASSERT(iter.numAllocations() >= CountArgSlots(script, fun));
>          JitSpew(JitSpew_BaselineBailouts, "      frame slots %u, nargs %zu, nfixed %zu",
>                  iter.numAllocations(), fun->nargs(), script->nfixed());
>  
> +        bool argsObjAliasesFormals = script->needsArgsObj() && script->hasMappedArgsObj();

Nit: can use script->argsObjAliasesFormals();

@@ +794,5 @@
>                  iter.numAllocations(), fun->nargs(), script->nfixed());
>  
> +        bool argsObjAliasesFormals = script->needsArgsObj() && script->hasMappedArgsObj();
> +        if (frameNo == 0 && !argsObjAliasesFormals) {
> +            // This is the first frame unless we have an argument object. Store

This is still always the first frame, arguments object or not. Maybe change to:

"This is the first (outermost) frame and we don't have an arguments object aliasing the formals."

@@ +814,5 @@
>                  size_t argOffset = builder.framePushed() + JitFrameLayout::offsetOfActualArg(i);
>                  builder.valuePointerAtStackOffset(argOffset).set(arg);
> +            } else if (argsObjAliasesFormals) {
> +                // When the arguments object aliases the formal arguments, then all
> +                // JSOP_SETARG mutates the argument object. In such cases, the list of

Nit: s/all//

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5811b189a158
Skip update of frame arguments when the Arguments object aliases the formal arguments. r=jandem

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5811b189a158