810253 - IonMonkey: arguments[i] incorrectly returns undefined

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

This is causing a jit-test failure with the baseline compiler, but a modified test case reproduces on aurora and inbound tip (revision 1fdb059f5ae6):

function f(x) {
    // Enter via OSR.
    for (var j = 0; j < 100; j++) { };

    for (var i = 0; i < arguments.length; i++)
        assertEq(arguments[i], i);
};
f(0, 1, 2, 3);

$ ./js --no-jm test.js
test4.js:1:0 Error: Assertion failed: got (void 0), expected 0

32-bit debug shell, OS X.

Comment 1

[Nicolas B. Pierron [:nbp]]

While looking at the assembly code generated, I think we need a bit of analysis to get rid of some checks:

   0x7ffff7faba5b:      mov    0x48(%rsp),%rcx  ; rcx = argc [0 … 4096]
   0x7ffff7faba60:      mov    %rcx,0x20(%rsp)
   0x7ffff7faba65:      mov    %rcx,%rdx        ; rdx = argc
   0x7ffff7faba68:      add    $0xffffffff,%edx ; edx -= 1
   0x7ffff7faba6b:      jo     0x7ffff7fabc93   ; if (rdx was int32_min) bailout … what ?!
   0x7ffff7faba71:      cmp    %edx,%ecx
   0x7ffff7faba73:      jbe    0x7ffff7fabc9d   ; if (argc < argc - 1) bailout … what ?!!!
   0x7ffff7faba79:      xor    %edx,%edx        ; edx = 0
   0x7ffff7faba7b:      cmp    $0x0,%edx        
   0x7ffff7faba7e:      jl     0x7ffff7fabca7   ; if (edx < 0) bailout, … what ?!!!!!
   0x7ffff7faba84:      xor    %edx,%edx        ; edx = 0

The current issue appears to be a snapshot issue during the bailout:

(gdb) 
365         IonBailoutIterator iter(ionActivations, sp);
(gdb) 
366         IonActivation *activation = ionActivations.activation();
(gdb) p iter.dump()
 JS frame
  callee fun: object 0x7fffef11b100
class 0xe9f4a0 Function
flags:
proto <unnamed function (:0) at 0x7fffef10e040>
parent <global object at 0x7fffef10b060>
properties:

  file _build/bug810253.js line 1
  script = 0x7fffef10f1a0, pc = 0xf05786
  current op: callgname
  slots: 5
  scope chain: undefined
  this: undefined
  formal (arg 0): undefined
  actual (arg 1): 1
  actual (arg 2): 2
  actual (arg 3): 3
  slot 0: 100
  slot 1: 0

The snapshot encodes an undefined, likely because the Phi introduced between the normal entrance path and the OSR path is removed because it is not used, except that it is used by Resume points.

Comment 2

[Alex Keybl [:akeybl]]

Would you all mind providing context as to the user/dev impact here if left unfixed?

Comment 3

[Nicolas B. Pierron [:nbp]]

The Phi got eliminated because we assume that there is no use of the formal argument, except that we restore the formal argument on top of the actual argument when we bailout.

1/ We can fix this bug by modifying MBasicBlock::addPredecessor such as we call setHasBytecodeUses on all formal arguments Phis.  The bad point about this solution is that it might need some register allocation for a variable that we don't use.  This solution might be easy but might also have a performance impact.

2/ Another option would be to avoid restoring from the formal arguments if the function is using lazy arguments, because in such case our current compilation scheme forbids setting formal arguments when we have lazy arguments.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Alex Keybl [:akeybl] from comment #2)
> Would you all mind providing context as to the user/dev impact here if left
> unfixed?

Did not show-up until now.  Arguments are not extremely frequent, and usually, when they are used with formal arguments, the formal arguments are used at least once, which either prevent such a bug or forbids Ion compilation.  This bug appears when there is an invalidation or a bailout, which might happen after coming back from an invalidating GC called under such function and which are still unlikely.

I don't know how frequent this kind of function are on the web, but I don't expect this bug to be likely.

Comment 5

[Nicolas B. Pierron [:nbp]]

This is messing up with TI information by returning an unexpected type.  TI might crash in an undocumented/ungarded way after that. => sec-critical

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: Keep formal arguments when they can be accessed with the argument object.

Comment 7

[Daniel Veditz [:dveditz]]

jandem: you marked this as only affecting FF18 and 19 (and Nicolas said it was a regression). Do you know what bug/checking caused the regression?

Comment 8

[Daniel Veditz [:dveditz]]

jandem: you marked this as only affecting FF18 and 19 (and Nicolas said it was a regression). Do you know what bug/checking caused the regression?

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Daniel Veditz [:dveditz] from comment #7)
> jandem: you marked this as only affecting FF18 and 19 (and Nicolas said it
> was a regression). Do you know what bug/checking caused the regression?

It's an IonMonkey problem so it must be Firefox 18+, and I could reproduce on 18 and 19. I'm pretty sure it's bug 735406. Nicolas can you confirm this?

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> (In reply to Daniel Veditz [:dveditz] from comment #7)
> > jandem: you marked this as only affecting FF18 and 19 (and Nicolas said it
> > was a regression). Do you know what bug/checking caused the regression?
> 
> It's an IonMonkey problem so it must be Firefox 18+, and I could reproduce
> on 18 and 19. I'm pretty sure it's bug 735406. Nicolas can you confirm this?

I confirm.
The regression flag was set, as this is was working fine in JM, before IonMonkey landing.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 681357 [details] [diff] [review]
Keep formal arguments when they can be accessed with the argument object.

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

::: js/src/ion/MIRGraph.cpp
@@ +156,5 @@
>          slots_[i] = from->slots_[i];
>  }
>  
> +void
> +MBasicBlock::protectFormalArgs()

Would it instead be possible to change IsPhiObservable to include this logic instead? We can access the block->info() and, I think, determine everything there, like we do for |this|:

    if (phi->slot() == 1)
        return true;

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: Formal arguments are observable after a bailout via the arguments object.

Follow previous suggestion to pacth IsPhiObservable instead of adding extra code to the MBasicBlock.

Comment 13

[Nicolas B. Pierron [:nbp]]

Offuscated & test-less & comment-less patch: (Keep formal arguments when they can be accessed with the argument object.)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad8812349f2

Set in-testsuite? and flag as leave open to land the test case present in the attached patch.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ad8812349f2

(Typically we don't keep bugs open for tests to be checked in, but I'll leave that up to you)

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 682681 [details] [diff] [review]
Formal arguments are observable after a bailout via the arguments object.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 735406

User impact if declined: see comment 4 & comment 5

Testing completed (on m-c, etc.): see comment 13

Risk to taking this patch (and alternatives if risky):
A possible slowdown on code having a lazy argument, should not be noticeable in practice.

String or UUID changes made by this patch: N/A

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 682681 [details] [diff] [review]
Formal arguments are observable after a bailout via the arguments object.

[Triage Comment]
IonMonkey sec-critical fix. Approving for Aurora/Beta.

Comment 17

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/22e3305ba6d0
https://hg.mozilla.org/releases/mozilla-beta/rev/32595106f709

Comment 18

[Josh Aas]

When are you planning to land the tests?

Comment 19

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b1ee0294ca (test case)

Comment 20

[Ed Morley [:emorley]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #19)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b1ee0294ca (test
> case)

https://hg.mozilla.org/mozilla-central/rev/a8b1ee0294ca