Closed Bug 1554053 Opened 1 year ago Closed 1 year ago

Wasm via Cranelift: size of interrupt check comparison is incorrect

Categories

(Core :: Javascript: WebAssembly, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: jseward, Assigned: jseward)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Wasm-via-Cranelift generates code to do interrupt checks by doing a machine-word sized comparison. However, the compared-against value, TlsData::interrupt, is an Atomic<uint32_t, mozilla::Relaxed>, and so the comparison is incorrect on all 64 bit targets: it also compares the 4 bytes following TlsData::interrupt, which look to me as if they are an alignment hole (iow, junk).

This is obviously incorrect, and it's observably inconsistent with what the -via-Ion and -baseline routes do, which is to always generate a 32-bit comparison.

It also holds a potential danger of a store-forwarding stall (big read after small write), although, based on the struct layout and detailed reading of the Intel opt guide, I think it's probably harmless for the Intel Core architecture family.

This patch changes Wasm-via-CL to always use a 32-bit comparison for interrupt checks.

Wasm-via-Cranelift generates code to do interrupt checks by doing a
machine-word sized comparison. However, the compared-against value,
TlsData::interrupt, is an Atomic<uint32_t, mozilla::Relaxed>, and so the
comparison is incorrect on all 64 bit targets: it also compares the 4 bytes
following TlsData::interrupt, which look to me as if they are an alignment
hole (iow, junk).

This is obviously incorrect, and it's observably inconsistent with what the
-via-Ion and -baseline routes do, which is to always generate a 32-bit
comparison.

It also holds a potential danger of a store-forwarding stall (big read after
small write), although, based on the struct layout and detailed reading of the
Intel opt guide, I think it's probably harmless for the Intel Core
architecture family.

Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c858f52192ad
Wasm via Cranelift: size of interrupt check comparison is incorrect. r=bbouvier
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.