Closed Bug 1071024 Opened 5 years ago Closed 5 years ago

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


(Core :: JavaScript Engine: JIT, defect)

Not set





(Reporter: lth, Assigned: lth)


(Blocks 1 open bug)



(1 file)

For the atomics work I need some instructions that haven't been needed before.  This patch implements those.
Attachment #8493113 - Flags: review?(mrosenberg)
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.
Attachment #8493113 - Flags: review?(mrosenberg) → review+

Patches on bug 979594 will make use of these changes and will provided test cases.
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment on attachment 8493113 [details] [diff] [review]

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.
(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.
You need to log in before you can comment on or make changes to this bug.