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

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.

Reviewers: rnk, zturner, rsmith

Reviewed By: rnk

Subscribers: jdoerfert, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D58544

llvm-svn: 356047

6 files changed, 91 insertions, 7 deletions

diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index 99b7cbd0224..2d112c45222 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2692,7 +2692,8 @@ void MicrosoftRecordLayoutBuilder::layoutBitField(const FieldDecl *FD) {
     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) {
@@ -2741,12 +2742,17 @@ void MicrosoftRecordLayoutBuilder::injectVBPtr(const CXXRecordDecl *RD) {
   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)
@@ -2771,8 +2777,14 @@ void MicrosoftRecordLayoutBuilder::injectVFPtr(const CXXRecordDecl *RD) {
   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;
 
diff --git a/clang/test/CodeGenCXX/Inputs/override-bit-field-layout.layout b/clang/test/CodeGenCXX/Inputs/override-bit-field-layout.layout
index 8e67dce6502..b57c6efbc13 100644
--- a/clang/test/CodeGenCXX/Inputs/override-bit-field-layout.layout
+++ b/clang/test/CodeGenCXX/Inputs/override-bit-field-layout.layout
@@ -14,3 +14,11 @@ Layout: <ASTRecordLayout
   Size:128
   Alignment:64
   FieldOffsets: [64]>
+
+*** Dumping AST Record Layout
+Type: struct S3
+
+Layout: <ASTRecordLayout
+  Size:32
+  Alignment:32
+  FieldOffsets: [0, 1]>
diff --git a/clang/test/CodeGenCXX/Inputs/override-layout-virtual-base.layout b/clang/test/CodeGenCXX/Inputs/override-layout-virtual-base.layout
new file mode 100644
index 00000000000..71d88c1e603
--- /dev/null
+++ b/clang/test/CodeGenCXX/Inputs/override-layout-virtual-base.layout
@@ -0,0 +1,8 @@
+
+*** Dumping AST Record Layout
+Type: struct S2
+
+Layout: <ASTRecordLayout
+  Size:64
+  Alignment:64
+  FieldOffsets: []>
diff --git a/clang/test/CodeGenCXX/override-bit-field-layout.cpp b/clang/test/CodeGenCXX/override-bit-field-layout.cpp
index e84fcb0f450..dee7944f6a5 100644
--- a/clang/test/CodeGenCXX/override-bit-field-layout.cpp
+++ b/clang/test/CodeGenCXX/override-bit-field-layout.cpp
@@ -1,4 +1,4 @@
-// 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]
@@ -14,7 +14,23 @@ struct S2 {
   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)];
 }
diff --git a/clang/test/CodeGenCXX/override-layout-virtual-base.cpp b/clang/test/CodeGenCXX/override-layout-virtual-base.cpp
new file mode 100644
index 00000000000..d9e7346737d
--- /dev/null
+++ b/clang/test/CodeGenCXX/override-layout-virtual-base.cpp
@@ -0,0 +1,21 @@
+// 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)];
+}
diff --git a/clang/test/CodeGenCXX/override-layout.cpp b/clang/test/CodeGenCXX/override-layout.cpp
index a3c4bb446ca..fea2a45c629 100644
--- a/clang/test/CodeGenCXX/override-layout.cpp
+++ b/clang/test/CodeGenCXX/override-layout.cpp
@@ -64,6 +64,23 @@ struct PACKED X5 {
   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)];
@@ -71,7 +88,9 @@ void use_structs() {
   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;
 }
-