Closed Bug 857838 Opened 7 years ago Closed 7 years ago

BaselineCompiler: Assertion failure: returnAddr > method_->raw(), at ion/BaselineJIT.cpp:470


(Core :: JavaScript Engine, defect, major)

Other Branch
Not set



Tracking Status
firefox22 --- unaffected
firefox23 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected


(Reporter: decoder, Assigned: djvj)


(Blocks 2 open bugs)


(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update])


(1 file, 1 obsolete file)

The following testcase asserts on baseline compiler branch revision 5fd27c1b3943 (run with --ion-eager):

function g(f,obj) {}
gcparam("maxBytes", gcparam("gcBytes") + 4*.53 );\
function f(b) {\
    if (b) {\
        f(b - 1);\
        g = {\
            apply:function(x,y) {}\
    g.apply(null, arguments);\
f(400 >> false);\
This assert is getting hit inside stack iterator which is instantiated within a call to FinishBailoutToBaseline.

It assertbs because the iterator hits a "fake exit frame" constructed for a BaselineJS frame => ABI call.  We probably need to use the actual return address into jitcode here instead of a fake NULL return address.
Attached patch Fix. (obsolete) — Splinter Review
Sets the actual return address for fake exit frame to the |FinishBailoutToBaseline| call, instead of bogus NULL return addr.
Attachment #733457 - Flags: review?(jdemooij)
Comment on attachment 733457 [details] [diff] [review]

Review of attachment 733457 [details] [diff] [review]:

::: js/src/ion/IonMacroAssembler.cpp
@@ +839,4 @@
>          makeFrameDescriptor(temp, IonFrame_BaselineJS);
>          push(temp);
> +        // Push return address.
> +        loadPtr(Address(bailoutInfo, offsetof(BaselineBailoutInfo, resumeAddr)), temp);

We are crashing in icEntryFromReturnAddress, but the resumeAddr does not necessarily have an ICEntry. Why are we walking the stack? If we can't avoid that we could do something like the setReturnAddress call in BaselineFrame::initForOsr: store this address in BaselineBailoutInfo and push it here...
Comment on attachment 733457 [details] [diff] [review]

Review of attachment 733457 [details] [diff] [review]:

See comment 3.
Attachment #733457 - Flags: review?(jdemooij)
Assignee: general → kvijayan
Is this just going to always try to execute NULL, making this just a safe crash?
Given the patch the current crash is "safe" and we could un-hide this bug. But if we've pushed the null because we're confused will pushing some other value make the problem worse?
We should probably keep it secure, because the issue is not dereferencing NULL, but doing a table-lookup using a pointer as a key, but in this case the pointer is NULL.  In opt cases we'll probably end up returning a random table entry.
Thought a bit about your comment, and came up with this.

Changing the NULL returnAddr to an actdual returnAddr (as in the previous patch) is necessary.  The issue, as you brought up, is that this returnAddr isn't always associated with an ICEntry in the baseline jit.

I think we can state with certainty that the returnAddr is _either_ going to be returning immediately after an IC (or fake IC for a CallVM form the main jitcode), or it's going to be returning into the start-of-jitcode for a particular op.

In the first case, we can use icEntryFromReturnAddress.  In the second case, we can use the PCMapping table to do a lookup.

The iterator doesn't know ahead of time which kind of return address it is getting, but it can just try both.

Main changes in this patch:
1. Add a maybeICEntryFromReturnAddress() variant for fallible IC lookup.
2. Add pcForReturnAddress() methods that use the PCMapping table on BaselineScripts.
3. Change baselineScriptAndPC to try the ICEntry method first, and fall back to the PCMapping table method if that doesn't work.

I tried to write up a test case for this, but it's hard.  Simulating a situation where we GC in the middle of a BailoutToBaseline, but the BailoutToBaseline has to be back to an on-ICed, or non-callVMed op.  Hard to generate those situations.  If you have any ideas lemme know.
Attachment #733457 - Attachment is obsolete: true
Attachment #739247 - Flags: review?(jdemooij)
Comment on attachment 739247 [details] [diff] [review]
Fix Stack Iteration.

Cancelling review on this, there are cases this doesn't handle.
Attachment #739247 - Flags: review?(jdemooij)
Comment on attachment 739247 [details] [diff] [review]
Fix Stack Iteration.

Re-enabling review.  I was mistaken earlier - this should actually handle the issue just fine.  I thought that sometimes we set the resumeAddr to be the first pointer into a TypeCheck stubcode, but that's not the case.
Attachment #739247 - Flags: review?(jdemooij)
Comment on attachment 739247 [details] [diff] [review]
Fix Stack Iteration.

Review of attachment 739247 [details] [diff] [review]:

Nice. As you said it's hard to trigger this, so it would be good to add some #ifdef DEBUG code to FinishBailoutToBaseline to get the current pc, something like:

#ifdef DEBUG
    jsbytecode *pc;

r=me with that + a comment.

::: js/src/ion/BaselineJIT.cpp
@@ +598,5 @@
>      JS_NOT_REACHED("Invalid pc");
>      return NULL;
>  }
> + 

Nit: trailing whitespace.
Attachment #739247 - Flags: review?(jdemooij) → review+
Good call on that assert, it uncovered a couple more unhandled cases: resuming into the script prologue, and a boundary error in the binary search in icEntryForReturnAddr.  Both fixed and pushed:
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Flags: in-testsuite+ → in-testsuite?
JSBugMon: This bug has been automatically verified fixed.
Depends on: 877826
Depends on: 928889
You need to log in before you can comment on or make changes to this bug.