Skip to content

rustc_const_eval: respect target.min_global_align #142198

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

folkertdev
Copy link
Contributor

This PR builds on #142179 (which will be merged soon), so ignore the first commit here.

Issue #44411 describes a correctness problem on s390x, where a static is not sufficiently aligned: the larl instruction that LLVM generates assumes an even address, but e.g. a static containing a bool may have an odd address.

Because miri is an interpreter, it is not actually behaving incorrectly in this case. Currently, the only LLVM target that also specifies a min_global_align is larai, which rust does not support. The nvptx targets inherit the alignment from their host, but nvptx + s390x seems unlikely. Nevertheless, miri should probably respect the target's min_global_align.

The solution here is really hard to test: it's kind of a numbers game. The test I added at least fails on current miri (and should now succeed on CI). We'll likely soon have #[align] on statics (rust-lang/rfcs#3806 is in FCP), so then we can have some more extensive static alignment checks. I'm assuming the tests are executed for a variety of targets, including s390x?

See also #miri > alignment of globals on s390x

r? @RalfJung

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 8, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 8, 2025

The Miri subtree was changed

cc @rust-lang/miri

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@folkertdev folkertdev force-pushed the miri-s390x-align-statics branch from c87f1ba to 21aae4d Compare June 8, 2025 17:17
@@ -0,0 +1,50 @@
// Test that miri respects the `target.min_global_align` value for the target.
// E.g. on s390x, statics have, in practice, a minimum alignment of 2.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"in practice" seems like unnecessarily wobbly language here. Can we say something more concrete, or if the answer is complicated point to where the full answer is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the actual answer is "LLVM and some other compilers generate incorrect code if globals have an alignment less than 2, as they generate accesses to globals using LALR, which requires alignment to even addresses in order to work".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the situation here is a confusing

  • technically, miri does not need to enforce this alignment of 2, because it does not actually emit the larl instruction that would behave incorrectly
  • but in practice, both gcc, clang and rustc do respect this alignment
  • intuitively miri should respect target.min_global_align
  • the only way to observe its effect right now is to test the alignment of statics on s390x

Anyway I've updated the comment with why this test was written.

@folkertdev folkertdev force-pushed the miri-s390x-align-statics branch from 21aae4d to 7b8bcdd Compare June 8, 2025 17:39
@folkertdev folkertdev force-pushed the miri-s390x-align-statics branch from 7b8bcdd to 553ae22 Compare June 8, 2025 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants