998997 - Fix AsmJSExceptionHandler on 32bit OS X 10.6 and Windows XP

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Shu-yu Guo [:shu]]

HandleSignal just doesn't get called when interrupt is requested.

Comment 1

[Shu-yu Guo [:shu]]

Okay, with some help from froydnj we figured this out.

10.6 is just so secure in its security that it hands out PROT_EXEC bits like candy in 32bit land. We should be using PROT_NONE when making jitcode inaccessible.

Comment 2

[Shu-yu Guo [:shu]]

I see that the mprotect code *used to* set PROT_NONE but was changed to PROT_READ | PROT_WRITE. Why was this change done?

As I mentioned above, this is causing bugs on 32bit 10.6 and Windows XP. I can't be sure what's going on in XP land, but for 10.6, search for |override_nx| in the following file, especially the call to it inside |vm_map_protect|: http://www.opensource.apple.com/source/xnu/xnu-1504.15.3/osfmk/vm/vm_map.c

Comment 3

[Shu-yu Guo [:shu]]

Ah, I found https://bugzilla.mozilla.org/show_bug.cgi?id=970643#c10, which says it's GC-related. Well, that seems like we're shit out of luck then. I suppose we have to disable signal-based handlers on 32bit 10.6 and WinXP?

Comment 4

[Shu-yu Guo [:shu]]

Attachment: testcase

Push this test to a 10.6 slave and run with a 32bit shell with --ion-parallel-compile=off to reproduce.

TBPL runs 10.6 Opt jit-tests with a 32bit shell. The Debug shell is always 64bit, so be sure to request Opt if you're pushing to try.

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8409985 [details]
testcase

Oops, wrong test.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Test

This test should fail to terminate on 32bit OS X 10.6 and Windows XP.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Check NX brokenness before installing JIT signal handlers.

The NX bit is broken on 32bit OS X 10.6 and possibly on Windows XP. Check for
brokenness before enabling signal-based handlers.

Comment 8

[Shu-yu Guo [:shu]]

Try push of just the test to indicate brokenness: https://tbpl.mozilla.org/?tree=Try&rev=8ee9dfffcfbb

Try push of the test + fix: https://tbpl.mozilla.org/?tree=Try&rev=5ef3f43cdcb1

Comment 9

[Luke Wagner [:luke]]

(In reply to Shu-yu Guo [:shu] from comment #3)
> Ah, I found https://bugzilla.mozilla.org/show_bug.cgi?id=970643#c10, which
> says it's GC-related. Well, that seems like we're shit out of luck then. I
> suppose we have to disable signal-based handlers on 32bit 10.6 and WinXP?

Not that 32-bit OSX 10.6 matters, but I thought/think that making the executable code read/write instead of PROT_NONE was unnecessary (https://bugzilla.mozilla.org/show_bug.cgi?id=970643#c12).  Could you just revert nbp's patch and see if that fixes the 10.6 problems?

For WinXP, http://msdn.microsoft.com/en-us/library/windows/desktop/bb736299%28v=vs.85%29.aspx seems to indicate that you can just use the function #included by windows.h without the GetProcAddress.  Does that not work?

Comment 10

[Brian Hackett [Laid off!]]

Comment on attachment 8410060 [details] [diff] [review]
Check NX brokenness before installing JIT signal handlers.

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

It would be nice if this patch wasn't necessary.  At least for the vanilla Ion signal handlers the protection can just be PROT_NONE rather than PROT_READ | PROT_WRITE (i.e. revert nbp's patch in bug 970643), and I'll defer to Luke as to whether read/write faults on asm.js code are correctly handled when they happen during GC and whether the signal mechanism works right on Windows XP.

Comment 11

[Shu-yu Guo [:shu]]

(In reply to Brian Hackett (:bhackett) from comment #10)
> Comment on attachment 8410060 [details] [diff] [review]
> Check NX brokenness before installing JIT signal handlers.
> 
> Review of attachment 8410060 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would be nice if this patch wasn't necessary.  At least for the vanilla
> Ion signal handlers the protection can just be PROT_NONE rather than
> PROT_READ | PROT_WRITE (i.e. revert nbp's patch in bug 970643), and I'll
> defer to Luke as to whether read/write faults on asm.js code are correctly
> handled when they happen during GC and whether the signal mechanism works
> right on Windows XP.

After talking with Luke I'll take this suggestion here and just use PROT_NONE and PAGE_NOACCESS over this feature detection BS. Clearing NI from nbp.

Comment 12

[Shu-yu Guo [:shu]]

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

r=luke over IRC

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #12)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c2672cd82c95
> 
> r=luke over IRC

Can we add the test case back?