aleksandr.urakov created this revision.
aleksandr.urakov added reviewers: rnk, rsmith, zturner, mstorsjo, majnemer.
aleksandr.urakov added a project: clang.
Herald added a subscriber: cfe-commits.
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 https://reviews.llvm.org/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?
Repository:
rC Clang
https://reviews.llvm.org/D53497
Files:
lib/AST/RecordLayoutBuilder.cpp
test/CodeGenCXX/Inputs/override-layout-packed-base.layout
test/CodeGenCXX/override-layout-packed-base.cpp
Index: test/CodeGenCXX/override-layout-packed-base.cpp
===================================================================
--- test/CodeGenCXX/override-layout-packed-base.cpp
+++ test/CodeGenCXX/override-layout-packed-base.cpp
@@ -1,26 +1,40 @@
-// 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)];
}
Index: test/CodeGenCXX/Inputs/override-layout-packed-base.layout
===================================================================
--- test/CodeGenCXX/Inputs/override-layout-packed-base.layout
+++ test/CodeGenCXX/Inputs/override-layout-packed-base.layout
@@ -3,16 +3,26 @@
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]>
Index: lib/AST/RecordLayoutBuilder.cpp
===================================================================
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -2829,15 +2829,14 @@
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();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
