Closed
Bug 1102278
Opened 10 years ago
Closed 9 years ago
IonMonkey MIPS: Test ion/bug883490.js is failing on MIPS
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: rankov, Assigned: rankov)
Details
Attachments
(1 file, 1 obsolete file)
2.63 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
The test ion/bug883490.js is failing on MIPS
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → branislav.rankov
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•10 years ago
|
||
The problem was that GlobalReg and HeapReg can get clobbered by Ion and Baseline. This is similar to bug 1046164. Only now, there is also changeHeap to be considered.
Attachment #8526062 -
Flags: review?(luke)
Comment 2•10 years ago
|
||
Comment on attachment 8526062 [details] [diff] [review] GlobalReg-clobber.patch Review of attachment 8526062 [details] [diff] [review]: ----------------------------------------------------------------- Wow, great catch! I guess this is a problem for ARM too, huh. I guess we never hit it because it's on the error path. ::: js/src/asmjs/AsmJSValidate.cpp @@ +8163,5 @@ > + JS_STATIC_ASSERT(MaybeSavedGlobalReg > 0); > + masm.loadPtr(Address(StackPointer, savedGlobalOffset), GlobalReg); > +#else > + JS_STATIC_ASSERT(MaybeSavedGlobalReg == 0); > +#endif Instead of adding an extra throw path, what about this: 1. put this reload of GlobalReg right before branchTestMagic 2. move the pre-existing reload of GlobalReg into the oolConvert path, right after the call and before the jump(&done) 3. keep the heap-detachment check where it is, at the done join point @@ +8168,5 @@ > + > + // The heap pointer has to be reloaded anyway since Ion could have clobbered > + // it. Additionally, the FFI may have detached the heap buffer. > + masm.loadAsmJSHeapRegisterFromGlobalData(); > + GenerateCheckForHeapDetachment(m, ABIArgGenerator::NonReturn_VolatileReg0); I don't think heap detachment is a problem for the throw path (we'll catch it in the CallAsmJS guard if someone tries to call into asm.js), so I think we can just have one check, after the 'done' label.
Updated•10 years ago
|
Attachment #8526062 -
Flags: review?(luke)
Assignee | ||
Comment 3•9 years ago
|
||
If we don't care about HeapReg in the throw path, then this should be enough. We don't need to restore GlobalReg after the oolConvert because we are calling C++ code and GlobalReg is chosen to be callee saved.
Attachment #8526062 -
Attachment is obsolete: true
Attachment #8528421 -
Flags: review?(luke)
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d461621a6254 https://tbpl.mozilla.org/?tree=Try&rev=d461621a6254
Comment 5•9 years ago
|
||
Comment on attachment 8528421 [details] [diff] [review] GlobalReg-clobber.patch Review of attachment 8528421 [details] [diff] [review]: ----------------------------------------------------------------- Good point!
Attachment #8528421 -
Flags: review?(luke) → review+
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e0ebb7cb962
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1e0ebb7cb962
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•