Skip to content

mitigate MSVC alignment issue on x86-32 #139261

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

Merged
merged 2 commits into from
Apr 24, 2025

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 2, 2025

This implements mitigation for #112480 by stopping to emit align attributes on loads and function arguments when building for a win32 MSVC target. MSVC is known to not properly align u64 and similar types, and claiming to LLVM that everything is properly aligned increases the chance that this will cause problems.

Of course, the misalignment is still a bug, but we can't fix that bug, only MSVC can.

Also add an errata note to the platform support page warning users about this known problem.

try-job: i686-msvc*

@rustbot
Copy link
Collaborator

rustbot commented Apr 2, 2025

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added 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 Apr 2, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 2, 2025

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

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

pub fn has_unreliable_alignment(&self) -> bool {
// FIXME(#112480) MSVC on x86-32 is unsound and fails to properly align many types with
// more-than-4-byte-alignment on the stack.
if self.is_like_msvc && self.arch == "x86" {
Copy link
Member Author

Choose a reason for hiding this comment

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

This function is called a lot during codegen. Should I be worried about the (short) string comparison here?

We can also add a new bool flag to the target spec that directly controls this.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, thinking about this some more it might make sense to make it a part of the target spec so that custom targets can be supported.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently we auto-detect this for custom targets, if we make it part of the spec custom target specs will have to remember to do this.

Copy link
Member

Choose a reason for hiding this comment

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

Right but I mean the trade-off is that a potential non-msvc target that's not in-tree will have no way to set this. I don't know how likely that is but you mentioned the possibility earlier. And maybe it's fine in any case if such a targets are likely to have other custom requirements that will necessitate forking.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 2, 2025

This will probably be hellish for the codegen test suite since all the align things are missing now...
@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 2, 2025
mitigate MSVC alignment issue on x86-32

This implements mitigation for rust-lang#112480 by stopping to emit `align` attributes on loads and function arguments when building for a win32 MSVC target. MSVC is known to not properly align `u64` and similar types, and claiming to LLVM that everything is properly aligned increases the chance that this will cause problems.

Of course, the misalignment is still a bug, but we can't fix that bug, only MSVC can.

Also add an errata note to the platform support page warning users about this known problem.

try-job: `i686-msvc*`
@bors
Copy link
Collaborator

bors commented Apr 2, 2025

⌛ Trying commit ce44d17 with merge 21a86ec...

@RalfJung RalfJung force-pushed the msvc-align-mitigation branch from ce44d17 to 66f7993 Compare April 2, 2025 15:44
@beetrees
Copy link
Contributor

beetrees commented Apr 2, 2025

Looking at #112480 (comment), I think the only alignment this PR needs to change as it that a type with an alignment of 8 bytes might only have an alignment of 4 bytes. MSVC will always align primitive types with an alignment < 8 bytes correctly, and the only types with an alignment > 8 bytes are SIMD types which MSVC aligns correctly anyway.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 2, 2025

Looking at #112480 (comment), I think the only alignment this PR needs to change as it that a type with an alignment of 8 bytes might only have an alignment of 4 bytes. MSVC will always align primitive types with an alignment < 8 bytes correctly, and the only types with an alignment > 8 bytes are SIMD types which MSVC aligns correctly anyway.

I'd rather not rely on the "> 8" part. But yeah I think changing the logic to "clamp to 4" rather than "skip applying alignment" will probably be better, also to affect fewer tests.

@RalfJung RalfJung force-pushed the msvc-align-mitigation branch from 66f7993 to ca29777 Compare April 3, 2025 15:48
@RalfJung
Copy link
Member Author

RalfJung commented Apr 3, 2025

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 3, 2025
mitigate MSVC alignment issue on x86-32

This implements mitigation for rust-lang#112480 by stopping to emit `align` attributes on loads and function arguments when building for a win32 MSVC target. MSVC is known to not properly align `u64` and similar types, and claiming to LLVM that everything is properly aligned increases the chance that this will cause problems.

Of course, the misalignment is still a bug, but we can't fix that bug, only MSVC can.

Also add an errata note to the platform support page warning users about this known problem.

try-job: `i686-msvc*`
@bors
Copy link
Collaborator

bors commented Apr 3, 2025

⌛ Trying commit ca29777 with merge 0b5c0be...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Apr 3, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2025
@RalfJung RalfJung force-pushed the msvc-align-mitigation branch from ca29777 to d4ce381 Compare April 4, 2025 13:14
@RalfJung
Copy link
Member Author

RalfJung commented Apr 4, 2025

@saethlin I adjusted the alignment check so that it is still active on MSVC but only checks an alignment of up to 4. I hope I did this right. :D What would be the best way to test this? An only-msvc MIR test?

@RalfJung RalfJung force-pushed the msvc-align-mitigation branch from 178790f to df69e5d Compare April 4, 2025 16:07
@RalfJung
Copy link
Member Author

RalfJung commented Apr 4, 2025

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 4, 2025
mitigate MSVC alignment issue on x86-32

This implements mitigation for rust-lang#112480 by stopping to emit `align` attributes on loads and function arguments when building for a win32 MSVC target. MSVC is known to not properly align `u64` and similar types, and claiming to LLVM that everything is properly aligned increases the chance that this will cause problems.

Of course, the misalignment is still a bug, but we can't fix that bug, only MSVC can.

Also add an errata note to the platform support page warning users about this known problem.

try-job: `i686-msvc*`
@bors
Copy link
Collaborator

bors commented Apr 4, 2025

⌛ Trying commit df69e5d with merge ae95269...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Apr 7, 2025

💔 Test failed - checks-actions

@RalfJung RalfJung force-pushed the msvc-align-mitigation branch from d9b828f to e702f96 Compare April 7, 2025 21:31
@RalfJung
Copy link
Member Author

RalfJung commented Apr 7, 2025

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 7, 2025
mitigate MSVC alignment issue on x86-32

This implements mitigation for rust-lang#112480 by stopping to emit `align` attributes on loads and function arguments when building for a win32 MSVC target. MSVC is known to not properly align `u64` and similar types, and claiming to LLVM that everything is properly aligned increases the chance that this will cause problems.

Of course, the misalignment is still a bug, but we can't fix that bug, only MSVC can.

Also add an errata note to the platform support page warning users about this known problem.

try-job: `i686-msvc*`
@bors
Copy link
Collaborator

bors commented Apr 7, 2025

⌛ Trying commit e702f96 with merge 27ef8fe...

@bors
Copy link
Collaborator

bors commented Apr 8, 2025

☀️ Try build successful - checks-actions
Build commit: 27ef8fe (27ef8fe238fd8ed3348e55d4d58f946056e636d8)

@RalfJung
Copy link
Member Author

RalfJung commented Apr 8, 2025

All right this should be @rustbot ready now.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 8, 2025
@RalfJung
Copy link
Member Author

@oli-obk friendly reviewer ping :)

@oli-obk
Copy link
Contributor

oli-obk commented Apr 23, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 23, 2025

📌 Commit e702f96 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Apr 24, 2025
…oli-obk

mitigate MSVC alignment issue on x86-32

This implements mitigation for rust-lang#112480 by stopping to emit `align` attributes on loads and function arguments when building for a win32 MSVC target. MSVC is known to not properly align `u64` and similar types, and claiming to LLVM that everything is properly aligned increases the chance that this will cause problems.

Of course, the misalignment is still a bug, but we can't fix that bug, only MSVC can.

Also add an errata note to the platform support page warning users about this known problem.

try-job: `i686-msvc*`
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 24, 2025
Rollup of 23 pull requests

Successful merges:

 - rust-lang#139261 (mitigate MSVC alignment issue on x86-32)
 - rust-lang#139307 (std: Add performance warnings to HashMap::get_disjoint_mut)
 - rust-lang#139700 (Autodiff flags)
 - rust-lang#139752 (set subsections_via_symbols for ld64 helper sections)
 - rust-lang#139809 (Don't warn about `v128` in wasm ABI transition)
 - rust-lang#139852 (StableMIR: Implement `CompilerInterface`)
 - rust-lang#139945 (Extend HIR to track the source and syntax of a lifetime)
 - rust-lang#140028 (`deref_patterns`: support string and byte string literals in explicit `deref!("...")` patterns)
 - rust-lang#140139 (rustc_target: Adjust RISC-V feature implication)
 - rust-lang#140143 (Move `sys::pal::os::Env` into `sys::env`)
 - rust-lang#140148 (CI: use aws codebuild for job dist-arm-linux)
 - rust-lang#140150 (fix MAX_EXP and MIN_EXP docs)
 - rust-lang#140172 (Make algebraic functions into `const fn` items.)
 - rust-lang#140177 ([compiletest] Parallelize test discovery)
 - rust-lang#140181 (Remove `synstructure::Structure::underscore_const` calls.)
 - rust-lang#140184 (Update doc of cygwin target)
 - rust-lang#140186 (Rename `compute_x` methods)
 - rust-lang#140187 ([AIX] Handle AIX dynamic library extensions within c-link-to-rust-dylib run-make test)
 - rust-lang#140191 (Remove git repository from git config)
 - rust-lang#140194 (minicore: Have `//@ add-core-stubs` also imply `-Cforce-unwind-tables=yes`)
 - rust-lang#140195 (triagebot: label minicore changes w/ `A-test-infra-minicore` and ping jieyouxu on changes)
 - rust-lang#140202 (Make #![feature(let_chains)] bootstrap conditional in compiler/)
 - rust-lang#140214 (Remove comment about handling non-global where bounds with corresponding projection)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 24, 2025
…oli-obk

mitigate MSVC alignment issue on x86-32

This implements mitigation for rust-lang#112480 by stopping to emit `align` attributes on loads and function arguments when building for a win32 MSVC target. MSVC is known to not properly align `u64` and similar types, and claiming to LLVM that everything is properly aligned increases the chance that this will cause problems.

Of course, the misalignment is still a bug, but we can't fix that bug, only MSVC can.

Also add an errata note to the platform support page warning users about this known problem.

try-job: `i686-msvc*`
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 24, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#139261 (mitigate MSVC alignment issue on x86-32)
 - rust-lang#140075 (Mention average in midpoint documentations)
 - rust-lang#140184 (Update doc of cygwin target)
 - rust-lang#140186 (Rename `compute_x` methods)
 - rust-lang#140194 (minicore: Have `//@ add-core-stubs` also imply `-Cforce-unwind-tables=yes`)
 - rust-lang#140195 (triagebot: label minicore changes w/ `A-test-infra-minicore` and ping jieyouxu on changes)
 - rust-lang#140214 (Remove comment about handling non-global where bounds with corresponding projection)
 - rust-lang#140228 (Revert overzealous parse recovery for single colons in paths)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a8ebfb2 into rust-lang:master Apr 24, 2025
7 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 24, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 24, 2025
Rollup merge of rust-lang#139261 - RalfJung:msvc-align-mitigation, r=oli-obk

mitigate MSVC alignment issue on x86-32

This implements mitigation for rust-lang#112480 by stopping to emit `align` attributes on loads and function arguments when building for a win32 MSVC target. MSVC is known to not properly align `u64` and similar types, and claiming to LLVM that everything is properly aligned increases the chance that this will cause problems.

Of course, the misalignment is still a bug, but we can't fix that bug, only MSVC can.

Also add an errata note to the platform support page warning users about this known problem.

try-job: `i686-msvc*`
@RalfJung RalfJung deleted the msvc-align-mitigation branch April 24, 2025 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

8 participants