Skip to content

Validate natural alignment for atomic memory operations - #8966

Open
ANAMASGARD wants to merge 1 commit into
WebAssembly:mainfrom
ANAMASGARD:fix/8962-atomic-alignment-validation
Open

Validate natural alignment for atomic memory operations#8966
ANAMASGARD wants to merge 1 commit into
WebAssembly:mainfrom
ANAMASGARD:fix/8962-atomic-alignment-validation

Conversation

@ANAMASGARD

Copy link
Copy Markdown

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

  • test/lit/validation/atomic-alignment.wast
  • test/lit/passes/dealign-atomics.wast
  • threads/atomic.wast (unskipped)

Preserve memarg alignment in IR for atomic load/store/RMW/cmpxchg/wait/notify
(including field delegations) so non-natural alignment fails validation.
Fixes WebAssembly#8962

Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
@ANAMASGARD
ANAMASGARD requested a review from a team as a code owner August 4, 2026 08:22
@ANAMASGARD
ANAMASGARD requested review from kripken and removed request for a team August 4, 2026 08:22
@stevenfontanella

Copy link
Copy Markdown
Member

Thanks for the PR! Will take a look in the next few days

@stevenfontanella

Copy link
Copy Markdown
Member

Thanks for the PR!

One high-level comment on the approach: this PR stores a redundant align field in the AST for each AtomicRMW, AtomicCmpxchg, and atomic wait/notify expression. e.g. for a 32-bit atomic RMW, the align is always 4 and anything else would be invalid, so there's really no need to store this except for the validation check. For load and store on the other hand, it does need to be stored because the non-atomic versions of these could have any alignment and we need to preserve that in the binary.

So let's change the approach and make this a parse-time failure instead. This is different from what the spec says but in general we don't distinguish the two in Binaryen (the spec test runner allows either type of failure for assert_invalid and assert_malformed). This way we don't need to store the alignment in the AST for these expressions.

@stevenfontanella

Copy link
Copy Markdown
Member

Looks like there are some existing tests that don't use a natural alignment e.g. here and here. We can change them to load from address 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Non-natural alignment should fail to validate for atomic instructions

2 participants