Wasm atomics must use standard bounds checking

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P1
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

9 months ago
(Forked from bug 1432577)

Currently we do not emit metadata for atomic accesses, so when we get a segv in wasm code without metadata we "know" it was an atomic oob access.  This was acceptable because asm.js used the same mechanism for its simd and atomics, but those are gone now, and we should clean this up for wasm: atomics need to emit appropriate metadata for the first (temporally speaking) memory access that each atomic operation will do.

Since we are using native atomic operations for all atomic accesses on all platforms this should not be all that difficult.
(Assignee)

Comment 1

9 months ago
See bug 1423577 comment 11.  Carrying Luke's f+, but note from that comment that more can be done here:

> I think you can delete LinkDataTier::outOfBoundsOffset too which will trigger another small transitive wave of removals.
Attachment #8997831 - Flags: feedback+
(Assignee)

Comment 2

9 months ago
Note, atomic load and store appear already to work because the common wasmLoad / wasmStore infrastructure emits the correct metadata.
(Assignee)

Updated

9 months ago
Depends on: 1343981
(Assignee)

Comment 3

9 months ago
Posted patch bug1481171-wasm-atomics.patch (obsolete) — Splinter Review
For safekeeping, almost ready for review.  Need to rework the append() calls for ARM64, they should not use masm.size() but masm.currentOffset().  Every platform has a different mechanism here, sigh.
(Assignee)

Comment 4

9 months ago
One note about this: On ARM64, this follows existing code in how it emits metadata for traps, namely, masm.append(access,masm.currentOffset()).  However, I suspect that that is incorrect, because currentOffset() calls nextOffset() which does *not* flush constant pools.  nextInstrOffset() does flush the pools though.  I will be investigating the matter immediately, but there's no particular reason to hold up this bug for that.
Attachment #8998513 - Attachment is obsolete: true
Attachment #8998811 - Flags: review?(luke)
(Assignee)

Comment 5

9 months ago
Now with even more code removal.
Attachment #8997831 - Attachment is obsolete: true
Attachment #8998812 - Flags: review?(luke)
(Assignee)

Updated

9 months ago
Blocks: 1477743
No longer blocks: 1444925
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1423496
(In reply to Lars T Hansen [:lth] from comment #4)
> Created attachment 8998811 [details] [diff] [review]
> bug1481171-wasm-atomics-v2.patch
> 
> One note about this: On ARM64, this follows existing code in how it emits
> metadata for traps, namely, masm.append(access,masm.currentOffset()).

I *thought* those cases were supposed all be guarded by AutoForbidPools; if they aren't, that seems like a bug.  I wasn't aware of nextInstrOffset(); that sounds better.  Either way, glad you'll be looking into next.
Comment on attachment 8998811 [details] [diff] [review]
bug1481171-wasm-atomics-v2.patch

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

Great!  Nice to see that passing around the MemoryAccessDesc actually tidies up a few places.
Attachment #8998811 - Flags: review?(luke) → review+
Comment on attachment 8998812 [details] [diff] [review]
bug1481171-rm-signal-handling-fallback-v2.patch

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

\o/
Attachment #8998812 - Flags: review?(luke) → review+

Comment 10

9 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/413be91a0257
Wasm-specific atomics operations.  r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7bd965f86b
Remove failure path for lookupTrap().  r=luke
(Assignee)

Comment 11

9 months ago
(In reply to Luke Wagner [:luke] from comment #7)
> (In reply to Lars T Hansen [:lth] from comment #4)
> > Created attachment 8998811 [details] [diff] [review]
> > bug1481171-wasm-atomics-v2.patch
> > 
> > One note about this: On ARM64, this follows existing code in how it emits
> > metadata for traps, namely, masm.append(access,masm.currentOffset()).
> 
> I *thought* those cases were supposed all be guarded by AutoForbidPools; if
> they aren't, that seems like a bug.  I wasn't aware of nextInstrOffset();
> that sounds better.  Either way, glad you'll be looking into next.

It's a bit of a mess, basically.  *One* instance uses AutoForbidPools in a reasonable way; *one* gets the instruction offset from the code emitter and append()s the metainfo afterward (ARM-style - I'm pretty sure I wrote this one because I tripped over a problem); existing wasmLoad and wasmStore just append() information before the instruction and hope for the best, as do the new atomic ops.

AutoForbidPools is a blunt instrument as one has to know a bit about the actual machine instructions emitted before it can be trusted; the MacroAssembler layers get in the way here.  But it looks doable to provide the necessary guarantees.
Keywords: leave-open
(Assignee)

Comment 13

9 months ago
Attachment #8999135 - Flags: review?(luke)

Updated

9 months ago
Attachment #8999135 - Flags: review?(luke) → review+

Comment 14

8 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9112406a54a2
Properly emit signal handling metadata for wasm on aarch64.  r=luke
(Assignee)

Updated

8 months ago
Keywords: leave-open

Comment 15

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9112406a54a2
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Updated

8 months ago
Blocks: 1482932
(Assignee)

Comment 16

8 months ago
Dragan, the first patch on this bug definitely affects MIPS, see the commit comments in the patch.
(In reply to Lars T Hansen [:lth] from comment #16)
> Dragan, the first patch on this bug definitely affects MIPS, see the commit
> comments in the patch.

Thanks for the heads up. Will look into it.
You need to log in before you can comment on or make changes to this bug.