non-zero-length bit-fields should make a class non-empty

[rsmith]

Summary

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.

[rsmith]

+rnk This might also affect the MS ABI, but it does not result in any test case failures at least (and MSVC's type trait matches our state after this patch rather than before).

[rnk]

In D45174#1054820, @rsmith wrote:

+rnk This might also affect the MS ABI, but it does not result in any test case failures at least (and MSVC's type trait matches our state after this patch rather than before).

I've convinced myself that this is NFC for MS record layout because this is the only place we use RD->isEmpty() that matters:

if (!FoundBase) {
  if (MDCUsesEBO && BaseDecl->isEmpty() &&
      BaseLayout.getNonVirtualSize() == CharUnits::Zero()) {
    BaseOffset = CharUnits::Zero();
  } else {
    // Otherwise, lay the base out at the end of the MDC.
    BaseOffset = Size = Size.alignTo(Info.Alignment);
  }
}

In particular, that getNonVirtualSize() check returns false when unnamed bitfields get involved. Here's a layout:

struct __declspec(empty_bases) A { char : 3; };
struct __declspec(empty_bases) B { char : 3; };
struct UseEmpty : A, B {
  char space;
} o[1];
static_assert(sizeof(o) == 3, "incorrect layout");


*** Dumping AST Record Layout
         0 | struct A (empty)
     0:0-2 |   char
           | [sizeof=1, align=1,
           |  nvsize=1, nvalign=1]

*** Dumping AST Record Layout
         0 | struct B (empty)
     0:0-2 |   char
           | [sizeof=1, align=1,
           |  nvsize=1, nvalign=1]

*** Dumping AST Record Layout
         0 | struct UseEmpty
         0 |   struct A (base) (empty)
     0:0-2 |     char
         1 |   struct B (base) (empty)
     1:0-2 |     char
         2 |   char space
           | [sizeof=3, align=1,
           |  nvsize=3, nvalign=1]

So even though these classes went from "empty" to "non-empty", we previously allocated space for them. I think @majnemer spent a while on that.

[rsmith]

In D45174#1054936, @rnk wrote:

I've convinced myself that this is NFC for MS record layout because this is the only place we use RD->isEmpty() that matters:

if (!FoundBase) {
  if (MDCUsesEBO && BaseDecl->isEmpty() &&
      BaseLayout.getNonVirtualSize() == CharUnits::Zero()) {
    BaseOffset = CharUnits::Zero();
  } else {
    // Otherwise, lay the base out at the end of the MDC.
    BaseOffset = Size = Size.alignTo(Info.Alignment);
  }
}

In particular, that getNonVirtualSize() check returns false when unnamed bitfields get involved.

I wonder if we can delete the getNonVirtualSize() check now -- I don't see any way that an empty class can have a nonzero nvsize except by this nonempty anonymous bit-fields situation.

[rsmith]

In D45174#1055048, @rsmith wrote:

I wonder if we can delete the getNonVirtualSize() check now -- I don't see any way that an empty class can have a nonzero nvsize except by this nonempty anonymous bit-fields situation.

Yup, looks like:

   if (!FoundBase) {
-    if (MDCUsesEBO && BaseDecl->isEmpty() &&
-        BaseLayout.getNonVirtualSize() == CharUnits::Zero()) {
+    if (MDCUsesEBO && BaseDecl->isEmpty()) {
+      assert(BaseLayout.getNonVirtualSize() == CharUnits::Zero());
       BaseOffset = CharUnits::Zero();
     } else {

Zero test failures.

[majnemer]

In D45174#1055125, @rsmith wrote:

In D45174#1055048, @rsmith wrote:

I wonder if we can delete the getNonVirtualSize() check now -- I don't see any way that an empty class can have a nonzero nvsize except by this nonempty anonymous bit-fields situation.

Yup, looks like:

   if (!FoundBase) {
-    if (MDCUsesEBO && BaseDecl->isEmpty() &&
-        BaseLayout.getNonVirtualSize() == CharUnits::Zero()) {
+    if (MDCUsesEBO && BaseDecl->isEmpty()) {
+      assert(BaseLayout.getNonVirtualSize() == CharUnits::Zero());
       BaseOffset = CharUnits::Zero();
     } else {

Zero test failures.

Yeah, I think this should be NFC for the MS ABI.

ReleaseNotes.rst

68–72

The "Clang" in the above paragraph has a lowercase "clang". I think we should choose a consistent capitalization.

[rsmith]

ReleaseNotes.rst

68–72

Fixed in r329098.

[rsmith]

Ping.

[rjmccall]

Okay, LGTM.

AST/DeclCXX.cpp

if (SMKind) {
       }
     }
 
     return;
   }
 
   // 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
     //   private or protected non-static data members (clause 11).
     //
     // A POD must be an aggregate.    
     if (D->getAccess() == AS_private || D->getAccess() == AS_protected) {
       data().Aggregate = false;
if (FieldDecl *Field = dyn_cast<FieldDecl>(D)) {
 
     // C++0x [class]p9:
     //   A POD struct is a class that is both a trivial class and a 
     //   standard-layout class, and has no non-static data members of type 
     //   non-POD struct, non-POD union (or array of such types).
     //
     // 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()) {
         // Objective-C Automatic Reference Counting:
         //   If a class has a non-static data member of Objective-C pointer
         //   type (or array thereof), it is a non-POD type and its
         //   default constructor (if any), copy constructor, move constructor,
         //   copy assignment operator, move assignment operator, and destructor are
if (FieldDecl *Field = dyn_cast<FieldDecl>(D)) {
     //       class and at most one base class with non-static data members,
     //       or has no base classes with non-static data members, and
     // At this point we know that we have a non-static data member, so the last
     // clause holds.
     if (!data().HasNoNonEmptyBases)
       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
   
   // Handle using declarations of conversion functions.
   if (UsingShadowDecl *Shadow = dyn_cast<UsingShadowDecl>(D)) {
     if (Shadow->getDeclName().getNameKind()
           == DeclarationName::CXXConversionFunctionName) {
       ASTContext &Ctx = getASTContext();
       data().Conversions.get(Ctx).addDecl(Ctx, Shadow, Shadow->getAccess());
     }

Layout/ms-x86-pack-and-align.cpp

 // CHECK-X64-NEXT:      | [sizeof=17, align=1
 // CHECK-X64-NEXT:      |  nvsize=17, nvalign=1]
 
 #pragma pack(16)
 struct YA {
   __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]
 
 #pragma pack(1)
 struct YB {
   char a;
   YA b;
 };
 // 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]
 
 #pragma pack(8)
 struct YC {
   __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]
 
 #pragma pack(1)
 struct YD {
   char a;
   YC b;
 };
 // 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]
 
 #pragma pack(4)
 struct YE {
   __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]
 
 #pragma pack(1)
 struct YF {
   char a;
   YE b;
 };
 // 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]
 
 #pragma pack(16)
 struct __declspec(align(16)) D0 { char a; };
 #pragma pack(1)
 struct D1 : public D0 { char a; };

Layout/v6-empty.cpp

+// 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]

ReleaseNotes.rst

- ``-Wextra-semi`` now also controls ``-Wc++98-compat-extra-semi``.
   to explicitly re-enable it (e.g. by appending ``-Wextra-semi``).
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 - 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
 ------------------
 
 - ...
 
 Deprecated Compiler Flags

SemaCXX/type-traits.cpp

// { int arr[F(__is_pod(NonPODUnion))]; }
   { int arr[F(__is_pod(AnIncompleteType))]; } // expected-error {{incomplete type}}
   { int arr[F(__is_pod(AnIncompleteType[]))]; } // expected-error {{incomplete type}}
   { int arr[F(__is_pod(AnIncompleteType[1]))]; } // expected-error {{incomplete type}}
 }
 
 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 {};
 
 void is_empty()
 {
   { int arr[T(__is_empty(Empty))]; }
   { int arr[T(__is_empty(DerivesEmpty))]; }
   { int arr[T(__is_empty(HasCons))]; }
void is_empty()
 
   { int arr[F(__is_empty(Int))]; }
   { int arr[F(__is_empty(POD))]; }
   { int arr[F(__is_empty(EmptyUnion))]; }
   { int arr[F(__is_empty(IncompleteUnion))]; }
   { 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))]; }
   { int arr[F(__is_empty(HasAnonymousUnion))]; }
 //  { int arr[F(__is_empty(DerivesVirt))]; }
 
   { int arr[T(__is_empty(ACompleteType))]; }
   { int arr[F(__is_empty(AnIncompleteType))]; } // expected-error {{incomplete type}}