Closed Bug 1203044 Opened 4 years ago Closed 4 years ago

IonMonkey: MIPS32: Atomics operations should throw on oob access

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: hev, Assigned: hev)

Details

Attachments

(1 file)

Some atomics operations not implemented on mips32, I just attach a part of this patch. I will implement atomics that marked as 'NYI' for mips32 in Bug 1090957 after Bug 1194139 finished.
thanks!
Attachment #8658589 - Flags: review?(lhansen)
Comment on attachment 8658589 [details] [diff] [review]
0001-IonMonkey-MIPS32-Atomics-operations-should-throw-on-.patch

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

I guess this is OK.  The code seems really branchy to me but I will assume you know best, I see the existing load code uses a similar pattern.

I would have expected to see changes to the implementation of compareExchange, add, etc, but perhaps those are implemented with VM callouts?

What worries me with load and store is that there are no memory barriers here.  That would have been correct on MIPS in the old days when it had a sequentially consistent memory model, but my understanding now is that it has a weak memory model a la ARM.  If so, you need barriers before and after the loads and stores, cf ARM.  Can you tell me why those are missing here?
(In reply to Lars T Hansen [:lth] from comment #2)
> Comment on attachment 8658589 [details] [diff] [review]
> 0001-IonMonkey-MIPS32-Atomics-operations-should-throw-on-.patch
> 
> Review of attachment 8658589 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess this is OK.  The code seems really branchy to me but I will assume
> you know best, I see the existing load code uses a similar pattern.
> 
> I would have expected to see changes to the implementation of
> compareExchange, add, etc, but perhaps those are implemented with VM
> callouts?
> 
> What worries me with load and store is that there are no memory barriers
> here.  That would have been correct on MIPS in the old days when it had a
> sequentially consistent memory model, but my understanding now is that it
> has a weak memory model a la ARM.  If so, you need barriers before and after
> the loads and stores, cf ARM.  Can you tell me why those are missing here?

Thank you. I plan implement these atomics (contain memoryBarrier) in Bug 1090957. I'm doing extract shareable code from MacroAssemblerMIPS to mips-shared directory, so i think attch atomics after that finished might a good way. If possible, please help review this patch in mips64 is ok? thx.

https://github.com/heiher/gecko-dev/commit/a0c6a43fea16bd49af011ecbce253d4afabd6093
Comment on attachment 8658589 [details] [diff] [review]
0001-IonMonkey-MIPS32-Atomics-operations-should-throw-on-.patch

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

Fair enough.
Attachment #8658589 - Flags: review?(lhansen) → review+
BTW, I only scanned it briefly - not a proper review at this point - but the MIPS64 code you posted looks right.  Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ec890000d297
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.