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

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla35
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Created attachment 8493113 [details] [diff] [review]
armsim-concurrency.diff

For the atomics work I need some instructions that haven't been needed before.  This patch implements those.
Attachment #8493113 - Flags: review?(mrosenberg)
(Assignee)

Updated

4 years ago
Blocks: 1054841
(Assignee)

Comment 1

4 years ago
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+
(Assignee)

Comment 2

3 years ago
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

3 years ago
https://hg.mozilla.org/mozilla-central/rev/ed7803d5ff87
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(Assignee)

Comment 4

3 years ago
Also see https://hg.mozilla.org/integration/mozilla-inbound/rev/1c34f6f342cd for a minor bugfix (r=me).
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.
(Assignee)

Comment 7

3 years ago
(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.