[AST] Do not align virtual bases in `MicrosoftRecordLayoutBuilder` when an external layout is used

[aleksandr.urakov]

Summary

The patch removes alignment of virtual bases when an external layout is used. We have two cases:

• the external layout source has an information about virtual bases offsets, so we just use them;
• the external source has no information about virtual bases offsets. In this case we can't predict where the base will be located. If we will align it but there will be something like #pragma pack(push, 1) really, then likely our layout will not fit into the real structure size, and then some asserts will hit. The asserts look reasonable, so I don't think that we need to remove them. May be it would be better instead don't align fields / bases etc. (so treat it always as #pragma pack(push, 1)) when an external layout source is used but no info about a field location is presented.

This one is related to D49871

Also it seems that MicrosoftRecordLayoutBuilder with an external source and without one differ considerably, may be it is worth to split this and to create two different builders? I think that it would make the logic simpler. What do you think about it?

[rnk]

Also it seems that MicrosoftRecordLayoutBuilder with an external source and without one differ considerably, may be it is worth to split this and to create two different builders? I think that it would make the logic simpler. What do you think about it?

Yes, I recall when reading the Itanium record layout code that it was riddled with extra checks for external layouts. Back then I thought it would be nicer to have a separate code path for external layouts. I appreciated that when we separated out the MS code, it was clean and not filled with these checks. But, of course, now here we are adding them in. If you want to do the work to come up with a separate, more minimal code path that produces ASTRecordLayouts from external layouts, I'd definitely help review it.

This change is pretty small and targeted, so feel free to land it as is first. Thanks!

[aleksandr.urakov]

Thank you!

lib/AST/RecordLayoutBuilder.cpp

if ((PreviousBaseLayout && PreviousBaseLayout->endsWithZeroSizedObject() &&
       Size = Size.alignTo(VtorDispAlignment) + VtorDispSize;
       Alignment = std::max(VtorDispAlignment, Alignment);
     }
     // Insert the virtual base.
     ElementInfo Info = getAdjustedElementInfo(BaseLayout);
     CharUnits BaseOffset;
 
     // Respect the external AST source base offset, if present.
-    bool FoundBase = false;
     if (UseExternalLayout) {
-      FoundBase = External.getExternalVBaseOffset(BaseDecl, BaseOffset);
-      if (FoundBase)
-        assert(BaseOffset >= Size && "base offset already allocated");
-    }
-    if (!FoundBase)
+      if (!External.getExternalVBaseOffset(BaseDecl, BaseOffset))
+        BaseOffset = Size;
+    } else
       BaseOffset = Size.alignTo(Info.Alignment);
 
+    assert(BaseOffset >= Size && "base offset already allocated");
+
     VBases.insert(std::make_pair(BaseDecl,
         ASTRecordLayout::VBaseInfo(BaseOffset, HasVtordisp)));
     Size = BaseOffset + BaseLayout.getNonVirtualSize();
     PreviousBaseLayout = &BaseLayout;
   }
 }
 
 void MicrosoftRecordLayoutBuilder::finalizeLayout(const RecordDecl *RD) {

test/CodeGenCXX/Inputs/override-layout-packed-base.layout

 
 *** Dumping AST Record Layout
 Type: class B<0>
 
 Layout: <ASTRecordLayout
+  Size:40
   FieldOffsets: [0, 32]>
 
 *** Dumping AST Record Layout
 Type: class B<1>
 
 Layout: <ASTRecordLayout
+  Size:40
   FieldOffsets: [0, 32]>
 
 *** Dumping AST Record Layout
 Type: class C
 
 Layout: <ASTRecordLayout
+  Size:88
   FieldOffsets: [80]>
+
+*** Dumping AST Record Layout
+Type: class D
+
+Layout: <ASTRecordLayout
+  Size:120
+  FieldOffsets: [32]>

test/CodeGenCXX/override-layout-packed-base.cpp

-// RUN: %clang_cc1 -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-layout-packed-base.layout %s | FileCheck %s
+// RUN: %clang_cc1 -triple i686-windows-msvc -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-layout-packed-base.layout %s | FileCheck %s
+
+//#pragma pack(push, 1)
 
 // CHECK: Type: class B<0>
+// CHECK:   Size:40
 // CHECK:   FieldOffsets: [0, 32]
 
 // CHECK: Type: class B<1>
+// CHECK:   Size:40
 // CHECK:   FieldOffsets: [0, 32]
 
-//#pragma pack(push, 1)
 template<int I>
 class B {
   int _b1;
   char _b2;
 };
-//#pragma pack(pop)
 
 // CHECK: Type: class C
+// CHECK:   Size:88
 // CHECK:   FieldOffsets: [80]
 
 class C : B<0>, B<1> {
   char _c;
 };
 
+// CHECK: Type: class D
+// CHECK:   Size:120
+// CHECK:   FieldOffsets: [32]
+
+class D : virtual B<0>, virtual B<1> {
+  char _d;
+};
+
+//#pragma pack(pop)
+
 void use_structs() {
   C cs[sizeof(C)];
+  D ds[sizeof(D)];
 }