MSVC on x86-32 Windows fails to align variables to their required alignment

[glandium]

This is a regression from #98112. I suppose it's not possible to disable this specific check only while preserving debug assertions...

The core problem is that the x86 ABI on Windows doesn't guarantee the stack alignment above 4. See for example https://developercommunity.visualstudio.com/t/vs2017-64-bit-int-alignment-problem/294259

And while some types have an alignment reported of 8 (e.g. UINT64), in practice, the C compiler will happily not align them on the stack.

So for example, this C code, compiled by MSVC for 32-bits:

#include <cinttypes>
extern void hoge(uint64_t*);
void foo() {
    uint64_t a;
    hoge(&a);
}

will produce this assembly:

_a$ = -8                                                ; size = 8
void foo(void) PROC                                        ; foo
        push    ebp
        mov     ebp, esp
        sub     esp, 8
        lea     eax, DWORD PTR _a$[ebp]
        push    eax
        call    void hoge(unsigned __int64 *)                        ; hoge
        add     esp, 4
        mov     esp, ebp
        pop     ebp
        ret     0
void foo(void) ENDP

(on godbolt)

If the stack pointer is not 8-bytes aligned when entering the function, the pointer passed to hoge is not going to be 8-bytes aligned.

As mentioned in the linked community post above, adding alignas(8) to the type definition makes the compiler align the stack:

#include <cinttypes>
extern void hoge(uint64_t*);
void foo() {
    alignas(8) uint64_t a;
    hoge(&a);
}

becomes

_a$ = -8                                                ; size = 8
void foo(void) PROC                                        ; foo
        push    ebx
        mov     ebx, esp
        sub     esp, 8
        and     esp, -8                             ; fffffff8H
        add     esp, 4
        push    ebp
        mov     ebp, DWORD PTR [ebx+4]
        mov     DWORD PTR [esp+4], ebp
        mov     ebp, esp
        sub     esp, 8
        lea     eax, DWORD PTR _a$[ebp]
        push    eax
        call    void hoge(unsigned __int64 *)                        ; hoge
        add     esp, 4
        mov     esp, ebp
        pop     ebp
        mov     esp, ebx
        pop     ebx
        ret     0
void foo(void) ENDP

(on godbolt)

Now, what this means is that if that hoge function is a rust FFI function, and it uses that pointer, the "misaligned pointer dereference" check is hit and panic ensues.

Real life case, for the curious:
https://github.com/servo/dwrote-rs/blob/master/src/font_file_loader_impl.rs#L116-L123

That function is called from dwrite.dll (which comes with Windows).

t-opsem FCP comment

Summary of the MSVC alignment rules

[glandium]

And while some types have an alignment reported of 8 (e.g. UINT64)

Let me substantiate this:

#include <cinttypes>
int foo() {
    return alignof(uint64_t);
}

compiles to:

int foo(void) PROC                                        ; foo
        push    ebp
        mov     ebp, esp
        mov     eax, 8
        pop     ebp
        ret     0
int foo(void) ENDP

(on godbolt)

If the stack pointer is not 8-bytes aligned when entering the function, the pointer passed to hoge is not going to be 8-bytes aligned.

Actually, if the stack pointer is 8-bytes aligned when entering the function, the pointer passed to hoge is going to be unaligned (because there's a push first).

[saethlin]

I suppose it's not possible to disable this specific check only while preserving debug assertions...

If you can pass unstable flags, currently, you can say -Zmir-enable-passes=-CheckAlignment.

[saethlin]

I think your report here is a duplicate of https://developercommunity.visualstudio.com/t/visual-c-alignof-is-not-conformant-on-x86/1258506

That is, this appears to be a known MSVC bug.

[glandium]

The MSVC bug would be the value returned by alignof. That doesn't change anything for the alignment check in rust.

[saethlin]

std::mem::align_of::<u64>() returns 8 on i686-pc-windows-msvc. Are you saying that is a bug?

[ChrisDenton]

It's complicated. See https://learn.microsoft.com/en-us/cpp/c-language/alignment-c?view=msvc-170

The compiler generally aligns data on natural boundaries that are based on the target processor and the size of the data. Data is aligned on up to 4-byte boundaries on 32-bit processors, and 8-byte boundaries on 64-bit processors.

So a type with a preferred alignment of 8 bytes can be 4-byte aligned on 32-bit targets.

[saethlin]

Ah, but std::mem::align_of does not return the preferred alignment.

Returns the ABI-required minimum alignment of a type in bytes.

Every reference to a value of the type T must be a multiple of this number.

This is the alignment used for struct fields. It may be smaller than the preferred alignment.

[ChrisDenton]

But see also https://learn.microsoft.com/en-us/cpp/build/reference/zp-struct-member-alignment?view=msvc-170

The compiler stores members after the first one on a boundary that's the smaller of either the size of the member type, or an N-byte boundary.

Where N is 8 for x86.

[glandium]

It's true that from the perspective that in struct { int a; uint64_t b; }, b is 8-bytes aligned, alignof returning 8 is actually correct. But it's more indicative of "you'll be safe if you align to 8 bytes" than "you must align to 8 bytes" or "it will always be aligned to 8 bytes". Because the struct itself, on the stack, will still be 4-bytes aligned. The x86 windows ABI is weird that way... but that essentially breaks the assumption from the alignment check.

[ChrisDenton]

Right. But it matters in terms of how this is addressed. It would be wrong to change align_of as that's documented to be for struct fields.

For the short term, skipping this check on i686-windows might be the only decent options (which is unfortunate). But longer term it would be better to have a proper fix. I'm not really what that would look like tbh. As you say, it's breaking a core assumption.

[saethlin]

The x86 windows ABI is weird that way... but that essentially breaks the assumption from the alignment check.

There is no assumption made by the alignment check. We unambiguously tell LLVM that a dereference of a *const u64 on i686-pc-windows-msvc is 8-byte-aligned: https://godbolt.org/z/MrdTPqWbY

pub unsafe extern "C" fn read_u64(ptr: *const u64) -> u64 {
    *ptr
}

define i64 @_ZN7example8read_u6417hbc6f74c29a983155E(ptr %ptr) unnamed_addr #0 !dbg !5 {
  %0 = load i64, ptr %ptr, align 8, !dbg !10, !noundef !9
  ret i64 %0, !dbg !11
}

If you are saying that this alignment assumption is wrong, then we have a soundness issue. The bug is not in the alignment check, we either have invalid codegen from MSVC because it does not sufficiently align its uint64_ts, or we have invalid codegen from rustc because it assumes u64 is 8-byte-aligned on this target when it is not.

[Mark-Simulacrum]

cc #103830 #80127, which I think may also be related in general

[jyn514]

Looks like #112157 is the latest incarnation of #103830.

[erikdesjardins]

#103830 / #112157 is basically the same issue but for byval parameters. The unsoundness is immediately visible there because if you assume a byval argument will be 8 byte aligned you'll load from rsp+8 instead of rsp+4, so Clang (and now rustc) implement a copy of MSVC's alignment logic. But that is not as bad from a language semantics perspective, because byval pointers are not exposed to user code.

Whereas here, for loads of pointer parameters, it looks like we've been generating code that's unsound when linked to MSVC, but it happened to work in most cases.

Here's a comparison of what rustc, Clang, and MSVC generate: https://godbolt.org/z/56h6Md1rd
Clang and rustc both generate the same LLVM IR that assumes 8 byte alignment, and LLVM generates code to align the stack as necessary.

This means we can't reduce our alignment to 4 in all cases, because then we wouldn't have sufficient alignment for FFI calls to Clang-compiled code. Unless we patch Clang at the same time, we'd have to use the higher alignment for our stack/heap allocations, but then use align 4 for loads/stores/etc.

[deltragon]

Wouldn't @Gankra's abi-cafe project be helpful here?

[apiraino]

Issue was discussed during compiler team meeting on Zulip (notes).

The actionable for now will be to add a toggle to disable the check introduced in #98112 and backport to the next beta. Next, T-opsem / T-lang will look into it and will define a long-term strategy.

@rustbot label -I-compiler-nominated

[RalfJung]

According to llvm/llvm-project#34563 (comment), this bug does not imply an ABI incompatibility... is that something clang somehow achieves? Does rustc need to do anything to get the same ABI?

[nbdd0121]

I think that simply means we tell LLVM is i64 is 4-byte aligned but still have the Rust i64 8-byte aligned.

[Nemo157]

IIUC #112157 fixes the ABI issues, marking i64 as align-4 when passed by-value.

[erikdesjardins]

#112157 fixes the ABI incompatibility where by-value arguments passed on the stack get incorrect stack offsets (meaning we read/write incorrect memory). It does not fix the ABI incompatibility with by-ref/pointer arguments getting incorrect align attributes.

According to llvm/llvm-project#34563 (comment), this bug does not imply an ABI incompatibility...

I assume it's been edited since then, but the first word of that comment is "most", so I don't think it's saying that all ABI incompatibilities have been fixed... :P

By "parameter passing isn't affected by the ABI alignment here" they mean the stack offsets of parameters, i.e. the problem fixed by #112157 (which was already implemented in Clang). That problem is easier to fix since byval argument stack slots aren't directly exposed to user code, so they can be underaligned without breaking language semantics.

[RalfJung]

I assume it's been edited since then, but the first word of that comment is "most", so I don't think it's saying that all ABI incompatibilities have been fixed... :P

Right, I didn't consider these align annotations to be (function call) ABI issues.

I hadn't seen #112157, good to know we have that work-around like clang does.

[apiraino]

Further discussion from T-compiler on weekly triage meeting (notes). Team will follow up for more discussion

[apiraino]

More discussion in the T-compiler triage meeting (on Zulip), this time floating the idea of a downgrade of the platform in the tier support. Or at least document the peculiarities/quirks of this specific target in our tier support list.

[eggyal]

According to llvm/llvm-project#34563 (comment), this bug does not imply an ABI incompatibility... is that something clang somehow achieves? Does rustc need to do anything to get the same ABI?

rust-lang/rfcs#3594 proposes to expose an attribute akin to force_align_arg_pointer in C compilers, which AIUI clang lowers to the LLVM IR attribute stackrealign that in turn injects into the annotated function's preamble a runtime realignment of its stack to 16(?) bytes.

[jendrikw]

Related discussion at the GCC bug tracker https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69560

[RalfJung]

That's not quite the same. There GCC chose to give a higher alignment to uint64_t on the stack than they have in struct fields. They are free to do so as long as they don't indicate that all uint64_t are aligned -- the bug was that they did so, but it was fixed.

This here is the opposite situation: we have a type that must be 8-aligned (and is 8-aligned in struct fields!), but MSVC decides to make it only 4-aligned when it occurs on the stack. That's just fundamentally broken and there is no sane way to deal with it. We would need a fundamentally new concept, separating a type's alignment for stack layout purposes from its minimal alignment requirement. We decided not to do that; all that remains is doing damage control for this fundamentally broken target.

rust-lang/rfcs#3594 proposes to expose an attribute akin to force_align_arg_pointer in C compilers, which AIUI clang lowers to the LLVM IR attribute stackrealign that in turn injects into the annotated function's preamble a runtime realignment of its stack to 16(?) bytes.

That doesn't help as there will still be code compiled with MSVC that creates pointers that get passed to Rust code.

I think what we should do is

• When generating code for this broken target, never emit align annotations to LLVM that are bigger than 4
• Find a suitable place to document this as a "target erratum"

Then this issue can be closed.

We can even re-enable @saethlin's UB checks for alignment with the same cap of 4, to ensure coherence of the LLVM IR we emit and the UB we check. But Rust crates may still make assumptions based on align_of and we're not going to ask the entire ecosystem to deal with the misaligned pointers produced by MSVC.

[RalfJung]

@CAD97 @ChrisDenton I realized I am actually unclear about one aspect of this issue: what exactly is the set of types where MSVC reports a higher alignment than what it will use when putting such a variable on the stack? We know uint64_t is on that list, but e.g. does it also affect structs containing uint64_t types? Does it affect structs explicitly annotation with the C equivalent of #[repr(align)]? (I am always assuming the variable declaration itself is not annotated, only the type may have annotations)

[CAD97]

Warning

The experimental results referenced below are only my recollection of the behavior I got originally. This description is accurate both to that recollection and the documentation, but I or someone else should triple-check it before trusting it. I hope to do so sometime this week once I have a chance.

@RalfJung: Based on previous experimental results and some fresh crawling of Microsoft Learn, MSVC alignment works basically as so:

• The compiler invocation specifies /Zp (Struct Member Alignment) to one of 1, 2, 4, 8, or 16.
  • If left unspecified, the default is
    • 8 for x86, ARM, and ARM64.
    • 16 for x64 and ARM64EC.
  • “Don't use this option unless you have specific alignment requirements.”
  • #pragma pack manipulates the same struct member alignment setting with a stateful stack.
• Padding is added between struct members to align them to their size or the current struct member alignment, whichever is smaller.
  • For compound types, padding is applied to the greatest alignment of the struct’s members.
• On the stack, data members are aligned to their size or the target's “natural” alignment boundary, whichever is smaller, which is 4 for 32-bit targets and 8 for 64-bit targets.
• Global override (yes, including overriding any #pragma pack): use of _Alignas:
  • Declaring a type with _Alignas(x) causes any member declaration with that type to be _Alignas(x).
  • Declaring a struct member with _Alignas(x) aligns that member. The containing type is implicitly declared with _Alignas(x).
  • Declaring a stack member with _Alignas(x) will guarantee that the member is aligned as requested by x.
  • Using _Alignas to specify a lower alignment than would be provided without the _Alignas specifier is ill-formed.
• _Alignof reports the alignment used for the type as a struct member. _Alignas(tag-name) is equivalent to _Alignas(_Alignof(tag-name)).

This indicates that 128-bit primitive data types will have the same alignment issue on x86_64 that 64-bit primitive data types do on x86. Is the abi-cafe checking simd vector type ABI/layout?

[RalfJung]

What does this mean for struct S { uint64_t field; } on the stack? How is it aligned?

For compound types, padding is applied to the greatest alignment of the struct’s members.

There doesn't seem to be a notion of a type having alignment, only a size and the global alignment flag, so I don't know what this part means.

[beetrees]

struct S { uint64_t field; }, double and struct S { double field; } are aligned by x86 MVSC the same as uint64_t (that is: alignof(T) == 8, but no stack realignment is done unless alignas is used, either in the struct or on the variable declaration itself). In comparison, SIMD types like __m64 and __m128 appear to always cause the stack to be realigned.

[ChrisDenton]

does it also affect structs containing uint64_t types?

Yes.

Does it affect structs explicitly annotation with the C equivalent of #[repr(align)]?

No.

You can see this on godbolt. The and sub pattern is where it's aligning the stack which, as the OP says, is necessary on x86 when alignment is >4.

Also note that SIMD types are special and will be properly aligned.

[RalfJung]

What about structs containing SIMD types?

[ChrisDenton]

That is aligned. I updated the godbolt link.

[RalfJung]

Okay so it's basically entirely behaving as-if u64 and u128 had alignment 4, except that their offset in a struct will be divisible by 8 (also recursively):

Each type has a "required alignment" and a "layout alignment". The former is used to determine which alignment accesses must have (or else UB) and how much variables on the stack will be aligned, the latter is used for struct layout. The required alignment of a struct is the largest required alignment of its fields; the layout alignment of a struct is the largest layout alignment of its fields. Most types have the same value for both kinds of alignment (including the SIMD types), except integer/float primitives larger than 4 have required alignment 4, but layout alignment matching their size.

[RalfJung]

This indicates that 128-bit primitive data types will have the same alignment issue on x86_64 that 64-bit primitive data types do on x86. Is the abi-cafe checking simd vector type ABI/layout?

I tried testing this but int128_t is not a valid type...

[ChrisDenton]

Right, only SIMD has 128-bit integers for MSVC. There isn't an issue for 64-bit because all "natural" types are <= 8 byte aligned (except for SIMD types which are special). Here's a list of all the built-in types I know of (minus some aliases):

Type	sizeof	alignof	stack align (32-bit)
__int8, unsigned __int8, char, bool	1	1	1
__int16, unsigned __int16, short, unsigned short, wchar_t	2	2	2
__int32, unsigned __int32, int, unsigned int, long, unsigned long	4	4	4
float	4	4	4
double	8	8	4
__int64, unsigned __int64, long long, unsigned long long	8	8	4
__ptr32	4	4	4
__ptr64	8	8	4
__m64	8	8	8
__m128, __m128d, __m128i	16	16	16

So all stack alignments for basic types on x86 are capped at 4. There are two exceptions I know of:

1. SIMD types (e.g. __m64, __m128, etc)
2. The alignas attribute will override the cap.

So oddly enough these two structs have different stack alignments but will otherwise be identical:

struct A {
    bool b;
    int64_t x;
};
struct B {
    bool b;
    alignas(int64_t) int64_t x;
};