rsmith created this revision.
rsmith added a reviewer: rjmccall.

Clang incorrectly applies the packed attribute to base classes. Per GCC's 
documentation and as can be observed from its behavior, packed only applies to 
members, not base classes.

This change is conditioned behind `-fclang-abi-compat` so that an ABI break can 
be avoided by users if desired.


Repository:
  rC Clang

https://reviews.llvm.org/D46218

Files:
  lib/AST/RecordLayoutBuilder.cpp
  test/CodeGenCXX/alignment.cpp
  test/SemaCXX/class-layout.cpp

Index: test/SemaCXX/class-layout.cpp
===================================================================
--- test/SemaCXX/class-layout.cpp
+++ test/SemaCXX/class-layout.cpp
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
 // expected-no-diagnostics
 
 #define SA(n, p) int a##n[(p) ? 1 : -1]
@@ -570,3 +571,19 @@
   SA(0, sizeof(might_use_tail_padding) == 80);
 }
 } // namespace PR16537
+
+namespace PR37275 {
+  struct X { char c; };
+
+  struct A { int n; };
+  _Static_assert(_Alignof(A) == _Alignof(int), "");
+
+  struct __attribute__((packed)) B : X, A {};
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 6
+  _Static_assert(_Alignof(B) == 1, "");
+  _Static_assert(__builtin_offsetof(B, n) == 1, "");
+#else
+  _Static_assert(_Alignof(B) == _Alignof(int), "");
+  _Static_assert(__builtin_offsetof(B, n) == 4, "");
+#endif
+}
Index: test/CodeGenCXX/alignment.cpp
===================================================================
--- test/CodeGenCXX/alignment.cpp
+++ test/CodeGenCXX/alignment.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin10 | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin10 | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-NOCOMPAT
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin10 -fclang-abi-compat=6.0 | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-V6COMPAT
 
 extern int int_source();
 extern void int_sink(int x);
@@ -54,19 +55,22 @@
     // CHECK: [[B_P:%.*]] = bitcast i8* [[T1]] to [[B]]*
     // CHECK: [[FIELD_P:%.*]] = bitcast [[B]]* [[B_P]] to i8*
     // CHECK: [[TRUNC:%.*]] = trunc i32 [[CALL]] to i8
-    // CHECK: [[OLD_VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 2
+    // CHECK-V6COMPAT: [[OLD_VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 2
+    // CHECK-NOCOMPAT: [[OLD_VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 4
     // CHECK: [[T0:%.*]] = and i8 [[TRUNC]], 3
     // CHECK: [[T1:%.*]] = and i8 [[OLD_VALUE]], -4
     // CHECK: [[T2:%.*]] = or i8 [[T1]], [[T0]]
-    // CHECK: store i8 [[T2]], i8* [[FIELD_P]], align 2
+    // CHECK-V6COMPAT: store i8 [[T2]], i8* [[FIELD_P]], align 2
+    // CHECK-NOCOMPAT: store i8 [[T2]], i8* [[FIELD_P]], align 4
     c.onebit = int_source();
 
     // CHECK: [[C_P:%.*]] = load [[C]]*, [[C]]**
     // CHECK: [[T0:%.*]] = bitcast [[C]]* [[C_P]] to i8*
     // CHECK: [[T1:%.*]] = getelementptr inbounds i8, i8* [[T0]], i64 8
     // CHECK: [[B_P:%.*]] = bitcast i8* [[T1]] to [[B]]*
     // CHECK: [[FIELD_P:%.*]] = bitcast [[B]]* [[B_P]] to i8*
-    // CHECK: [[VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 2
+    // CHECK-V6COMPAT: [[VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 2
+    // CHECK-NOCOMPAT: [[VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 4
     // CHECK: [[T0:%.*]] = shl i8 [[VALUE]], 6
     // CHECK: [[T1:%.*]] = ashr i8 [[T0]], 6
     // CHECK: [[T2:%.*]] = sext i8 [[T1]] to i32
@@ -83,19 +87,22 @@
     // CHECK: [[B_P:%.*]] = bitcast i8* [[T1]] to [[B]]*
     // CHECK: [[FIELD_P:%.*]] = bitcast [[B]]* [[B_P]] to i8*
     // CHECK: [[TRUNC:%.*]] = trunc i32 [[CALL]] to i8
-    // CHECK: [[OLD_VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 2
+    // CHECK-V6COMPAT: [[OLD_VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 2
+    // CHECK-NOCOMPAT: [[OLD_VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 4
     // CHECK: [[T0:%.*]] = and i8 [[TRUNC]], 3
     // CHECK: [[T1:%.*]] = and i8 [[OLD_VALUE]], -4
     // CHECK: [[T2:%.*]] = or i8 [[T1]], [[T0]]
-    // CHECK: store i8 [[T2]], i8* [[FIELD_P]], align 2
+    // CHECK-V6COMPAT: store i8 [[T2]], i8* [[FIELD_P]], align 2
+    // CHECK-NOCOMPAT: store i8 [[T2]], i8* [[FIELD_P]], align 4
     c->onebit = int_source();
 
     // CHECK: [[C_P:%.*]] = load [[C:%.*]]*, [[C]]**
     // CHECK: [[T0:%.*]] = bitcast [[C]]* [[C_P]] to i8*
     // CHECK: [[T1:%.*]] = getelementptr inbounds i8, i8* [[T0]], i64 8
     // CHECK: [[B_P:%.*]] = bitcast i8* [[T1]] to [[B:%.*]]*
     // CHECK: [[FIELD_P:%.*]] = bitcast [[B]]* [[B_P]] to i8*
-    // CHECK: [[VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 2
+    // CHECK-V6COMPAT: [[VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 2
+    // CHECK-NOCOMPAT: [[VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 4
     // CHECK: [[T0:%.*]] = shl i8 [[VALUE]], 6
     // CHECK: [[T1:%.*]] = ashr i8 [[T0]], 6
     // CHECK: [[T2:%.*]] = sext i8 [[T1]] to i32
@@ -107,27 +114,31 @@
   // in an alignment-2 variable.
   // CHECK-LABEL: @_ZN5test01dEv
   void d() {
-    // CHECK: [[C_P:%.*]] = alloca [[C:%.*]], align 2
+    // CHECK-V6COMPAT: [[C_P:%.*]] = alloca [[C:%.*]], align 2
+    // CHECK-NOCOMPAT: [[C_P:%.*]] = alloca [[C:%.*]], align 4
     C c;
 
     // CHECK: [[CALL:%.*]] = call i32 @_Z10int_sourcev()
     // CHECK: [[T0:%.*]] = bitcast [[C]]* [[C_P]] to i8*
     // CHECK: [[T1:%.*]] = getelementptr inbounds i8, i8* [[T0]], i64 8
     // CHECK: [[B_P:%.*]] = bitcast i8* [[T1]] to [[B]]*
     // CHECK: [[FIELD_P:%.*]] = bitcast [[B]]* [[B_P]] to i8*
     // CHECK: [[TRUNC:%.*]] = trunc i32 [[CALL]] to i8
-    // CHECK: [[OLD_VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 2
+    // CHECK-V6COMPAT: [[OLD_VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 2
+    // CHECK-NOCOMPAT: [[OLD_VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 4
     // CHECK: [[T0:%.*]] = and i8 [[TRUNC]], 3
     // CHECK: [[T1:%.*]] = and i8 [[OLD_VALUE]], -4
     // CHECK: [[T2:%.*]] = or i8 [[T1]], [[T0]]
-    // CHECK: store i8 [[T2]], i8* [[FIELD_P]], align 2
+    // CHECK-V6COMPAT: store i8 [[T2]], i8* [[FIELD_P]], align 2
+    // CHECK-NOCOMPAT: store i8 [[T2]], i8* [[FIELD_P]], align 4
     c.onebit = int_source();
 
     // CHECK: [[T0:%.*]] = bitcast [[C]]* [[C_P]] to i8*
     // CHECK: [[T1:%.*]] = getelementptr inbounds i8, i8* [[T0]], i64 8
     // CHECK: [[B_P:%.*]] = bitcast i8* [[T1]] to [[B:%.*]]*
     // CHECK: [[FIELD_P:%.*]] = bitcast [[B]]* [[B_P]] to i8*
-    // CHECK: [[VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 2
+    // CHECK-V6COMPAT: [[VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 2
+    // CHECK-NOCOMPAT: [[VALUE:%.*]] = load i8, i8* [[FIELD_P]], align 4
     // CHECK: [[T0:%.*]] = shl i8 [[VALUE]], 6
     // CHECK: [[T1:%.*]] = ashr i8 [[T0]], 6
     // CHECK: [[T2:%.*]] = sext i8 [[T1]] to i32
Index: lib/AST/RecordLayoutBuilder.cpp
===================================================================
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -967,7 +967,7 @@
 
 void ItaniumRecordLayoutBuilder::EnsureVTablePointerAlignment(
     CharUnits UnpackedBaseAlign) {
-  CharUnits BaseAlign = (Packed) ? CharUnits::One() : UnpackedBaseAlign;
+  CharUnits BaseAlign = Packed ? CharUnits::One() : UnpackedBaseAlign;
 
   // The maximum field alignment overrides base align.
   if (!MaxFieldAlignment.isZero()) {
@@ -1175,9 +1175,14 @@
       HasExternalLayout = External.getExternalVBaseOffset(Base->Class, Offset);
   }
   
+  // Clang <= 6 incorrectly applied the 'packed' attribute to base classes.
+  // Per GCC's documentation, it only applies to non-static data members.
   CharUnits UnpackedBaseAlign = Layout.getNonVirtualAlignment();
-  CharUnits BaseAlign = (Packed) ? CharUnits::One() : UnpackedBaseAlign;
- 
+  CharUnits BaseAlign = (Packed && Context.getLangOpts().getClangABICompat() <=
+                                       LangOptions::ClangABI::Ver6)
+                            ? CharUnits::One()
+                            : UnpackedBaseAlign;
+
   // If we have an empty base class, try to place it at offset 0.
   if (Base->Class->isEmpty() &&
       (!HasExternalLayout || Offset == CharUnits::Zero()) &&
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D46218: P... Richard Smith - zygoloid via Phabricator via cfe-commits

Reply via email to