[clang] Ensure minimal alignment of global vars of incomplete type.

[JonPsson1]

The SystemZ ABI requires any global variable to be aligned to at least 2 bytes, and therefore an external global Value with an opaque type should get this alignment as well.

[llvmbot]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Jonas Paulsson (JonPsson1)

Changes

The SystemZ ABI requires any global variable to be aligned to at least 2 bytes, and therefore an external global Value with an opaque type should get this alignment as well.

---

Full diff: https://github.com/llvm/llvm-project/pull/72886.diff

2 Files Affected:

• (modified) clang/lib/AST/ASTContext.cpp (+8-6)
• (added) clang/test/Driver/systemz-alignment.c (+32)

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 1c893d008cb49f3..a7cee3b7ba2b0db 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -1680,14 +1680,16 @@ CharUnits ASTContext::getDeclAlign(const Decl *D, bool ForAlignof) const {
       Align = std::max(Align, getPreferredTypeAlign(T.getTypePtr()));
       if (BaseT.getQualifiers().hasUnaligned())
         Align = Target->getCharWidth();
-      if (const auto *VD = dyn_cast<VarDecl>(D)) {
-        if (VD->hasGlobalStorage() && !ForAlignof) {
-          uint64_t TypeSize = getTypeSize(T.getTypePtr());
-          Align = std::max(Align, getTargetInfo().getMinGlobalAlign(TypeSize));
-        }
-      }
     }
 
+    // Ensure miminum alignment for global variables.
+    if (const auto *VD = dyn_cast<VarDecl>(D))
+      if (VD->hasGlobalStorage() && !ForAlignof) {
+        uint64_t TypeSize =
+            !BaseT->isIncompleteType() ? getTypeSize(T.getTypePtr()) : 0;
+        Align = std::max(Align, getTargetInfo().getMinGlobalAlign(TypeSize));
+      }
+
     // Fields can be subject to extra alignment constraints, like if
     // the field is packed, the struct is packed, or the struct has a
     // a max-field-alignment constraint (#pragma pack).  So calculate
diff --git a/clang/test/Driver/systemz-alignment.c b/clang/test/Driver/systemz-alignment.c
new file mode 100644
index 000000000000000..6f3b2bc38be3688
--- /dev/null
+++ b/clang/test/Driver/systemz-alignment.c
@@ -0,0 +1,32 @@
+// RUN: %clang --target=s390x-linux -S -emit-llvm -o - %s | FileCheck %s
+//
+// Test that a global variable with an incomplete type gets the minimum
+// alignment of 2 per the ABI if no alignment was specified by user.
+//
+// CHECK:      @VarNoAl {{.*}} align 2
+// CHECK-NEXT: @VarExplAl1  {{.*}} align 1
+// CHECK-NEXT: @VarExplAl4  {{.*}} align 4
+
+// No alignemnt specified by user.
+struct incomplete_ty_noal;
+extern struct incomplete_ty_noal VarNoAl;
+struct incomplete_ty_noal *fun0 (void)
+{
+  return &VarNoAl;
+}
+
+// User-specified alignment of 1.
+struct incomplete_ty_al1;
+extern struct incomplete_ty_al1 __attribute__((aligned(1))) VarExplAl1;
+struct incomplete_ty_al1 *fun1 (void)
+{
+  return &VarExplAl1;
+}
+
+// User-specified alignment of 4.
+struct incomplete_ty_al4;
+extern struct incomplete_ty_al4 __attribute__((aligned(4))) VarExplAl4;
+struct incomplete_ty_al4 *fun2 (void)
+{
+  return &VarExplAl4;
+}

[JonPsson1]

@efriedma-quic
@rjmccall
@uweigand
(for review)

[efriedma-quic]

LGTM

The questions that I came up with while reviewing:

• Does this affect other targets?

It looks like it affects Lanai, but that difference seems fine. It looks like it doesn't affect anything else in-tree; in particular, the overrides just return the default for size=0. Maybe out-of-tree, but we can't really do much about that (unless we want to add a separate target hook, but that seems like overkill).

• Are there any possible side-effects?

Given the actual usage, this seems safe enough. I'm a little concerned about people doing weird stuff with extern globals and linker scripts, but it looks like none of the targets where that would matter are actually impacted by this.

[nikic]

Why is this a Driver test rather than a CodeGen test using clang_cc1?

[JonPsson1]

I found some other align tests there, like arm-alignment.c, so I thought this should work..?

[efriedma-quic]

Sorry, I missed this when reviewing.

We generally only use Driver tests when we're trying to test some aspect of the driver. For arm-alignment.c, it isn't really trying to test the code generation itself; it's trying to test the effect of alignment-related flags.

[JonPsson1]

Sorry, my bad. There is a SystemZ test for this already, so I moved these tests into it instead. I created a new PR: #73230.