Skip to content

Cranelift: properly reject unimplemented big-endian loads/stores. #10863

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented May 29, 2025

At some point during the development of the Cranelift backend infrastructure, to properly support big-endian architectures such as s390x, we added explicit endianness flags to MemFlags, which are given to all memory operations (e.g., loads, stores, and atomic ops). In s390x in particular, the backend carefully observes these flags, because a prominent use of Cranelift (as a Wasm backend) requires explicit little-endian behavior and the system is big-endian. However, all of our other supported ISAs are little-endian and so we did not implement explicit checks at the time, instead accepting all loads and stores as an artifact of our little-endian-only origins.

This PR adds explicit conditions to all ISLE rules that lower loads, stores, and atomic ops on x86-64, aarch64, and riscv64 to accept little or "native" (also little) endian operations only. Compilation of a big-endian operation will now result in a compilation error because no ISLE rule will match (no lowering exists). At some later point we could add these lowerings, but for now we at least do not miscompile.

Fixes #10861.

At some point during the development of the Cranelift backend
infrastructure, to properly support big-endian architectures such as
s390x, we added explicit endianness flags to `MemFlags`, which are
given to all memory operations (e.g., loads, stores, and atomic
ops). In s390x in particular, the backend carefully observes these
flags, because a prominent use of Cranelift (as a Wasm backend)
requires explicit little-endian behavior and the system is
big-endian. However, all of our other supported ISAs are little-endian
and so we did not implement explicit checks at the time, instead
accepting all loads and stores as an artifact of our
little-endian-only origins.

This PR adds explicit conditions to all ISLE rules that lower loads,
stores, and atomic ops on x86-64, aarch64, and riscv64 to accept
little or "native" (also little) endian operations only. Compilation
of a big-endian operation will now result in a compilation error
because no ISLE rule will match (no lowering exists). At some later
point we could add these lowerings, but for now we at least do not
miscompile.

Fixes bytecodealliance#10861.
@cfallin cfallin requested a review from a team as a code owner May 29, 2025 17:07
@cfallin cfallin requested review from fitzgen and removed request for a team May 29, 2025 17:07
@cfallin
Copy link
Member Author

cfallin commented May 29, 2025

(Note that the Pulley backend rules appear to have explicit consideration for endianness, so I skipped that backend here.)

@cfallin
Copy link
Member Author

cfallin commented May 29, 2025

Design note: I could have added checks to more central places in all three backends instead -- namely, Amode construction -- but opted instead to annotate every rule with this predicate, because the information logically belongs to the individual lowerings and if we want to add actual big-endian lowerings in the future, we'll want Amode to work for those too.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen labels May 29, 2025
@fitzgen fitzgen added this pull request to the merge queue May 30, 2025
Merged via the queue into bytecodealliance:main with commit 84477fc May 30, 2025
65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cranelift: Respect or error on big-endian flag for loads/stores on little-endian targets
2 participants