Closed
Bug 998997
Opened 10 years ago
Closed 10 years ago
Fix AsmJSExceptionHandler on 32bit OS X 10.6 and Windows XP
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: shu, Assigned: shu)
References
Details
Attachments
(1 file, 2 obsolete files)
438 bytes,
patch
|
Details | Diff | Splinter Review |
HandleSignal just doesn't get called when interrupt is requested.
Assignee | ||
Comment 1•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Summary: AsmJSMachExceptionHandler doesn't work on 32bit 10.6 → Fix AsmJSExceptionHandler on 32bit OS X 10.6 and Windows XP
Assignee | ||
Comment 2•10 years ago
|
||
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
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(luke)
Assignee | ||
Comment 3•10 years ago
|
||
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?
Assignee | ||
Comment 4•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8409985 [details]
testcase
Oops, wrong test.
Attachment #8409985 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
This test should fail to terminate on 32bit OS X 10.6 and Windows XP.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → shu
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•10 years ago
|
||
The NX bit is broken on 32bit OS X 10.6 and possibly on Windows XP. Check for brokenness before enabling signal-based handlers.
Attachment #8410060 -
Flags: review?(luke)
Attachment #8410060 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 8•10 years ago
|
||
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•10 years ago
|
||
(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?
Flags: needinfo?(luke)
Comment 10•10 years ago
|
||
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.
Attachment #8410060 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Flags: needinfo?(nicolas.b.pierron)
Assignee | ||
Updated•10 years ago
|
Attachment #8410060 -
Attachment is obsolete: true
Attachment #8410060 -
Flags: review?(luke)
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2672cd82c95 r=luke over IRC
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c2672cd82c95
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 14•10 years ago
|
||
(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?
You need to log in
before you can comment on or make changes to this bug.
Description
•