Page MenuHomePhabricator

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

Authored by rsmith on Apr 2 2018, 12:04 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

rsmith created this revision.Apr 2 2018, 12:04 PM
rsmith added a reviewer: rnk.Apr 2 2018, 1:30 PM

+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 added a subscriber: majnemer.Apr 2 2018, 3:09 PM

+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 added a comment.Apr 2 2018, 4:35 PM
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 added a comment.Apr 2 2018, 5:48 PM

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.

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 ↗(On Diff #140654)

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

rsmith marked an inline comment as done.Apr 3 2018, 11:31 AM
rsmith added inline comments.
ReleaseNotes.rst
68–72 ↗(On Diff #140654)

Fixed in r329098.

rsmith marked an inline comment as done.May 4 2018, 2:39 PM

Ping.

rjmccall accepted this revision.May 4 2018, 8:28 PM

Okay, LGTM.

This revision is now accepted and ready to land.May 4 2018, 8:28 PM
This revision was automatically updated to reflect the committed changes.