ARMv6: Do not inline atomics if the assembler will MOZ_CRASH

RESOLVED FIXED in Firefox 39



4 years ago
4 years ago


(Reporter: lth, Assigned: lth)


(Blocks: 1 bug)


Firefox Tracking Flags

(firefox39 fixed)



(2 attachments)



4 years ago
The ARM assembler does not yet support atomics for ARMv6 before ARMv6-K (since 1-byte and 2-byte atomics must be simulated with 4-byte atomics and there's some hair in implementing that).  The assembler will MOZ_CRASH if it tries to generate code for ARMv6.  A better thing to do would be to not inline the atomics on such platforms.

Comment 1

4 years ago
Hm, not totally trivial to use that workaround for asm.js.

Comment 2

4 years ago
Created attachment 8582483 [details] [diff] [review]
Disable Atomics / SAB on ARM until we have ARMv6 support

To handle asm.js we pretty much need to implement the support for ARMv6 and I had hoped today would not be the day for that, but it may be the only fix that works.

The attached patch just disables the shared memory types and Atomics on ARM.  But this will run afoul of test cases that test that those names *are* defined in nightly builds (dom/workers/test/test_worker_interfaces.js is just one of those tests).  I'm storing it here in case it is useful, but I do not intend to land it.

Comment 3

4 years ago
Created attachment 8582596 [details] [diff] [review]
Do not inline the Atomics if we're on an ARMv6

This takes care of the Ion piece of the puzzle.  The patch I've offered on bug 1077318 will take care of the asm.js piece.

Together, when I force ARMv6 in the ARM simulator, these two patches make all NYI crashes go away, and all my tests pass.
Attachment #8582596 - Flags: review?(jdemooij)
Comment on attachment 8582596 [details] [diff] [review]
Do not inline the Atomics if we're on an ARMv6

Review of attachment 8582596 [details] [diff] [review]:


::: js/src/jit/Ion.cpp
@@ +3061,5 @@
> +bool
> +jit::JitSupportsAtomics()
> +{

Super nit: #ifdef? #if probably also works but we use #ifdef everywhere else.
Attachment #8582596 - Flags: review?(jdemooij) → review+

Comment 5

4 years ago

Bug 1077318 will also have to land before the problem goes away fully, though.
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.