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

Clang: ABI incompatibility with MSVC on i686-windows for __declspec(align(N)), with N <= 4 #63257

Closed
erikdesjardins opened this issue Jun 11, 2023 · 5 comments
Labels
ABI Application Binary Interface llvm:codegen

Comments

@erikdesjardins
Copy link
Member

For the following three struct definitions

struct NoRequestedAlign {
  uint64_t x;
  uint64_t y;
  uint64_t z;
};

__declspec(align(1))
struct RequestedAlign1 {
  uint64_t x;
  uint64_t y;
  uint64_t z;
};

__declspec(align(8))
struct RequestedAlign8 {
  uint64_t x;
  uint64_t y;
  uint64_t z;
};

when passed by value:

MSVC passes NoRequestedAlign and RequestedAlign1 on the stack (like byval), and passes RequestedAlign8 indirectly.
Clang passes NoRequestedAlign on the stack (with byval), and passes RequestedAlign1 and RequestedAlign8 indirectly.

It seems that Clang passes a struct indirectly if there is any __declspec(align) requested alignment, but MSVC only does so if the requested alignment is > 4.

Demo: https://clang.godbolt.org/z/fsn78PhxT

@nikic
Copy link
Contributor

nikic commented Jun 11, 2023

cc @rnk who implemented https://reviews.llvm.org/D72114.

@rnk
Copy link
Collaborator

rnk commented Jun 12, 2023

The text from the commit message describes the correct behavior: "remaining arguments with required alignment greater than 4 bytes are passed indirectly", but I guess there must be a bug in implementation.

@rnk
Copy link
Collaborator

rnk commented Jun 12, 2023

This code doesn't look right:
https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ASTContext.cpp#L2339

It seems like we have a related bug if you move the alignment requirement from the RecordDecl to the FieldDecl:
https://clang.godbolt.org/z/PTccrsd8j

Clang passes directly, but MSVC passes indirectly. Ideally, we should pull the required alignment from the ASTRecordLayout, which has the RequiredAlign field and rolls up all the subobject alignment requirements, but I'm sure that will break something somewhere.

@rnk
Copy link
Collaborator

rnk commented Jun 12, 2023

I think MSVC is actually ignoring the requests to underalign the struct, see warning C4359, which you get if you move the declspec between struct and the identifier.

This is a janky patch, but I think it will fix things:

$ git diff
diff --git a/clang/lib/CodeGen/TargetInfo.cpp b/clang/lib/CodeGen/TargetInfo.cpp
index 3b9cce40b2fb..ce19c960353a 100644
--- a/clang/lib/CodeGen/TargetInfo.cpp
+++ b/clang/lib/CodeGen/TargetInfo.cpp
@@ -1894,8 +1894,16 @@ ABIArgInfo X86_32ABIInfo::classifyArgumentType(QualType Ty,
 
     // Pass over-aligned aggregates on Windows indirectly. This behavior was
     // added in MSVC 2015.
-    if (IsWin32StructABI && TI.isAlignRequired() && TI.Align > 32)
-      return getIndirectResult(Ty, /*ByVal=*/false, State);
+    if (IsWin32StructABI) {
+      if (RT) {
+        const ASTRecordLayout &Layout =
+            getContext().getASTRecordLayout(RT->getDecl());
+        if (Layout.getRequiredAlignment() > CharUnits::fromQuantity(4))
+          return getIndirectResult(Ty, /*ByVal=*/false, State);
+      } else if (TI.isAlignRequired() && TI.Align > 32) {
+        return getIndirectResult(Ty, /*ByVal=*/false, State);
+      }
+    }
 
     // Expand small (<= 128-bit) record types when we know that the stack layout
     // of those arguments will match the struct. This is important because the

The meaning of TypeInfo::AlignRequirement still seems underspecified.

@rnk
Copy link
Collaborator

rnk commented Jun 13, 2023

Should be fixed by 651e5ae

@rnk rnk closed this as completed Jun 13, 2023
hubot pushed a commit to wine-mirror/wine that referenced this issue Feb 12, 2025
Based on Piotr's findings.

On MSVC i386 targets, structs requiring alignment greater than 4 are never passed by value.
Clang follows the same behavior in MSVC mode (see [1] for details and [2] for a follow-up
that applies this logic when fields, not necessarily the entire struct, are aligned).

A number of ios functions take fpos_mbstatet as an argument and expect it to be passed by value.

[1] https://reviews.llvm.org/D72114
[2] llvm/llvm-project#63257

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57817
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABI Application Binary Interface llvm:codegen
Projects
None yet
Development

No branches or pull requests

4 participants