1071024 - ARM simulator: support load-exclusive, store-exclusive, sign extend, and memory barriers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

Attachment: armsim-concurrency.diff

For the atomics work I need some instructions that haven't been needed before.  This patch implements those.

Comment 1

[Lars T Hansen [:lth]]

Note that:

- LDREX and STREX are available from ARMv6
- LDREXH, LDREXB, LDREXD, STREXH, STREXB, and STREXD are available from ARMv6K.  When compiling
  for straight ARMv6 it's OK to use the word instructions for the byte and halfword instructions,
  but for the doubleword instructions one needs to be more clever (currently I don't use those).
  Said cleverness is provided as a system call on Linux, according to one reporter.
- STXB and STXH are available from ARMv6.
- DMB, DSB, and ISB are available from ARMv7.  For ARMv6, one can use a coprocessor trap to effect
  a DSB, and if I ever find my notes on that I will make support for that a separate patch.  Code
  compiled for ARMv6 should then emit that trap, and not use the DMB (sic) that would normally be used.

Note also that there are some deprecated synchronization instructions, SWP and SWPB, that I've not implemented here.

Comment 2

[Lars T Hansen [:lth]]

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

Patches on bug 979594 will make use of these changes and will provided test cases.

Comment 3

[Ed Morley [:emorley]]

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

Comment 4

[Lars T Hansen [:lth]]

Also see https://hg.mozilla.org/integration/mozilla-inbound/rev/1c34f6f342cd for a minor bugfix (r=me).

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1c34f6f342cd

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8493113 [details] [diff] [review]
armsim-concurrency.diff

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

Some drive-by style nits.

Marty, please point those out next time. I spent a lot of time converting the simulator's style when I initially imported it...

::: js/src/jit/arm/Simulator-arm.cpp
@@ +2507,5 @@
> +                        switch (instr->bits(22,21)) {
> +                        case 0:
> +                            set_register(rt, readW(address, instr));
> +                            break;
> +                        case 1:

Nit: cases and defaults are indented with 2 spaces, also a bunch of times below.

@@ +4035,5 @@
>        case 0xA:
> +        if (instr->bits(31,20) == 0xf57) {
> +            // Minimal simulation: do nothing.
> +            //
> +            // If/when we upgrade load-exclusive and store-exclusive (above) to do something

Comments should fit within 80 columns, 99 for code.

Comment 7

[Lars T Hansen [:lth]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> Comment on attachment 8493113 [details] [diff] [review]
> armsim-concurrency.diff
> 
> Review of attachment 8493113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some drive-by style nits.

Will fix these, by and by.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40b534ef781d