Differential Testing: Different output message involving arguments

RESOLVED FIXED in Firefox 63

Status

()

defect
P1
major
RESOLVED FIXED
3 years ago
9 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)

Reporter

Description

3 years ago
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)
Reporter

Comment 2

3 years ago
Jan, what's next here then?
Flags: needinfo?(jdemooij)
Reporter

Comment 3

2 years ago
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
Assignee

Comment 4

2 years ago
Gary mentions that this is blocking differential testing involving arguments.
Flags: needinfo?(nicolas.b.pierron)
Priority: P3 → P1
Whiteboard: [fuzzblocker]
Reporter

Updated

2 years ago
Flags: needinfo?(nicolas.b.pierron)
Assignee

Updated

10 months ago
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(nicolas.b.pierron)
Assignee

Comment 5

10 months ago
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+

Comment 8

9 months ago
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

Comment 9

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5811b189a158
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.