[clang] 5d54213 - [Clang][MS] Remove assertion on BaseOffset can't be smaller than Size.
Zequan Wu via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 14 13:48:32 PDT 2023
Author: Zequan Wu
Date: 2023-06-14T16:48:26-04:00
New Revision: 5d54213ee557a86fae82af0f75498adf02f24e82
URL: https://github.com/llvm/llvm-project/commit/5d54213ee557a86fae82af0f75498adf02f24e82
DIFF: https://github.com/llvm/llvm-project/commit/5d54213ee557a86fae82af0f75498adf02f24e82.diff
LOG: [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
Added:
clang/test/CodeGenCXX/Inputs/override-layout-ms.layout
clang/test/CodeGenCXX/override-layout-ms.cpp
Modified:
clang/include/clang/Frontend/LayoutOverrideSource.h
clang/lib/AST/RecordLayoutBuilder.cpp
clang/lib/Frontend/LayoutOverrideSource.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Frontend/LayoutOverrideSource.h b/clang/include/clang/Frontend/LayoutOverrideSource.h
index ea1611470a76a..c6e2d73111833 100644
--- a/clang/include/clang/Frontend/LayoutOverrideSource.h
+++ b/clang/include/clang/Frontend/LayoutOverrideSource.h
@@ -30,6 +30,12 @@ namespace clang {
/// The alignment of the record.
uint64_t Align;
+ /// The offsets of non-virtual base classes in the record.
+ SmallVector<CharUnits, 8> BaseOffsets;
+
+ /// The offsets of virtual base classes in the record.
+ SmallVector<CharUnits, 8> VBaseOffsets;
+
/// The offsets of the fields, in source order.
SmallVector<uint64_t, 8> FieldOffsets;
};
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index aca50912dceac..3f836cb96be57 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2926,8 +2926,7 @@ void MicrosoftRecordLayoutBuilder::layoutNonVirtualBase(
bool FoundBase = false;
if (UseExternalLayout) {
FoundBase = External.getExternalNVBaseOffset(BaseDecl, BaseOffset);
- if (FoundBase) {
- assert(BaseOffset >= Size && "base offset already allocated");
+ if (BaseOffset > Size) {
Size = BaseOffset;
}
}
@@ -3723,6 +3722,28 @@ void ASTContext::DumpRecordLayout(const RecordDecl *RD, raw_ostream &OS,
if (Target->defaultsToAIXPowerAlignment())
OS << " PreferredAlignment:" << toBits(Info.getPreferredAlignment())
<< "\n";
+ if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
+ OS << " BaseOffsets: [";
+ const CXXRecordDecl *Base = nullptr;
+ for (auto I : CXXRD->bases()) {
+ if (I.isVirtual())
+ continue;
+ if (Base)
+ OS << ", ";
+ Base = I.getType()->getAsCXXRecordDecl();
+ OS << Info.CXXInfo->BaseOffsets[Base].getQuantity();
+ }
+ OS << "]>\n";
+ OS << " VBaseOffsets: [";
+ const CXXRecordDecl *VBase = nullptr;
+ for (auto I : CXXRD->vbases()) {
+ if (VBase)
+ OS << ", ";
+ VBase = I.getType()->getAsCXXRecordDecl();
+ OS << Info.CXXInfo->VBaseOffsets[VBase].VBaseOffset.getQuantity();
+ }
+ OS << "]>\n";
+ }
OS << " FieldOffsets: [";
for (unsigned i = 0, e = Info.getFieldCount(); i != e; ++i) {
if (i)
diff --git a/clang/lib/Frontend/LayoutOverrideSource.cpp b/clang/lib/Frontend/LayoutOverrideSource.cpp
index 0d288db0632fd..f09444deb8d38 100644
--- a/clang/lib/Frontend/LayoutOverrideSource.cpp
+++ b/clang/lib/Frontend/LayoutOverrideSource.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "clang/Frontend/LayoutOverrideSource.h"
#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
#include "clang/Basic/CharInfo.h"
#include "llvm/Support/raw_ostream.h"
#include <fstream>
@@ -26,6 +27,18 @@ static std::string parseName(StringRef S) {
return S.substr(0, Offset).str();
}
+/// Parse an unsigned integer and move S to the next non-digit character.
+static bool parseUnsigned(StringRef &S, unsigned long long &ULL) {
+ if (S.empty() || !isDigit(S[0]))
+ return false;
+ unsigned Idx = 1;
+ while (Idx < S.size() && isDigit(S[Idx]))
+ ++Idx;
+ (void)S.substr(0, Idx).getAsInteger(10, ULL);
+ S = S.substr(Idx);
+ return true;
+}
+
LayoutOverrideSource::LayoutOverrideSource(StringRef Filename) {
std::ifstream Input(Filename.str().c_str());
if (!Input.is_open())
@@ -80,8 +93,8 @@ LayoutOverrideSource::LayoutOverrideSource(StringRef Filename) {
LineStr = LineStr.substr(Pos + strlen(" Size:"));
unsigned long long Size = 0;
- (void)LineStr.getAsInteger(10, Size);
- CurrentLayout.Size = Size;
+ if (parseUnsigned(LineStr, Size))
+ CurrentLayout.Size = Size;
continue;
}
@@ -92,12 +105,13 @@ LayoutOverrideSource::LayoutOverrideSource(StringRef Filename) {
LineStr = LineStr.substr(Pos + strlen("Alignment:"));
unsigned long long Alignment = 0;
- (void)LineStr.getAsInteger(10, Alignment);
- CurrentLayout.Align = Alignment;
+ if (parseUnsigned(LineStr, Alignment))
+ CurrentLayout.Align = Alignment;
continue;
}
- // Check for the size/alignment of the type.
+ // Check for the size/alignment of the type. The number follows "size=" or
+ // "align=" indicates number of bytes.
Pos = LineStr.find("sizeof=");
if (Pos != StringRef::npos) {
/* Skip past the sizeof= prefix. */
@@ -105,8 +119,8 @@ LayoutOverrideSource::LayoutOverrideSource(StringRef Filename) {
// Parse size.
unsigned long long Size = 0;
- (void)LineStr.getAsInteger(10, Size);
- CurrentLayout.Size = Size;
+ if (parseUnsigned(LineStr, Size))
+ CurrentLayout.Size = Size * 8;
Pos = LineStr.find("align=");
if (Pos != StringRef::npos) {
@@ -115,8 +129,8 @@ LayoutOverrideSource::LayoutOverrideSource(StringRef Filename) {
// Parse alignment.
unsigned long long Alignment = 0;
- (void)LineStr.getAsInteger(10, Alignment);
- CurrentLayout.Align = Alignment;
+ if (parseUnsigned(LineStr, Alignment))
+ CurrentLayout.Align = Alignment * 8;
}
continue;
@@ -124,25 +138,50 @@ LayoutOverrideSource::LayoutOverrideSource(StringRef Filename) {
// Check for the field offsets of the type.
Pos = LineStr.find("FieldOffsets: [");
- if (Pos == StringRef::npos)
- continue;
+ if (Pos != StringRef::npos) {
+ LineStr = LineStr.substr(Pos + strlen("FieldOffsets: ["));
+ while (!LineStr.empty() && isDigit(LineStr[0])) {
+ unsigned long long Offset = 0;
+ if (parseUnsigned(LineStr, Offset))
+ CurrentLayout.FieldOffsets.push_back(Offset);
+
+ // Skip over this offset, the following comma, and any spaces.
+ LineStr = LineStr.substr(1);
+ while (!LineStr.empty() && isWhitespace(LineStr[0]))
+ LineStr = LineStr.substr(1);
+ }
+ }
- LineStr = LineStr.substr(Pos + strlen("FieldOffsets: ["));
- while (!LineStr.empty() && isDigit(LineStr[0])) {
- // Parse this offset.
- unsigned Idx = 1;
- while (Idx < LineStr.size() && isDigit(LineStr[Idx]))
- ++Idx;
+ // Check for the base offsets.
+ Pos = LineStr.find("BaseOffsets: [");
+ if (Pos != StringRef::npos) {
+ LineStr = LineStr.substr(Pos + strlen("BaseOffsets: ["));
+ while (!LineStr.empty() && isDigit(LineStr[0])) {
+ unsigned long long Offset = 0;
+ if (parseUnsigned(LineStr, Offset))
+ CurrentLayout.BaseOffsets.push_back(CharUnits::fromQuantity(Offset));
- unsigned long long Offset = 0;
- (void)LineStr.substr(0, Idx).getAsInteger(10, Offset);
+ // Skip over this offset, the following comma, and any spaces.
+ LineStr = LineStr.substr(1);
+ while (!LineStr.empty() && isWhitespace(LineStr[0]))
+ LineStr = LineStr.substr(1);
+ }
+ }
- CurrentLayout.FieldOffsets.push_back(Offset);
+ // Check for the virtual base offsets.
+ Pos = LineStr.find("VBaseOffsets: [");
+ if (Pos != StringRef::npos) {
+ LineStr = LineStr.substr(Pos + strlen("VBaseOffsets: ["));
+ while (!LineStr.empty() && isDigit(LineStr[0])) {
+ unsigned long long Offset = 0;
+ if (parseUnsigned(LineStr, Offset))
+ CurrentLayout.VBaseOffsets.push_back(CharUnits::fromQuantity(Offset));
- // Skip over this offset, the following comma, and any spaces.
- LineStr = LineStr.substr(Idx + 1);
- while (!LineStr.empty() && isWhitespace(LineStr[0]))
+ // Skip over this offset, the following comma, and any spaces.
LineStr = LineStr.substr(1);
+ while (!LineStr.empty() && isWhitespace(LineStr[0]))
+ LineStr = LineStr.substr(1);
+ }
}
}
@@ -182,6 +221,24 @@ LayoutOverrideSource::layoutRecordType(const RecordDecl *Record,
if (NumFields != Known->second.FieldOffsets.size())
return false;
+ // Provide base offsets.
+ if (const auto *RD = dyn_cast<CXXRecordDecl>(Record)) {
+ unsigned NumNB = 0;
+ unsigned NumVB = 0;
+ for (const auto &I : RD->vbases()) {
+ if (NumVB >= Known->second.VBaseOffsets.size())
+ continue;
+ const CXXRecordDecl *VBase = I.getType()->getAsCXXRecordDecl();
+ VirtualBaseOffsets[VBase] = Known->second.VBaseOffsets[NumVB++];
+ }
+ for (const auto &I : RD->bases()) {
+ if (I.isVirtual() || NumNB >= Known->second.BaseOffsets.size())
+ continue;
+ const CXXRecordDecl *Base = I.getType()->getAsCXXRecordDecl();
+ BaseOffsets[Base] = Known->second.BaseOffsets[NumNB++];
+ }
+ }
+
Size = Known->second.Size;
Alignment = Known->second.Align;
return true;
diff --git a/clang/test/CodeGenCXX/Inputs/override-layout-ms.layout b/clang/test/CodeGenCXX/Inputs/override-layout-ms.layout
new file mode 100644
index 0000000000000..27790e8589c46
--- /dev/null
+++ b/clang/test/CodeGenCXX/Inputs/override-layout-ms.layout
@@ -0,0 +1,49 @@
+*** Dumping AST Record Layout
+Type: struct E1
+
+Layout: <ASTRecordLayout
+ Size:8
+ Alignment:8
+ BaseOffsets: []>
+ VBaseOffsets: []>
+ FieldOffsets: []>
+
+*** Dumping AST Record Layout
+Type: struct Mid
+
+Layout: <ASTRecordLayout
+ Size:64
+ Alignment:64
+ BaseOffsets: []>
+ VBaseOffsets: []>
+ FieldOffsets: [0]>
+
+*** Dumping AST Record Layout
+Type: struct E2
+
+Layout: <ASTRecordLayout
+ Size:8
+ Alignment:8
+ BaseOffsets: []>
+ VBaseOffsets: []>
+ FieldOffsets: []>
+
+*** Dumping AST Record Layout
+Type: struct Combine
+
+Layout: <ASTRecordLayout
+ Size:64
+ Alignment:64
+ BaseOffsets: [0, 0, 0]>
+ VBaseOffsets: []>
+ FieldOffsets: []>
+
+*** Dumping AST Record Layout
+Type: struct Combine2
+
+Layout: <ASTRecordLayout
+ Size:128
+ Alignment:64
+ BaseOffsets: [0, 8]>
+ VBaseOffsets: []>
+ FieldOffsets: []>
diff --git a/clang/test/CodeGenCXX/override-layout-ms.cpp b/clang/test/CodeGenCXX/override-layout-ms.cpp
new file mode 100644
index 0000000000000..1e678a4576a12
--- /dev/null
+++ b/clang/test/CodeGenCXX/override-layout-ms.cpp
@@ -0,0 +1,43 @@
+// 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
+// RUN: %clang_cc1 -w -triple=x86_64-pc-win32 -fms-compatibility -fdump-record-layouts %s | FileCheck %s
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK: 0 | struct E1 (empty)
+// CHECK: | [sizeof=1, align=1,
+// CHECK: | nvsize=0, nvalign=1]
+// CHECK: *** Dumping AST Record Layout
+// CHECK: 0 | struct Mid
+// CHECK: 0 | void * p
+// CHECK: | [sizeof=8, align=8,
+// CHECK: | nvsize=8, nvalign=8]
+// CHECK: *** Dumping AST Record Layout
+// CHECK: 0 | struct E2 (empty)
+// CHECK: | [sizeof=1, align=1,
+// CHECK: | nvsize=0, nvalign=1]
+// CHECK: *** Dumping AST Record Layout
+// CHECK: 0 | struct Combine
+// CHECK: 0 | struct E1 (base) (empty)
+// CHECK: 0 | struct Mid (base)
+// CHECK: 0 | void * p
+// CHECK: 0 | struct E2 (base) (empty)
+// CHECK: | [sizeof=8, align=8,
+// CHECK: | nvsize=8, nvalign=8]
+// CHECK: *** Dumping AST Record Layout
+// CHECK: 0 | struct Combine2
+// CHECK: 0 | struct VB1 (primary base)
+// CHECK: 0 | (VB1 vftable pointer)
+// CHECK: 8 | struct VB2 (base)
+// CHECK: 8 | (VB2 vftable pointer)
+// CHECK: | [sizeof=16, align=8,
+// CHECK: | nvsize=16, nvalign=8]
+
+
+struct E1 {};
+struct E2 {};
+struct Mid {void *p; };
+struct __declspec(empty_bases) Combine : E1, Mid, E2 {};
+struct VB1 { virtual void foo() {}};
+struct VB2 { virtual void bar() {}};
+struct Combine2: VB1, VB2 {};
+Combine g;
+Combine2 f;
\ No newline at end of file
More information about the cfe-commits
mailing list