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)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: rankov, Assigned: rankov)

Details

Attachments

(1 file, 1 obsolete file)

The test ion/bug883490.js is failing on MIPS
Assignee: nobody → branislav.rankov
Status: NEW → ASSIGNED
Attached patch GlobalReg-clobber.patch (obsolete) — Splinter Review
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 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.
Attachment #8526062 - Flags: review?(luke)
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)
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+
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.