Skip to content

Commit 5d54213

Browse files
ZequanWuZequanWu
committed
[Clang][MS] Remove assertion on BaseOffset can't be smaller than Size.
This assertion triggered when we have two base classes sharing the same offset and the first base is empty and the second class is non-empty. Remove it for correctness. I can't add a test case for this because -foverride-record-layout doesn't read base class info at all. I can add that support later for testing if needed. Reviewed By: rnk Differential Revision: https://reviews.llvm.org/D152472
1 parent aa49521 commit 5d54213

File tree

5 files changed

+201
-25
lines changed

5 files changed

+201
-25
lines changed

‎clang/include/clang/Frontend/LayoutOverrideSource.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,12 @@ namespace clang {
3030
/// The alignment of the record.
3131
uint64_t Align;
3232

33+
/// The offsets of non-virtual base classes in the record.
34+
SmallVector<CharUnits, 8> BaseOffsets;
35+
36+
/// The offsets of virtual base classes in the record.
37+
SmallVector<CharUnits, 8> VBaseOffsets;
38+
3339
/// The offsets of the fields, in source order.
3440
SmallVector<uint64_t, 8> FieldOffsets;
3541
};

‎clang/lib/AST/RecordLayoutBuilder.cpp

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2926,8 +2926,7 @@ void MicrosoftRecordLayoutBuilder::layoutNonVirtualBase(
29262926
bool FoundBase = false;
29272927
if (UseExternalLayout) {
29282928
FoundBase = External.getExternalNVBaseOffset(BaseDecl, BaseOffset);
2929-
if (FoundBase) {
2930-
assert(BaseOffset >= Size && "base offset already allocated");
2929+
if (BaseOffset > Size) {
29312930
Size = BaseOffset;
29322931
}
29332932
}
@@ -3723,6 +3722,28 @@ void ASTContext::DumpRecordLayout(const RecordDecl *RD, raw_ostream &OS,
37233722
if (Target->defaultsToAIXPowerAlignment())
37243723
OS << " PreferredAlignment:" << toBits(Info.getPreferredAlignment())
37253724
<< "\n";
3725+
if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
3726+
OS << " BaseOffsets: [";
3727+
const CXXRecordDecl *Base = nullptr;
3728+
for (auto I : CXXRD->bases()) {
3729+
if (I.isVirtual())
3730+
continue;
3731+
if (Base)
3732+
OS << ", ";
3733+
Base = I.getType()->getAsCXXRecordDecl();
3734+
OS << Info.CXXInfo->BaseOffsets[Base].getQuantity();
3735+
}
3736+
OS << "]>\n";
3737+
OS << " VBaseOffsets: [";
3738+
const CXXRecordDecl *VBase = nullptr;
3739+
for (auto I : CXXRD->vbases()) {
3740+
if (VBase)
3741+
OS << ", ";
3742+
VBase = I.getType()->getAsCXXRecordDecl();
3743+
OS << Info.CXXInfo->VBaseOffsets[VBase].VBaseOffset.getQuantity();
3744+
}
3745+
OS << "]>\n";
3746+
}
37263747
OS << " FieldOffsets: [";
37273748
for (unsigned i = 0, e = Info.getFieldCount(); i != e; ++i) {
37283749
if (i)

‎clang/lib/Frontend/LayoutOverrideSource.cpp

Lines changed: 80 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
//===----------------------------------------------------------------------===//
88
#include "clang/Frontend/LayoutOverrideSource.h"
99
#include "clang/AST/Decl.h"
10+
#include "clang/AST/DeclCXX.h"
1011
#include "clang/Basic/CharInfo.h"
1112
#include "llvm/Support/raw_ostream.h"
1213
#include <fstream>
@@ -26,6 +27,18 @@ static std::string parseName(StringRef S) {
2627
return S.substr(0, Offset).str();
2728
}
2829

30+
/// Parse an unsigned integer and move S to the next non-digit character.
31+
static bool parseUnsigned(StringRef &S, unsigned long long &ULL) {
32+
if (S.empty() || !isDigit(S[0]))
33+
return false;
34+
unsigned Idx = 1;
35+
while (Idx < S.size() && isDigit(S[Idx]))
36+
++Idx;
37+
(void)S.substr(0, Idx).getAsInteger(10, ULL);
38+
S = S.substr(Idx);
39+
return true;
40+
}
41+
2942
LayoutOverrideSource::LayoutOverrideSource(StringRef Filename) {
3043
std::ifstream Input(Filename.str().c_str());
3144
if (!Input.is_open())
@@ -80,8 +93,8 @@ LayoutOverrideSource::LayoutOverrideSource(StringRef Filename) {
8093
LineStr = LineStr.substr(Pos + strlen(" Size:"));
8194

8295
unsigned long long Size = 0;
83-
(void)LineStr.getAsInteger(10, Size);
84-
CurrentLayout.Size = Size;
96+
if (parseUnsigned(LineStr, Size))
97+
CurrentLayout.Size = Size;
8598
continue;
8699
}
87100

@@ -92,21 +105,22 @@ LayoutOverrideSource::LayoutOverrideSource(StringRef Filename) {
92105
LineStr = LineStr.substr(Pos + strlen("Alignment:"));
93106

94107
unsigned long long Alignment = 0;
95-
(void)LineStr.getAsInteger(10, Alignment);
96-
CurrentLayout.Align = Alignment;
108+
if (parseUnsigned(LineStr, Alignment))
109+
CurrentLayout.Align = Alignment;
97110
continue;
98111
}
99112

100-
// Check for the size/alignment of the type.
113+
// Check for the size/alignment of the type. The number follows "size=" or
114+
// "align=" indicates number of bytes.
101115
Pos = LineStr.find("sizeof=");
102116
if (Pos != StringRef::npos) {
103117
/* Skip past the sizeof= prefix. */
104118
LineStr = LineStr.substr(Pos + strlen("sizeof="));
105119

106120
// Parse size.
107121
unsigned long long Size = 0;
108-
(void)LineStr.getAsInteger(10, Size);
109-
CurrentLayout.Size = Size;
122+
if (parseUnsigned(LineStr, Size))
123+
CurrentLayout.Size = Size * 8;
110124

111125
Pos = LineStr.find("align=");
112126
if (Pos != StringRef::npos) {
@@ -115,34 +129,59 @@ LayoutOverrideSource::LayoutOverrideSource(StringRef Filename) {
115129

116130
// Parse alignment.
117131
unsigned long long Alignment = 0;
118-
(void)LineStr.getAsInteger(10, Alignment);
119-
CurrentLayout.Align = Alignment;
132+
if (parseUnsigned(LineStr, Alignment))
133+
CurrentLayout.Align = Alignment * 8;
120134
}
121135

122136
continue;
123137
}
124138

125139
// Check for the field offsets of the type.
126140
Pos = LineStr.find("FieldOffsets: [");
127-
if (Pos == StringRef::npos)
128-
continue;
141+
if (Pos != StringRef::npos) {
142+
LineStr = LineStr.substr(Pos + strlen("FieldOffsets: ["));
143+
while (!LineStr.empty() && isDigit(LineStr[0])) {
144+
unsigned long long Offset = 0;
145+
if (parseUnsigned(LineStr, Offset))
146+
CurrentLayout.FieldOffsets.push_back(Offset);
147+
148+
// Skip over this offset, the following comma, and any spaces.
149+
LineStr = LineStr.substr(1);
150+
while (!LineStr.empty() && isWhitespace(LineStr[0]))
151+
LineStr = LineStr.substr(1);
152+
}
153+
}
129154

130-
LineStr = LineStr.substr(Pos + strlen("FieldOffsets: ["));
131-
while (!LineStr.empty() && isDigit(LineStr[0])) {
132-
// Parse this offset.
133-
unsigned Idx = 1;
134-
while (Idx < LineStr.size() && isDigit(LineStr[Idx]))
135-
++Idx;
155+
// Check for the base offsets.
156+
Pos = LineStr.find("BaseOffsets: [");
157+
if (Pos != StringRef::npos) {
158+
LineStr = LineStr.substr(Pos + strlen("BaseOffsets: ["));
159+
while (!LineStr.empty() && isDigit(LineStr[0])) {
160+
unsigned long long Offset = 0;
161+
if (parseUnsigned(LineStr, Offset))
162+
CurrentLayout.BaseOffsets.push_back(CharUnits::fromQuantity(Offset));
136163

137-
unsigned long long Offset = 0;
138-
(void)LineStr.substr(0, Idx).getAsInteger(10, Offset);
164+
// Skip over this offset, the following comma, and any spaces.
165+
LineStr = LineStr.substr(1);
166+
while (!LineStr.empty() && isWhitespace(LineStr[0]))
167+
LineStr = LineStr.substr(1);
168+
}
169+
}
139170

140-
CurrentLayout.FieldOffsets.push_back(Offset);
171+
// Check for the virtual base offsets.
172+
Pos = LineStr.find("VBaseOffsets: [");
173+
if (Pos != StringRef::npos) {
174+
LineStr = LineStr.substr(Pos + strlen("VBaseOffsets: ["));
175+
while (!LineStr.empty() && isDigit(LineStr[0])) {
176+
unsigned long long Offset = 0;
177+
if (parseUnsigned(LineStr, Offset))
178+
CurrentLayout.VBaseOffsets.push_back(CharUnits::fromQuantity(Offset));
141179

142-
// Skip over this offset, the following comma, and any spaces.
143-
LineStr = LineStr.substr(Idx + 1);
144-
while (!LineStr.empty() && isWhitespace(LineStr[0]))
180+
// Skip over this offset, the following comma, and any spaces.
145181
LineStr = LineStr.substr(1);
182+
while (!LineStr.empty() && isWhitespace(LineStr[0]))
183+
LineStr = LineStr.substr(1);
184+
}
146185
}
147186
}
148187

@@ -182,6 +221,24 @@ LayoutOverrideSource::layoutRecordType(const RecordDecl *Record,
182221
if (NumFields != Known->second.FieldOffsets.size())
183222
return false;
184223

224+
// Provide base offsets.
225+
if (const auto *RD = dyn_cast<CXXRecordDecl>(Record)) {
226+
unsigned NumNB = 0;
227+
unsigned NumVB = 0;
228+
for (const auto &I : RD->vbases()) {
229+
if (NumVB >= Known->second.VBaseOffsets.size())
230+
continue;
231+
const CXXRecordDecl *VBase = I.getType()->getAsCXXRecordDecl();
232+
VirtualBaseOffsets[VBase] = Known->second.VBaseOffsets[NumVB++];
233+
}
234+
for (const auto &I : RD->bases()) {
235+
if (I.isVirtual() || NumNB >= Known->second.BaseOffsets.size())
236+
continue;
237+
const CXXRecordDecl *Base = I.getType()->getAsCXXRecordDecl();
238+
BaseOffsets[Base] = Known->second.BaseOffsets[NumNB++];
239+
}
240+
}
241+
185242
Size = Known->second.Size;
186243
Alignment = Known->second.Align;
187244
return true;

‎clang/test/CodeGenCXX/Inputs/override-layout-ms.layout

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
*** Dumping AST Record Layout
2+
Type: struct E1
3+
4+
Layout: <ASTRecordLayout
5+
Size:8
6+
Alignment:8
7+
BaseOffsets: []>
8+
VBaseOffsets: []>
9+
FieldOffsets: []>
10+
11+
*** Dumping AST Record Layout
12+
Type: struct Mid
13+
14+
Layout: <ASTRecordLayout
15+
Size:64
16+
Alignment:64
17+
BaseOffsets: []>
18+
VBaseOffsets: []>
19+
FieldOffsets: [0]>
20+
21+
*** Dumping AST Record Layout
22+
Type: struct E2
23+
24+
Layout: <ASTRecordLayout
25+
Size:8
26+
Alignment:8
27+
BaseOffsets: []>
28+
VBaseOffsets: []>
29+
FieldOffsets: []>
30+
31+
*** Dumping AST Record Layout
32+
Type: struct Combine
33+
34+
Layout: <ASTRecordLayout
35+
Size:64
36+
Alignment:64
37+
BaseOffsets: [0, 0, 0]>
38+
VBaseOffsets: []>
39+
FieldOffsets: []>
40+
41+
*** Dumping AST Record Layout
42+
Type: struct Combine2
43+
44+
Layout: <ASTRecordLayout
45+
Size:128
46+
Alignment:64
47+
BaseOffsets: [0, 8]>
48+
VBaseOffsets: []>
49+
FieldOffsets: []>

‎clang/test/CodeGenCXX/override-layout-ms.cpp

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// RUN: %clang_cc1 -w -triple=x86_64-pc-win32 -fms-compatibility -fdump-record-layouts -foverride-record-layout=%S/Inputs/override-layout-ms.layout %s | FileCheck %s
2+
// RUN: %clang_cc1 -w -triple=x86_64-pc-win32 -fms-compatibility -fdump-record-layouts %s | FileCheck %s
3+
4+
// CHECK: *** Dumping AST Record Layout
5+
// CHECK: 0 | struct E1 (empty)
6+
// CHECK: | [sizeof=1, align=1,
7+
// CHECK: | nvsize=0, nvalign=1]
8+
// CHECK: *** Dumping AST Record Layout
9+
// CHECK: 0 | struct Mid
10+
// CHECK: 0 | void * p
11+
// CHECK: | [sizeof=8, align=8,
12+
// CHECK: | nvsize=8, nvalign=8]
13+
// CHECK: *** Dumping AST Record Layout
14+
// CHECK: 0 | struct E2 (empty)
15+
// CHECK: | [sizeof=1, align=1,
16+
// CHECK: | nvsize=0, nvalign=1]
17+
// CHECK: *** Dumping AST Record Layout
18+
// CHECK: 0 | struct Combine
19+
// CHECK: 0 | struct E1 (base) (empty)
20+
// CHECK: 0 | struct Mid (base)
21+
// CHECK: 0 | void * p
22+
// CHECK: 0 | struct E2 (base) (empty)
23+
// CHECK: | [sizeof=8, align=8,
24+
// CHECK: | nvsize=8, nvalign=8]
25+
// CHECK: *** Dumping AST Record Layout
26+
// CHECK: 0 | struct Combine2
27+
// CHECK: 0 | struct VB1 (primary base)
28+
// CHECK: 0 | (VB1 vftable pointer)
29+
// CHECK: 8 | struct VB2 (base)
30+
// CHECK: 8 | (VB2 vftable pointer)
31+
// CHECK: | [sizeof=16, align=8,
32+
// CHECK: | nvsize=16, nvalign=8]
33+
34+
35+
struct E1 {};
36+
struct E2 {};
37+
struct Mid {void *p; };
38+
struct __declspec(empty_bases) Combine : E1, Mid, E2 {};
39+
struct VB1 { virtual void foo() {}};
40+
struct VB2 { virtual void bar() {}};
41+
struct Combine2: VB1, VB2 {};
42+
Combine g;
43+
Combine2 f;

0 commit comments

Comments
 (0)