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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

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;

Comment 1

[Jan de Mooij [:jandem]]

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? :)

Comment 2

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

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.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: 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 :)

Comment 4

[Jan de Mooij [:jandem]]

(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

[Shu-yu Guo [:shu]]

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()).

Comment 6

[Shu-yu Guo [:shu]]

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.

Comment 7

[Shu-yu Guo [:shu]]

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.

Comment 8

[Jan de Mooij [:jandem]]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Jan de Mooij [:jandem]]

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.

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

Given that you understand the problems quite well (comment 3), can you verify the fix in the latest Nightly before we uplift to 37?

Comment 12

[Jan de Mooij [:jandem]]

(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).

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8562179 [details] [diff] [review]
Patch

Thank you for verifying. Aurora+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69ce849146bf