rsmith created this revision.
rsmith added a reviewer: rjmccall.
Herald added a subscriber: sanjoy.

This implements the rule intended by the standard (see LWG 2358) and the rule 
presumably intended by the Itanium C++ ABI (see 
https://github.com/itanium-cxx-abi/cxx-abi/pull/51), and makes Clang match the 
behavior of GCC, ICC, and MSVC. A pedantic reading of both the standard and the 
ABI indicate that Clang is currently technically correct, but that's not worth 
much when it's clear that the wording is wrong in both those places.

This is an ABI break for classes that derive from a class that is empty other 
than one or more unnamed non-zero-length bit-fields. Such cases are expected to 
be rare, but -fclang-abi-compat=6 restores the old behavior just in case.


Repository:
  rC Clang

https://reviews.llvm.org/D45174

Files:
  AST/DeclCXX.cpp
  Layout/ms-x86-pack-and-align.cpp
  Layout/v6-empty.cpp
  ReleaseNotes.rst
  SemaCXX/type-traits.cpp

Index: SemaCXX/type-traits.cpp
===================================================================
--- SemaCXX/type-traits.cpp
+++ SemaCXX/type-traits.cpp
@@ -262,6 +262,7 @@
 typedef Empty EmptyAr[10];
 struct Bit0 { int : 0; };
 struct Bit0Cons { int : 0; Bit0Cons(); };
+struct AnonBitOnly { int : 3; };
 struct BitOnly { int x : 3; };
 struct DerivesVirt : virtual POD {};
 
@@ -287,6 +288,7 @@
   { int arr[F(__is_empty(EmptyAr))]; }
   { int arr[F(__is_empty(HasRef))]; }
   { int arr[F(__is_empty(HasVirt))]; }
+  { int arr[F(__is_empty(AnonBitOnly))]; }
   { int arr[F(__is_empty(BitOnly))]; }
   { int arr[F(__is_empty(void))]; }
   { int arr[F(__is_empty(IntArNB))]; }
Index: Layout/v6-empty.cpp
===================================================================
--- /dev/null
+++ Layout/v6-empty.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -fsyntax-only -fclang-abi-compat=6 -triple x86_64-linux-gnu -fdump-record-layouts %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-V6
+// RUN: %clang_cc1 -fsyntax-only -fclang-abi-compat=7 -triple x86_64-linux-gnu -fdump-record-layouts %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-V7
+
+// In Clang 6 and before, we determined that Nonempty was empty, so we
+// applied EBO to it.
+struct Nonempty { int : 4; };
+struct A : Nonempty { int n; };
+int k = sizeof(A);
+
+// CHECK:*** Dumping AST Record Layout
+// CHECK:             0 | struct A
+// CHECK-V6-NEXT:     0 |   struct Nonempty (base) (empty)
+// CHECK-V7-NEXT:     0 |   struct Nonempty (base){{$}}
+// CHECK-NEXT:    0:0-3 |     int
+// CHECK-V6-NEXT:     0 |   int n
+// CHECK-V7-NEXT:     4 |   int n
+// CHECK-V6-NEXT:       | [sizeof=4, dsize=4, align=4,
+// CHECK-V6-NEXT:       |  nvsize=4, nvalign=4]
+// CHECK-V7-NEXT:       | [sizeof=8, dsize=8, align=4,
+// CHECK-V7-NEXT:       |  nvsize=8, nvalign=4]
Index: Layout/ms-x86-pack-and-align.cpp
===================================================================
--- Layout/ms-x86-pack-and-align.cpp
+++ Layout/ms-x86-pack-and-align.cpp
@@ -188,12 +188,12 @@
 	__declspec(align(32)) char : 1;
 };
 // CHECK: *** Dumping AST Record Layout
-// CHECK-NEXT:    0 | struct YA (empty)
+// CHECK-NEXT:    0 | struct YA
 // CHECK-NEXT:0:0-0 |   char
 // CHECK-NEXT:      | [sizeof=32, align=32
 // CHECK-NEXT:      |  nvsize=32, nvalign=32]
 // CHECK-X64: *** Dumping AST Record Layout
-// CHECK-X64-NEXT:    0 | struct YA (empty)
+// CHECK-X64-NEXT:    0 | struct YA
 // CHECK-X64-NEXT:0:0-0 |   char
 // CHECK-X64-NEXT:      | [sizeof=32, align=32
 // CHECK-X64-NEXT:      |  nvsize=32, nvalign=32]
@@ -206,14 +206,14 @@
 // CHECK: *** Dumping AST Record Layout
 // CHECK-NEXT:    0 | struct YB
 // CHECK-NEXT:    0 |   char a
-// CHECK-NEXT:    1 |   struct YA b (empty)
+// CHECK-NEXT:    1 |   struct YA b
 // CHECK-NEXT:1:0-0 |     char
 // CHECK-NEXT:      | [sizeof=33, align=1
 // CHECK-NEXT:      |  nvsize=33, nvalign=1]
 // CHECK-X64: *** Dumping AST Record Layout
 // CHECK-X64-NEXT:    0 | struct YB
 // CHECK-X64-NEXT:    0 |   char a
-// CHECK-X64-NEXT:    1 |   struct YA b (empty)
+// CHECK-X64-NEXT:    1 |   struct YA b
 // CHECK-X64-NEXT:1:0-0 |     char
 // CHECK-X64-NEXT:      | [sizeof=33, align=1
 // CHECK-X64-NEXT:      |  nvsize=33, nvalign=1]
@@ -223,12 +223,12 @@
 	__declspec(align(32)) char : 1;
 };
 // CHECK: *** Dumping AST Record Layout
-// CHECK-NEXT:    0 | struct YC (empty)
+// CHECK-NEXT:    0 | struct YC
 // CHECK-NEXT:0:0-0 |   char
 // CHECK-NEXT:      | [sizeof=32, align=32
 // CHECK-NEXT:      |  nvsize=32, nvalign=32]
 // CHECK-X64: *** Dumping AST Record Layout
-// CHECK-X64-NEXT:    0 | struct YC (empty)
+// CHECK-X64-NEXT:    0 | struct YC
 // CHECK-X64-NEXT:    0:0-0 |   char
 // CHECK-X64-NEXT:      | [sizeof=8, align=32
 // CHECK-X64-NEXT:      |  nvsize=8, nvalign=32]
@@ -241,14 +241,14 @@
 // CHECK: *** Dumping AST Record Layout
 // CHECK-NEXT:    0 | struct YD
 // CHECK-NEXT:    0 |   char a
-// CHECK-NEXT:    1 |   struct YC b (empty)
+// CHECK-NEXT:    1 |   struct YC b
 // CHECK-NEXT:1:0-0 |     char
 // CHECK-NEXT:      | [sizeof=33, align=1
 // CHECK-NEXT:      |  nvsize=33, nvalign=1]
 // CHECK-X64: *** Dumping AST Record Layout
 // CHECK-X64-NEXT:    0 | struct YD
 // CHECK-X64-NEXT:    0 |   char a
-// CHECK-X64-NEXT:    1 |   struct YC b (empty)
+// CHECK-X64-NEXT:    1 |   struct YC b
 // CHECK-X64-NEXT:1:0-0 |     char
 // CHECK-X64-NEXT:      | [sizeof=9, align=1
 // CHECK-X64-NEXT:      |  nvsize=9, nvalign=1]
@@ -258,12 +258,12 @@
 	__declspec(align(32)) char : 1;
 };
 // CHECK: *** Dumping AST Record Layout
-// CHECK-NEXT:    0 | struct YE (empty)
+// CHECK-NEXT:    0 | struct YE
 // CHECK-NEXT:    0:0-0 |   char
 // CHECK-NEXT:      | [sizeof=4, align=32
 // CHECK-NEXT:      |  nvsize=4, nvalign=32]
 // CHECK-X64: *** Dumping AST Record Layout
-// CHECK-X64-NEXT:    0 | struct YE (empty)
+// CHECK-X64-NEXT:    0 | struct YE
 // CHECK-X64-NEXT:    0:0-0 |   char
 // CHECK-X64-NEXT:      | [sizeof=4, align=32
 // CHECK-X64-NEXT:      |  nvsize=4, nvalign=32]
@@ -276,14 +276,14 @@
 // CHECK: *** Dumping AST Record Layout
 // CHECK-NEXT:    0 | struct YF
 // CHECK-NEXT:    0 |   char a
-// CHECK-NEXT:    1 |   struct YE b (empty)
+// CHECK-NEXT:    1 |   struct YE b
 // CHECK-NEXT:1:0-0 |     char
 // CHECK-NEXT:      | [sizeof=5, align=1
 // CHECK-NEXT:      |  nvsize=5, nvalign=1]
 // CHECK-X64: *** Dumping AST Record Layout
 // CHECK-X64-NEXT:    0 | struct YF
 // CHECK-X64-NEXT:    0 |   char a
-// CHECK-X64-NEXT:    1 |   struct YE b (empty)
+// CHECK-X64-NEXT:    1 |   struct YE b
 // CHECK-X64-NEXT:1:0-0 |     char
 // CHECK-X64-NEXT:      | [sizeof=5, align=1
 // CHECK-X64-NEXT:      |  nvsize=5, nvalign=1]
Index: AST/DeclCXX.cpp
===================================================================
--- AST/DeclCXX.cpp
+++ AST/DeclCXX.cpp
@@ -737,12 +737,22 @@
 
   // Handle non-static data members.
   if (FieldDecl *Field = dyn_cast<FieldDecl>(D)) {
+    ASTContext &Context = getASTContext();
+
     // C++ [class.bit]p2:
     //   A declaration for a bit-field that omits the identifier declares an 
     //   unnamed bit-field. Unnamed bit-fields are not members and cannot be 
     //   initialized.
-    if (Field->isUnnamedBitfield())
+    if (Field->isUnnamedBitfield()) {
+      // C++ [meta.unary.prop]p4: [LWG2358]
+      //   T is a class type [...] with [...] no unnamed bit-fields of non-zero
+      //   length
+      if (data().Empty && !Field->isZeroLengthBitField(Context) &&
+          Context.getLangOpts().getClangABICompat() >
+              LangOptions::ClangABI::Ver6)
+        data().Empty = false;
       return;
+    }
     
     // C++ [dcl.init.aggr]p1:
     //   An aggregate is an array or a class (clause 9) with [...] no
@@ -787,7 +797,6 @@
     //
     // Automatic Reference Counting: the presence of a member of Objective-C pointer type
     // that does not explicitly have no lifetime makes the class a non-POD.
-    ASTContext &Context = getASTContext();
     QualType T = Context.getBaseElementType(Field->getType());
     if (T->isObjCRetainableType() || T.isObjCGCStrong()) {
       if (T.hasNonTrivialObjCLifetime()) {
@@ -1082,12 +1091,8 @@
       data().IsStandardLayout = false;
 
     // C++14 [meta.unary.prop]p4:
-    //   T is a class type [...] with [...] no non-static data members other
-    //   than bit-fields of length 0...
-    if (data().Empty) {
-      if (!Field->isZeroLengthBitField(Context))
-        data().Empty = false;
-    }
+    //   T is a class type [...] with [...] no non-static data members
+    data().Empty = false;
   }
   
   // Handle using declarations of conversion functions.
Index: ReleaseNotes.rst
===================================================================
--- ReleaseNotes.rst
+++ ReleaseNotes.rst
@@ -65,6 +65,15 @@
 - 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 the proposed resolution of LWG issue 2358, along with the
+  `corresponding change to the Itanium C++ ABI
+  <https://github.com/itanium-cxx-abi/cxx-abi/pull/51>`_, which make classes
+  containing only unnamed non-zero-length bit-fields be considered non-empty.
+  This is an ABI break compared to prior Clang releases, but makes Clang
+  generate code that is ABI-compatible with other compilers. The old
+  behavior can be restored by setting ``-fclang-abi-compat`` to ``6`` or
+  lower.
+
 - ...
 
 New Compiler Flags
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D45174: n... Richard Smith - zygoloid via Phabricator via cfe-commits

Reply via email to