PR37275 packed attribute should not apply to base classes

[rsmith]

Summary

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.

[rsmith]

Added release notes.

[rjmccall]

I wonder if that was originally just an oversight that got turned into official policy. Anyway, if it's the policy, it's what we have to do; LGTM.

Have you checked whether we do the right thing with #pragma pack on MSVC?

[rsmith]

In D46218#1081933, @rjmccall wrote:

Have you checked whether we do the right thing with #pragma pack on MSVC?

Great question, I hadn't. Both MSVC and GCC treat #pragma pack as applying to base classes, which is what we do with/without this patch. I'll add some tests to cover that.

[probinson]

In D46218#1081933, @rjmccall wrote:

I wonder if that was originally just an oversight that got turned into official policy. Anyway, if it's the policy, it's what we have to do; LGTM.

I think it's actually correct behavior. Why would an attribute on a derived class be allowed to alter the layout of a base class? It would mean you can't convert Derived* to Base* safely.

[rsmith]

In D46218#1082834, @probinson wrote:

In D46218#1081933, @rjmccall wrote:

I wonder if that was originally just an oversight that got turned into official policy. Anyway, if it's the policy, it's what we have to do; LGTM.

I think it's actually correct behavior. Why would an attribute on a derived class be allowed to alter the layout of a base class? It would mean you can't convert Derived* to Base* safely.

With Clang's old behavior, base classes were treated exactly like fields -- their layout was not changed per se, but their minimum alignment was reduced. Yes, that means you can't convert Derived* to Base* safely, but the packed attribute means you can't use &object.member to convert Class* to Member* safely either, so that's not an entirely unique problem. Probably more serious is that GCC doesn't permit a T& to bind to a packed field, so treating base classes as packed would interfere with passing a Derived lvalue to a Base&, which does seem like a much more severe restriction for base classes than for fields.

cfe/trunk/docs/ReleaseNotes.rst

- Clang binary and libraries have been renamed from 7.0 to 7.
   For example, the ``clang`` binary will be called ``clang-7``
   instead of ``clang-7.0``.
 
 - Clang implements a collection of recent fixes to the C++ standard's definition
   of "standard-layout". In particular, a class is only considered to be
   standard-layout if all base classes and the first data member (or bit-field)
   can be laid out at offset zero.
 
+- Clang's handling of the GCC ``packed`` class attribute in C++ has been fixed
+  to apply only to non-static data members and not to base classes. This fixes
+  an ABI difference between Clang and GCC, but creates an ABI difference between
+  Clang 7 and earlier versions. The old behavior can be restored by setting
+  ``-fclang-abi-compat`` to ``6`` or earlier.
+
 - ...
 
 New Compiler Flags
 ------------------
 
 - :option:`-ffp-cast-overflow-workaround` and
   :option:`-fno-fp-cast-overflow-workaround`
   enable (disable) a workaround for code that casts floating-point values to

cfe/trunk/lib/AST/RecordLayoutBuilder.cpp

if (IsVirtual) {
              "Non-virtual base already exists!");
       NonVirtualBaseInfo.insert(std::make_pair(BaseDecl, Info));
     }
   }
 }
 
 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()) {
     BaseAlign = std::min(BaseAlign, MaxFieldAlignment);
     UnpackedBaseAlign = std::min(UnpackedBaseAlign, MaxFieldAlignment);
   }
 
   // Round up the current record size to pointer alignment.
ItaniumRecordLayoutBuilder::LayoutBase(const BaseSubobjectInfo *Base) {
   bool HasExternalLayout = false;
   if (UseExternalLayout) {
     if (Base->IsVirtual)
       HasExternalLayout = External.getExternalNVBaseOffset(Base->Class, Offset);
     else
       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()) &&
       EmptySubobjects->CanPlaceBaseAtOffset(Base, CharUnits::Zero())) {
     setSize(std::max(getSize(), Layout.getSize()));
     UpdateAlignment(BaseAlign, UnpackedBaseAlign);
 
     return CharUnits::Zero();

cfe/trunk/test/CodeGenCXX/alignment.cpp

-// 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);
 
 namespace test0 {
   struct A {
     int aField;
     int bField;
namespace test0 {
   void b(C &c) {
     // CHECK: [[CALL:%.*]] = call i32 @_Z10int_sourcev()
     // 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: [[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
     // CHECK: call void @_Z8int_sinki(i32 [[T2]])
     int_sink(c.onebit);
   }
 
   // CHECK-LABEL: @_ZN5test01cEPNS_1CE
   void c(C *c) {
     // CHECK: [[CALL:%.*]] = call i32 @_Z10int_sourcev()
     // 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: [[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
     // CHECK: call void @_Z8int_sinki(i32 [[T2]])
     int_sink(c->onebit);
   }
 
   // These accesses should have alignment 2 because they're at offset 8
   // 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
     // CHECK: call void @_Z8int_sinki(i32 [[T2]])
     int_sink(c.onebit);
   }
 
   // These accesses should have alignment 8 because they're at offset 8

cfe/trunk/test/SemaCXX/class-layout.cpp

 // 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]
 
 struct A {
   int a;
   char b;
 };
namespace test18 {
   };
 
   struct might_use_tail_padding : public F {
     char m;
   };
   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), "");
+
+  // __attribute__((packed)) does not apply to base classes.
+  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
+
+  // #pragma pack does, though.
+#pragma pack(push, 2)
+  struct C : X, A {};
+  _Static_assert(_Alignof(C) == 2, "");
+  _Static_assert(__builtin_offsetof(C, n) == 2, "");
+
+  struct __attribute__((packed)) D : X, A {};
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 6
+  _Static_assert(_Alignof(D) == 1, "");
+  _Static_assert(__builtin_offsetof(D, n) == 1, "");
+#else
+  _Static_assert(_Alignof(D) == 2, "");
+  _Static_assert(__builtin_offsetof(D, n) == 2, "");
+#endif
+#pragma pack(pop)
+}