Differential Testing: Different output message involving arguments

RESOLVED FIXED in Firefox 63

Status

()

defect
P1
major
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: gkw, Assigned: nbp)

Tracking

(Blocks 2 bugs, {testcase, triage-deferred})

Trunk
mozilla63
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [fuzzblocker])

Attachments

(1 attachment)

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.
Flags: needinfo?(jdemooij)
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));
Flags: needinfo?(jdemooij)
Jan, what's next here then?
Flags: needinfo?(jdemooij)
We (Jan/me/nbp at SF All Hands) agreed that :nbp should be the right person to needinfo? here.
Flags: needinfo?(jdemooij) → needinfo?(nicolas.b.pierron)
Keywords: triage-deferred
Priority: -- → P3
Gary mentions that this is blocking differential testing involving arguments.
Flags: needinfo?(nicolas.b.pierron)
Priority: P3 → P1
Whiteboard: [fuzzblocker]
Flags: needinfo?(nicolas.b.pierron)
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(nicolas.b.pierron)
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)
Attachment #8996367 - Flags: review?(jdemooij)
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 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//
Attachment #8996367 - Flags: review?(jdemooij) → review+
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5811b189a158
Skip update of frame arguments when the Arguments object aliases the formal arguments. r=jandem
https://hg.mozilla.org/mozilla-central/rev/5811b189a158
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.