Skip to content

Windows 7 i686 does not reliably align 16 byte aligned thread locals #138903

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

Closed
drewkett opened this issue Mar 24, 2025 · 2 comments · Fixed by #140007
Closed

Windows 7 i686 does not reliably align 16 byte aligned thread locals #138903

drewkett opened this issue Mar 24, 2025 · 2 comments · Fixed by #140007
Labels
A-align Area: alignment control (`repr(align(N))` and so on) A-thread-locals Area: Thread local storage (TLS) C-bug Category: This is a bug. O-windows-7 OS: Windows 7 or Windows Server 2008 R2 or etc. O-x86_32 Target: x86 processors, 32 bit (like i686-*) (IA-32) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@drewkett
Copy link

drewkett commented Mar 24, 2025

The following program will reliably crash using the i686-win7-windows-msvc target on a Windows 7 installation using latest nightly.

I tried this code:

#[repr(align(16))]
struct Align16(u8);

thread_local! {
    static ALIGN16: Align16 = Align16(0);
}

fn main() {
    let mut threads = Vec::new();
    for _ in 0..10 {
        threads.push(std::thread::spawn(|| {
            ALIGN16.with(|r| println!("ALIGN16 {:p}", r as *const _));
        }));
    }
    for thread in threads {
        thread.join().unwrap();
    }
}

This reliably produces the following panic.

unsafe precondition(s) violated: ptr::replace requires that the pointer argument is aligned and non-null

This indicates a bug in the program. This Undefined Behavior check is optional, and cannot be relied on for safety.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused a non-unwinding panic. aborting.

My experience with/understanding of thread locals is mostly limited to what i've researched due to this issue, but it does appear that an i686 targeted executable on Windows 7 does not reliably align thread locals when 16 byte alignment is requested. I didn't see issues with even 8 byte alignment. I was not able to reproduce this issue on Windows 10.

Given that i686 win7 is a likely pretty niche target, do you think it would be reasonable to partially undo #123257 by disabling has_thread_local for i686-win7-windows-msvc. It seems that i686 on Windows 7 can't be trusted for alignment of thread locals. I did test adjusting the i686-win7-windows-msvc target in a local build of the compiler and it does appear to work for the test program.

The 16 byte aligned thread local came from matrixmultiply which actually works around misalignment internally, but compiled this way I get the UB check panic anyway. I can certainly work around it by seeing if matrixmultiply can be patched, or just doing that locally. But it seems like this should be fixed in the compiler ideally.

@drewkett drewkett added the C-bug Category: This is a bug. label Mar 24, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 24, 2025
@moxian
Copy link
Contributor

moxian commented Mar 24, 2025

@rustbot label: +O-windows-7 +O-x86_32 +A-thread-locals +A-align -needs-triage +T-compiler
cc @roblabla @ChrisDenton

@rustbot rustbot added A-align Area: alignment control (`repr(align(N))` and so on) A-thread-locals Area: Thread local storage (TLS) O-windows-7 OS: Windows 7 or Windows Server 2008 R2 or etc. O-x86_32 Target: x86 processors, 32 bit (like i686-*) (IA-32) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 24, 2025
drewkett added a commit to drewkett/matrixmultiply that referenced this issue Mar 25, 2025
Windows 7 does not respect 16 byte alignment for thread locals on i686
builds. Rust 1.79 changed i686 windows builds to use native thread local
support. As a result, using `matrixmultiply` on i686 win7 builds leads
to a UB check panic nounwind when run on Windows 7. See
<rust-lang/rust#138903> for more info.

This change adds `i686-win7-windows-msvc` as an excluded target for the
alignment attribute on `MaskBuffer`.
@roblabla
Copy link
Contributor

roblabla commented Apr 18, 2025

So I dug into this a bit, and was able to reproduce. Thankfully there's a bit of a similar issue in GNU targets, with some instructions on how to investigate: #135719 (comment)

llvm-objdump --all gives me the following information:

TLS directory:
  StartAddressOfRawData: 0x444000
  EndAddressOfRawData: 0x444064
  AddressOfIndex: 0x442a0c
  AddressOfCallBacks: 0x43e804
  SizeOfZeroFill: 0
  Characteristics: 5242880
  Alignment: 16

Characteristics 5242880 is 0x500000 in hex, which equals IMAGE_SCN_ALIGN_16BYTES, which matches what we expect. However, when running the reproducer, I indeed get the same crashes. So this would indicate that Windows 7 does not respect the IMAGE_SCN_ALIGN_16BYTES flag in the TLS directory.

I'll make a PR to put has_thread_local back to false.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 18, 2025
Disable has_thread_local on i686-win7-windows-msvc

On Windows 7 32-bit, the alignment characteristic of the TLS Directory don't appear to be respected by the PE Loader, leading to crashes. As a result, let's disable has_thread_local to make sure TLS goes through the emulation layer.

Fixes rust-lang#138903
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 18, 2025
Disable has_thread_local on i686-win7-windows-msvc

On Windows 7 32-bit, the alignment characteristic of the TLS Directory don't appear to be respected by the PE Loader, leading to crashes. As a result, let's disable has_thread_local to make sure TLS goes through the emulation layer.

Fixes rust-lang#138903
jhpratt added a commit to jhpratt/rust that referenced this issue Apr 19, 2025
Disable has_thread_local on i686-win7-windows-msvc

On Windows 7 32-bit, the alignment characteristic of the TLS Directory don't appear to be respected by the PE Loader, leading to crashes. As a result, let's disable has_thread_local to make sure TLS goes through the emulation layer.

Fixes rust-lang#138903
@bors bors closed this as completed in 67a97ba Apr 19, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 19, 2025
Rollup merge of rust-lang#140007 - roblabla:fix-win7, r=ChrisDenton

Disable has_thread_local on i686-win7-windows-msvc

On Windows 7 32-bit, the alignment characteristic of the TLS Directory don't appear to be respected by the PE Loader, leading to crashes. As a result, let's disable has_thread_local to make sure TLS goes through the emulation layer.

Fixes rust-lang#138903
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-align Area: alignment control (`repr(align(N))` and so on) A-thread-locals Area: Thread local storage (TLS) C-bug Category: This is a bug. O-windows-7 OS: Windows 7 or Windows Server 2008 R2 or etc. O-x86_32 Target: x86 processors, 32 bit (like i686-*) (IA-32) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants