Assertion failure: returnAddr > method_->raw(), at BaselineJIT.cpp:647

RESOLVED FIXED in Firefox 37

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla38
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Testcase below asserts with --baseline-eager (inbound rev 9a3a666c17d7).

var g = newGlobal();
g.parent = this;
g.eval("(" + function() {
    var dbg = new Debugger(parent);
    dbg.onExceptionUnwind = function(frame) {
	frame.onPop = function() { frame.eval("x = 3"); };
    };
} + ")()");
Object.defineProperty(this, "x", {set: [].map});
x = 1;
(Assignee)

Comment 1

4 years ago
returnAddr is nullptr and we have an override pc so this could be from bug 1118826. Will look into this more next week.

Gary can you run autoBisect on this? :)
Flags: needinfo?(jdemooij)
Flags: needinfo?(gary)
I can't reproduce this on Mac 64-bit js debug shell:

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-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

on m-c rev 9a3a666c17d7. Not even on treeherder js shells either.
Flags: needinfo?(gary)
(Assignee)

Comment 3

4 years ago
Created attachment 8562179 [details] [diff] [review]
Patch

There were two issues:

(1) We were in HandleException, called an onExceptionUnwind hook and triggered DebugModeOSR. DebugModeOSR then set the frame's return address to nullptr, because we will never return to that frame.

Then, while still in HandleException, we called an onPop hook which triggered annother DebugModeOSR and we got confused by the nullptr return address.

The simplest fix for this was to set the is-handling-exception flag for the whole duration of HandleException, so that DebugModeOSR will use the override pc also when we're executing the DebugEpilogue/onPop handler.

(2) After that, there was a problem with CloneOldBaselineStub. When HandleException unwinds scopes, it can modify the frame's override pc. This however means that if we have a stub frame on the stack, the stub frame's fallbackStub will no longer match this (modified) pc, so CloneOldBaselineStub asserted.

The patch sets the stub frame's stub pointer to nullptr in this case. I think that's fine as HandleException will never return to this stub frame.

Let me know if this all makes sense. The patch could use a careful review :)
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8562179 - Flags: review?(shu)
(Assignee)

Comment 4

4 years ago
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> I can't reproduce this on Mac 64-bit js debug shell:

Thanks for looking into this :) It reproduced here on OS X with both 32-bit and 64-bit shells, debug and debug+opt, with and without --baseline-eager.

Comment 5

4 years ago
Comment on attachment 8562179 [details] [diff] [review]
Patch

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

Thanks a lot for the fix! I missed this bug when it was first filed. The explanation and the high-level fix are right.

To be extra safe, could you assert !frame_->isHandlingException() in DebugModeOSRVolatileStub::invalid?

I'd like the code reorganized a bit. Specific comments inline below. r=me with those addressed.

::: js/src/jit/BaselineDebugModeOSR.cpp
@@ +220,5 @@
> +                // the entry's pcOffset we just set doesn't necessarily match
> +                // the stub that's still on the stack. To prevent that, we just
> +                // null prevFrameStubPtr, as the exception handler will never
> +                // return to this stub frame anyway.
> +                prevFrameStubPtr = nullptr;

I think it would be cleaner to instead patch CloneOldBaselineStub to set |entry.newStub = nullptr| and return early when |entry.frameKind == ICEntry::Kind_Invalid|. We shouldn't have invalid ICEntries with stub pointers other than via exception handling.

The comment is nice though and should accompany the code in CloneOldBaselineStub instead.

@@ +589,5 @@
> +                MOZ_ASSERT(!entry.oldStub);
> +                MOZ_ASSERT(!entry.newStub);
> +                layout->setStubPtr(nullptr);
> +                break;
> +            }

Along with the change above, this block would go away as well. Instead, the MOZ_ASSERT(entry.newStub) below would be changed to MOZ_ASSERT(entry.newStub || prevFrame->isHandlingException()).
Attachment #8562179 - Flags: review?(shu) → review+

Comment 6

4 years ago
Comment on attachment 8562179 [details] [diff] [review]
Patch

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

Actually, wait. I'm debugging this myself right now and I'm not seeing the call to onPop causing the assert, but a second call to onExceptionUnwind.
Attachment #8562179 - Flags: review+

Comment 7

4 years ago
Comment on attachment 8562179 [details] [diff] [review]
Patch

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

I'm dumb. Forgot it was --baseline-eager. r+ stands.
Attachment #8562179 - Flags: review+
(Assignee)

Comment 8

4 years ago
Good suggestions. Pushed with those addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f581bbbe990d

NI myself to request uplift after this has been in a few nightlies.
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/f581bbbe990d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Comment 10

4 years ago
Comment on attachment 8562179 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Maybe bug 1118826, not sure.
[User impact if declined]: Crashes or other problems when using the debugger.
[Describe test coverage new/current, TreeHerder]: Patch adds a test and has been in a few nightlies.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.
Flags: needinfo?(jdemooij)
Attachment #8562179 - Flags: approval-mozilla-aurora?
status-firefox37: --- → affected
Given that you understand the problems quite well (comment 3), can you verify the fix in the latest Nightly before we uplift to 37?
Flags: needinfo?(jdemooij)
(Assignee)

Comment 12

4 years ago
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #11)
> Given that you understand the problems quite well (comment 3), can you
> verify the fix in the latest Nightly before we uplift to 37?

I just verified the test in comment 0 no longer asserts (the patch also adds this test to our test suite, FWIW).
Flags: needinfo?(jdemooij)
Comment on attachment 8562179 [details] [diff] [review]
Patch

Thank you for verifying. Aurora+
Attachment #8562179 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.