Make non-natural alignment a validation error in spec interpreter / test - #242
Conversation
stevenfontanella
changed the title
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
LGTM. Let's see what @conrad-watt and @rossberg think.
rossberg
left a comment
rossberg
left a comment
There was a problem hiding this comment.
Looks good, I only have stylistic nits regarding the validator change.
|
CI keeps failing due to setup problems. The workflows in this repo are outdated, maybe that contributes to it. Can you please try to update |
Looks like that worked and the new CI failure is a spec test that fails from |
|
Looks like this commit removed the failing tests in the upstream. Besides that it looks like quite a few tests fail from |
|
Hm, I'm a bit confused now, in the latest CI I only see a test failure for SB.wast, which isn't from upstream? |
|
I see. It indeed fails with I'm not too sure about the error itself but I see that this commit in the upstream disabled JS tests in CI and ran the equivalent of my succeeding test. Maybe we should port that to our CI as well? I don't know what the motivation of the upstream commit was. |
|
Opened #244 to see if the CI can be fixed easily but I don't have permission to run it. Can you check if that helps? |
90a44b9 to
8737666
Compare
|
I don't have write access here. Can one of you merge this or give me write access? |
Fixes #8962 Atomic memory instructions were parsing `memarg` alignment but not keeping it in the IR for RMW, cmpxchg, wait, and notify. The builder also forced natural alignment, so invalid alignment from WAT or binary input was silently normalized and passed validation. The threads proposal requires atomic accesses to use natural alignment (see WebAssembly/threads#242). Binaryen should reject invalid modules rather than accept them. This PR stores `align` on all atomic memory IR nodes, threads it through WAT/binary parsing and printing, and validates it centrally via the existing `validateAlignment(..., isAtomic=true)` check. The DeAlign pass skips atomic load/store so it does not rewrite their alignment. Adds lit tests and unskips `threads/atomic.wast`. ## Test plan - [x] `test/lit/validation/atomic-alignment.wast` - [x] `test/lit/passes/dealign-atomics.wast` - [x] `threads/atomic.wast` (unskipped) --------- Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
3 participants
Followup to #243. Prior to #243, a non-natural alignment for an atomic instruction was not a parse, validation or runtime failure in the spec interpreter / tests, although the spec specified that it would be a parse error (and would be a validation error after #243). Change the reference interpreter and spec test to make a non-natural alignment into a validation error. 8-bit instructions aren't tested for the validation failure since there's no way to have a non-natural alignment for those cases (only a greater one which results in a different validation failure).
Also removed some extra spaces in cmpxchg tests.
Tested with
make testlocally.