[AST] Improve support of external layouts in `MicrosoftRecordLayoutBuilder`

[aleksandr.urakov]

Summary

This patch fixes several small problems with external layouts support in MicrosoftRecordLayoutBuilder:

• aligns properly the size of a struct that ends with a bit field. It was aligned on byte before, not on the size of the field, so the struct size was smaller than it should be;
• adjusts the struct size when injecting a vbptr in the case when there were no bases or fields allocated after the vbptr. Similarly, without the adjustment the struct was smaller than it should be;
• the same fix as above for the vfptr.

All these fixes affect the non-virtual size of a struct, so they are tested through non-virtual inheritance.

[aleksandr.urakov]

Ping! Can you take a look, please?

[zturner]

Probably @rnk needs to look at this, i'll ping him today.

[rnk]

lgtm

lib/AST/RecordLayoutBuilder.cpp

2750–2753 ↗

(On Diff #187922)

I think this may be an interesting test case:

struct __declspec(align(16)) NVBase {
  int x, y;
  virtual ~NVBase();
};
struct VBase { int z; };
struct Foo : NVBase, virtual VBase {
};

On reading the code more, I don't think this test uncovers any bugs, but it seems worth including to make sure we do the right thing w.r.t. alignment.

[aleksandr.urakov]

Thank you! I've added a slightly reworked test too.

cfe/trunk/lib/AST/RecordLayoutBuilder.cpp

if (!UseExternalLayout && !IsUnion && LastFieldIsNonZeroWidthBitfield &&
     return;
   }
   LastFieldIsNonZeroWidthBitfield = true;
   CurrentBitfieldSize = Info.Size;
   if (UseExternalLayout) {
     auto FieldBitOffset = External.getExternalFieldOffset(FD);
     placeFieldAtBitOffset(FieldBitOffset);
     auto NewSize = Context.toCharUnitsFromBits(
-        llvm::alignTo(FieldBitOffset + Width, Context.getCharWidth()));
+        llvm::alignDown(FieldBitOffset, Context.toBits(Info.Alignment)) +
+        Context.toBits(Info.Size));
     Size = std::max(Size, NewSize);
     Alignment = std::max(Alignment, Info.Alignment);
   } else if (IsUnion) {
     placeFieldAtOffset(CharUnits::Zero());
     Size = std::max(Size, Info.Size);
     // TODO: Add a Sema warning that MS ignores bitfield alignment in unions.
   } else {
     // Allocate a new block of memory and place the bitfield in it.

 
 void MicrosoftRecordLayoutBuilder::injectVBPtr(const CXXRecordDecl *RD) {
   if (!HasVBPtr || SharedVBPtrBase)
     return;
   // Inject the VBPointer at the injection site.
   CharUnits InjectionSite = VBPtrOffset;
   // But before we do, make sure it's properly aligned.
   VBPtrOffset = VBPtrOffset.alignTo(PointerInfo.Alignment);
+  // Determine where the first field should be laid out after the vbptr.
+  CharUnits FieldStart = VBPtrOffset + PointerInfo.Size;
   // Shift everything after the vbptr down, unless we're using an external
   // layout.
-  if (UseExternalLayout)
+  if (UseExternalLayout) {
+    // It is possible that there were no fields or bases located after vbptr,
+    // so the size was not adjusted before.
+    if (Size < FieldStart)
+      Size = FieldStart;
     return;
-  // Determine where the first field should be laid out after the vbptr.
-  CharUnits FieldStart = VBPtrOffset + PointerInfo.Size;
+  }
   // Make sure that the amount we push the fields back by is a multiple of the
   // alignment.
   CharUnits Offset = (FieldStart - InjectionSite)
                          .alignTo(std::max(RequiredAlignment, Alignment));
   Size += Offset;
   for (uint64_t &FieldOffset : FieldOffsets)
     FieldOffset += Context.toBits(Offset);
   for (BaseOffsetsMapTy::value_type &Base : Bases)
     if (Base.second >= InjectionSite)
       Base.second += Offset;
 }
 
 void MicrosoftRecordLayoutBuilder::injectVFPtr(const CXXRecordDecl *RD) {
   if (!HasOwnVFPtr)
     return;
   // Make sure that the amount we push the struct back by is a multiple of the
   // alignment.
   CharUnits Offset =
       PointerInfo.Size.alignTo(std::max(RequiredAlignment, Alignment));
   // Push back the vbptr, but increase the size of the object and push back
   // regular fields by the offset only if not using external record layout.
   if (HasVBPtr)
     VBPtrOffset += Offset;
 
-  if (UseExternalLayout)
+  if (UseExternalLayout) {
+    // The class may have no bases or fields, but still have a vfptr
+    // (e.g. it's an interface class). The size was not correctly set before
+    // in this case.
+    if (FieldOffsets.empty() && Bases.empty())
+      Size += Offset;
     return;
+  }
 
   Size += Offset;
 
   // If we're using an external layout, the fields offsets have already
   // accounted for this adjustment.
   for (uint64_t &FieldOffset : FieldOffsets)
     FieldOffset += Context.toBits(Offset);
   for (BaseOffsetsMapTy::value_type &Base : Bases)

cfe/trunk/test/CodeGenCXX/Inputs/override-bit-field-layout.layout

 
 *** Dumping AST Record Layout
 Type: struct S1
 
 Layout: <ASTRecordLayout
   Size:16
   Alignment:16
   FieldOffsets: [0, 11]>
 
 *** Dumping AST Record Layout
 Type: struct S2
 
 Layout: <ASTRecordLayout
   Size:128
   Alignment:64
   FieldOffsets: [64]>
+
+*** Dumping AST Record Layout
+Type: struct S3
+
+Layout: <ASTRecordLayout
+  Size:32
+  Alignment:32
+  FieldOffsets: [0, 1]>

cfe/trunk/test/CodeGenCXX/Inputs/override-layout-virtual-base.layout

+
+*** Dumping AST Record Layout
+Type: struct S2
+
+Layout: <ASTRecordLayout
+  Size:64
+  Alignment:64
+  FieldOffsets: []>

cfe/trunk/test/CodeGenCXX/override-bit-field-layout.cpp

-// RUN: %clang_cc1 -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-bit-field-layout.layout %s | FileCheck %s
+// RUN: %clang_cc1 -w -triple=x86_64-pc-win32 -fms-compatibility -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-bit-field-layout.layout %s | FileCheck %s
 
 // CHECK: Type: struct S1
 // CHECK:   FieldOffsets: [0, 11]
 struct S1 {
   short a : 3;
   short b : 5;
 };
 
 // CHECK: Type: struct S2
 // CHECK:   FieldOffsets: [64]
 struct S2 {
   virtual ~S2() = default;
   short a : 3;
 };
 
+// CHECK: Type: struct S3
+// CHECK:   Size:32
+// CHECK:   FieldOffsets: [0, 1]
+struct S3 {
+  int a : 1;
+  int b : 2;
+};
+
+// CHECK: Type: struct S4
+// CHECK:   FieldOffsets: [32]
+struct S4 : S3 {
+  char c;
+};
+
 void use_structs() {
   S1 s1s[sizeof(S1)];
   S2 s2s[sizeof(S2)];
+  S3 s3s[sizeof(S3)];
+  S4 s4s[sizeof(S4)];
 }

cfe/trunk/test/CodeGenCXX/override-layout-virtual-base.cpp

+// RUN: %clang_cc1 -w -triple=x86_64-pc-win32 -fms-compatibility -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-layout-virtual-base.layout %s | FileCheck %s
+
+struct S1 {
+  int a;
+};
+
+struct S2 : virtual S1 {
+  virtual void foo() {}
+};
+
+// CHECK: Type: struct S3
+// CHECK:   FieldOffsets: [128]
+struct S3 : S2 {
+  char b;
+};
+
+void use_structs() {
+  S1 s1s[sizeof(S1)];
+  S2 s2s[sizeof(S2)];
+  S3 s3s[sizeof(S3)];
+}

cfe/trunk/test/CodeGenCXX/override-layout.cpp

struct PACKED X5 {
   union {
     long a;
     long b;
   };
   short l;
   short r;
 };
 
+// CHECK: Type: struct X6
+struct __attribute__((aligned(16))) X6 {
+  int x;
+  int y;
+  virtual ~X6();
+};
+
+// CHECK: Type: struct X7
+struct X7 {
+  int z;
+};
+
+// CHECK: Type: struct X8
+struct X8 : X6, virtual X7 {
+  char c;
+};
+
 void use_structs() {
   X0 x0s[sizeof(X0)];
   X1 x1s[sizeof(X1)];
   X2 x2s[sizeof(X2)];
   X3 x3s[sizeof(X3)];
   X4 x4s[sizeof(X4)];
   X5 x5s[sizeof(X5)];
+  X6 x6s[sizeof(X6)];
+  X7 x7s[sizeof(X7)];
+  X8 x8s[sizeof(X8)];
   x4s[1].a = 1;
   x5s[1].a = 17;
 }
-