Skip to content
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

<type_traits>: std::aligned_storage<8, 8> doesn't force stack realignment on MSVC x86 #1533

Open
rnk opened this issue Dec 15, 2020 · 4 comments
Labels
bug Something isn't working vNext Breaks binary compatibility

Comments

@rnk
Copy link

rnk commented Dec 15, 2020

If a user declares a local variable with the type std::aligned_union_t<1, double> or std::aligned_storage<8, 8>, the local variable may not in practice be 8-byte aligned. While accessing the memory will not cause crashes, it can lead to issues if the program assumes that the low bits of the variable's address are zero. LLVM does this often, and that is where I encountered the issue:
https://reviews.llvm.org/D92509#2452929

The MSVC x86 compiler only maintains 4 byte stack alignment. If a user declares a variable with 8 byte alignment, such as a double or __int64, it will not realign the stack. If the user explicitly requests 8 byte alignment with alignas or __declspec(align), then MSVC will realign the stack.

Arguably, this is a Visual C++ compiler bug: the compiler should realign the stack for doubles or any other type with 8 byte alignment. However, the STL could help the user out here. If the user is bothering to use a std::aligned* type, they probably care about the alignment more than any old double local variable, and the STL should go the extra mile to satisfy that alignment by applying the alignas attribute to the double specialization of the _Aligned template.

Consider the assembly generated for the test case on godbolt:
https://gcc.godbolt.org/z/TxEo7b

Command-line test case

C:\Temp>type repro.cpp
#include <type_traits>
struct Foo {
  std::aligned_union_t<1, double> data;
};
void escape(Foo*);
void declareAndEscape() {
#ifdef REALIGN
  alignas(double)
#endif
  Foo o;
  escape(&o);
}

C:\Temp>cl -c -GS- repro.cpp -Facl.asm
Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29334 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

repro.cpp

C:\Temp>type cl.asm
; Listing generated by Microsoft (R) Optimizing Compiler Version 19.28.29334.0

        TITLE   C:\src\llvm-project\build\repro.cpp
        .686P
        .XMM
        include listing.inc
        .model  flat

INCLUDELIB LIBCMT
INCLUDELIB OLDNAMES

PUBLIC  ?declareAndEscape@@YAXXZ                        ; declareAndEscape
EXTRN   ?escape@@YAXPAUFoo@@@Z:PROC                     ; escape
; Function compile flags: /Odtp
_TEXT   SEGMENT
_o$ = -8                                                ; size = 8
?declareAndEscape@@YAXXZ PROC                           ; declareAndEscape
; File C:\src\llvm-project\build\repro.cpp
; Line 6
        push    ebp
        mov     ebp, esp
        sub     esp, 8
; Line 11
        lea     eax, DWORD PTR _o$[ebp]
        push    eax
        call    ?escape@@YAXPAUFoo@@@Z                  ; escape
        add     esp, 4
; Line 12
        mov     esp, ebp
        pop     ebp
        ret     0
?declareAndEscape@@YAXXZ ENDP                           ; declareAndEscape
_TEXT   ENDS
END

Expected behavior

Note the lack of AND instructions to realign ESP. Recompile with -DREALIGN to see the desired stack realignment prologue.

STL version
19.28

rnk added a commit to llvm/llvm-project that referenced this issue Dec 15, 2020
…nion_t, NFC"

We determined that the MSVC implementation of std::aligned* isn't suited
to our needs. It doesn't support 16 byte alignment or higher, and it
doesn't really guarantee 8 byte alignment. See
microsoft/STL#1533

Also reverts "ADT: Change AlignedCharArrayUnion to an alias of std::aligned_union_t, NFC"

Also reverts "ADT: Remove AlignedCharArrayUnion, NFC" to bring back
AlignedCharArrayUnion.

This reverts commit 4d8bf87.

This reverts commit d10f986.

This reverts commit 4b5dc15.
@StephanTLavavej StephanTLavavej added bug Something isn't working vNext Breaks binary compatibility labels Dec 15, 2020
@StephanTLavavej
Copy link
Member

Yep, this is a bug. I believe that fixing it will break bincompat, so I'm labeling this as vNext.

The Microsoft-internal compiler bug VSO-402395 "[c2]alignas by-value parameters should be permitted on ARM" also blocks using alignas unconditionally.

@MikeGitb
Copy link

Does the compiler align plain unions correctly?

@rnk
Copy link
Author

rnk commented Dec 15, 2020

You're right, this will break binary compatibility, since it will cause std::aligned* objects (and objects containing them) to be passed indirectly in memory. For some reason, when I was writing the report, I thought to myself that fixing this would only affect the alignment of local variables, not the calling convention used to pass them, but that isn't correct.

Re @MikeGitb, no, I believe the compiler does not align double, or a union with a double member, to 8 bytes.

Perhaps it's best to refile this as a compiler bug. The compiler is able to realign the stack if it chooses to without breaking ABI compatibility. 8 byte aligned objects can still be passed misaligned to 4 bytes for compatibility. When receiving such an object as an argument, the compiler can copy it into aligned stack memory. Clang does this, FWIW. A compiler change would affect performance, and probably disturb any x86 inline assembly making assumptions about stack frame registers (EBP vs EBX).

I can't speak for all users, but from my PoV, x86 is a legacy target, so it's more surprising when edge cases like this pop up on x86. I would rather take the performance hit of the stack realignment and sleep better at night knowing that alignment works.

@CaseyCarter
Copy link
Member

The Microsoft-internal compiler bug VSO-402395 "[c2]alignas by-value parameters should be permitted on ARM" also blocks using alignas unconditionally.

How does lack of support for function parameter alignment greater than 16 on ARM mean that we can't use alignas to properly implement aligned_storage/aligned_union any more than does lack of support for function parameter alignment greater than 64 on ARM64/x86/x64?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working vNext Breaks binary compatibility
Projects
None yet
Development

No branches or pull requests

4 participants