1476417 - Differential Testing: Different output message involving arguments

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

function f(x) {
    return (Math, arguments = 9999 * (x -= 1));
}
x = [0, 0, 524288];
for (var j = 0; j < 3; ++j) {
    print(f(x[j]));
}


$ ./js-dbg-64-dm-linux-547144f5596c --fuzzing-safe --no-threads --ion-eager testcase.js 
-9999
-9999
5242335714

$ ./js-dbg-64-dm-linux-547144f5596c --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js 
-9999
-9999
5242345713

Tested this on m-c rev 547144f5596c.

My configure flags are:

'AR=ar' 'sh' '/home/ubuntu/trees/mozilla-central/js/src/configure' '--enable-debug' '--enable-more-deterministic' '--with-ccache' '--enable-gczeal' '--enable-debug-symbols' '--disable-tests'

python -u -m funfuzz.js.compile_shell -b "--enable-debug --enable-more-deterministic" -r 547144f5596c

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/1832a6e47f1c
user:        Tom Schuster
date:        Fri Dec 30 17:38:08 2016 +0100
summary:     Bug 1324566 - Port Baseline GlobalNameAccessor to CacheIR. r=jandem

Tom, is bug 1324566 a likely regressor?

Comment 1

[Tom S [:evilpie]]

I don't think so, no GlobalNameGetter is attached while running this. Maybe Matthew can take a look, he has been working on CacheIR the most recently.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

It's definitely not a CacheIR problem: I see the same behaviour with --cache-ir-stubs=off

Gary: Is it possible to re-do the regression search with that option set? (Just wondering if it might find a more interesting regressor, or if the bisect will double-down on that bug)

Comment 3

[André Bargull [:anba]]

Here's my guess what happens:
---
function f(x) {
    arguments; // <-- Force creation of mapped arguments, so modifying |x| writes to memory.
    Math; // <-- We resume from here after the int32 overflow in the multiplication. (Access to global variables creates a resume point.)
    var y;
    x += 1; // <-- Is executed twice because the resume point is before the addition.
    y = 2 * x; // <-- Triggers bailout when overflowing int32 boundaries.
    print(x, y); // Should print "1073741824 2147483648" in the 3rd iteration, but actually prints "1073741825 2147483650".
}
x = [0, 0, 0x3FFFFFFF];
for (var j = 0; j < 3; ++j) {
    f(x[j]);
}
---

Comment 4

[André Bargull [:anba]]

This patch does fix the issue, but I have no idea if that's the correct solution here:

diff --git a/js/src/jit/IonBuilder.cpp b/js/src/jit/IonBuilder.cpp
--- a/js/src/jit/IonBuilder.cpp
+++ b/js/src/jit/IonBuilder.cpp
@@ -12252,18 +12252,20 @@ IonBuilder::jsop_setarg(uint32_t arg)
                   script()->baselineScript()->modifiesArguments());
     MDefinition* val = current->peek(-1);
 
     // If an arguments object is in use, and it aliases formals, then all SETARGs
     // must go through the arguments object.
     if (info().argsObjAliasesFormals()) {
         if (needsPostBarrier(val))
             current->add(MPostWriteBarrier::New(alloc(), current->argumentsObject(), val));
-        current->add(MSetArgumentsObjectArg::New(alloc(), current->argumentsObject(),
-                                                 GET_ARGNO(pc), val));
+        auto* ins = MSetArgumentsObjectArg::New(alloc(), current->argumentsObject(),
+                                                GET_ARGNO(pc), val);
+        current->add(ins);
+        MOZ_TRY(resumeAfter(ins));
         return Ok();
     }
 
     // :TODO: if hasArguments() is true, and the script has a JSOP_SETARG, then
     // convert all arg accesses to go through the arguments object. (see Bug 957475)
     if (info().hasArguments())
         return abort(AbortReason::Disable, "NYI: arguments & setarg.");

Comment 5

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

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #2)
> Gary: Is it possible to re-do the regression search with that option set?
> (Just wondering if it might find a more interesting regressor, or if the
> bisect will double-down on that bug)

I'm not sure if a new bisect would help. If CacheIR isn't involved, then something else is.

Passing the baton to Jan as a start, esp regarding Andre's comment 4.

Comment 6

[Jan de Mooij [:jandem]]

Very old bug, nice find.

André's fix is the right one, effectful instructions like MSetArgumentsObjectArg need a resume point. We've tried to assert this in the past but IIRC it's complicated.

anba, want to finish this?

Comment 7

[André Bargull [:anba]]

Attachment: bug1476417.patch

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8994830 [details] [diff] [review]
bug1476417.patch

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

Thanks!

Comment 9

[Nicolas B. Pierron [:nbp]]

André, should we land this patch on your behalf?

Comment 10

[André Bargull [:anba]]

Oh, I just forgot to request check-in for this one. I'll spin a try-build for this patch and when it's successful, I'll set the checkin-needed flag.

Comment 11

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dafa3f50d04bf9bc793cab64e8caf4653b907dc

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3db99794bc30
Add a resume point after assigning to a mapped argument. r=jandem

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/3db99794bc30