Skip to content

[LLDB][NativePDB] Add non-overlapping fields in root struct #166243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 5, 2025
Merged

[LLDB][NativePDB] Add non-overlapping fields in root struct #166243

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
22628e5
[LLDB][NativePDB] Add non-overlapping fields in root struct
Nerixyz Nov 3, 2025
fc1478f
fix: class layout test
Nerixyz Nov 4, 2025
5643c37
fix: move `parent` assignment to else branch
Nerixyz Nov 4, 2025
2e0b691
fix: test name
Nerixyz Nov 4, 2025
8207a5f
fix: handle unions as well
Nerixyz Nov 4, 2025
8135db8
fix: parentheses and assert
Nerixyz Nov 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,10 @@ void UdtRecordCompleter::Record::ConstructRecord() {

// The end offset to a vector of field/struct that ends at the offset.
std::map<uint64_t, std::vector<Member *>> end_offset_map;
auto is_last_end_offset = [&](auto it) {
return it != end_offset_map.end() && ++it == end_offset_map.end();
};

for (auto &pair : fields_map) {
uint64_t offset = pair.first;
auto &fields = pair.second;
Expand All @@ -462,8 +466,23 @@ void UdtRecordCompleter::Record::ConstructRecord() {
}
if (iter->second.empty())
continue;
parent = iter->second.back();
iter->second.pop_back();

// If the new fields come after the already added ones
// without overlap, go back to the root.
if (iter->first <= offset && is_last_end_offset(iter)) {
if (record.kind == Member::Struct)
parent = &record;
else {
lldbassert(record.kind == Member::Union &&
"Current record must be a union");
lldbassert(!record.fields.empty());
Copy link
Member

@JDevlieghere JDevlieghere Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per https://lldb.llvm.org/resources/contributing.html#error-handling, we'd rather not add new uses of lldbassert. If these are proper preconditions, they can be converted to regular asserts.

Copy link
Contributor Author

@Nerixyz Nerixyz Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These asserts should fall under

An assertion should be placed to assert invariants that the developer is convinced will always hold, regardless what an end-user does with LLDB.

The first assert holds because we only ever get here for unions and structs. The fields should never be empty, because at this point, the offset is larger than the starting one / if the fields were empty, offset == start_offset.

// For unions, append the field to the last struct
parent = record.fields.back().get();
}
} else {
parent = iter->second.back();
iter->second.pop_back();
}
}
// If it's a field, then the field is inside a union, so we can safely
// increase its size by converting it to a struct to hold multiple fields.
Expand Down
8 changes: 4 additions & 4 deletions lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,6 @@
// CHECK-NEXT: s4 = {
// CHECK-NEXT: x = ([0] = 67, [1] = 68, [2] = 99)
// CHECK-NEXT: }
// CHECK-NEXT: s1 = {
// CHECK-NEXT: x = ([0] = 69, [1] = 70, [2] = 71)
// CHECK-NEXT: }
// CHECK-NEXT: }
// CHECK-NEXT: }
// CHECK-NEXT: }
Expand All @@ -47,6 +44,9 @@
// CHECK-NEXT: c2 = 'D'
// CHECK-NEXT: }
// CHECK-NEXT: }
// CHECK-NEXT: s1 = {
// CHECK-NEXT: x = ([0] = 69, [1] = 70, [2] = 71)
// CHECK-NEXT: }
// CHECK-NEXT: }
// CHECK-NEXT: (lldb) type lookup C
// CHECK-NEXT: struct C {
Expand All @@ -63,7 +63,6 @@
// CHECK-NEXT: struct {
// CHECK-NEXT: char c4;
// CHECK-NEXT: S3 s4;
// CHECK-NEXT: S3 s1;
// CHECK-NEXT: };
// CHECK-NEXT: };
// CHECK-NEXT: };
Expand All @@ -72,6 +71,7 @@
// CHECK-NEXT: char c2;
// CHECK-NEXT: };
// CHECK-NEXT: };
// CHECK-NEXT: S3 s1;
// CHECK-NEXT: }


Expand Down
52 changes: 51 additions & 1 deletion lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ Member *AddField(Member *member, StringRef name, uint64_t byte_offset,
std::make_unique<Member>(name, byte_offset * 8, byte_size * 8,
clang::QualType(), lldb::eAccessPublic, 0);
field->kind = kind;
field->base_offset = base_offset;
field->base_offset = base_offset * 8;
member->fields.push_back(std::move(field));
return member->fields.back().get();
}
Expand All @@ -111,6 +111,9 @@ TEST_F(UdtRecordCompleterRecordTests, TestAnonymousUnionInStruct) {
CollectMember("m2", 0, 4);
CollectMember("m3", 0, 1);
CollectMember("m4", 0, 8);
CollectMember("m5", 8, 8);
CollectMember("m6", 16, 4);
CollectMember("m7", 16, 8);
ConstructRecord();

// struct {
Expand All @@ -120,6 +123,11 @@ TEST_F(UdtRecordCompleterRecordTests, TestAnonymousUnionInStruct) {
// m3;
// m4;
// };
// m5;
// union {
// m6;
// m7;
// };
// };
Record record;
record.start_offset = 0;
Expand All @@ -128,6 +136,10 @@ TEST_F(UdtRecordCompleterRecordTests, TestAnonymousUnionInStruct) {
AddField(u, "m2", 0, 4, Member::Field);
AddField(u, "m3", 0, 1, Member::Field);
AddField(u, "m4", 0, 8, Member::Field);
AddField(&record.record, "m5", 8, 8, Member::Field);
Member *u2 = AddField(&record.record, "", 16, 0, Member::Union);
AddField(u2, "m6", 16, 4, Member::Field);
AddField(u2, "m7", 16, 8, Member::Field);
EXPECT_EQ(WrappedRecord(this->record), WrappedRecord(record));
}

Expand Down Expand Up @@ -243,3 +255,41 @@ TEST_F(UdtRecordCompleterRecordTests, TestNestedUnionStructInUnion) {
AddField(s2, "m4", 2, 4, Member::Field);
EXPECT_EQ(WrappedRecord(this->record), WrappedRecord(record));
}

TEST_F(UdtRecordCompleterRecordTests, TestNestedStructInUnionInStructInUnion) {
SetKind(Member::Kind::Union);
Copy link
Contributor

@ZequanWu ZequanWu Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this testing this change? This change only changed the behaviour when the root record is a struct.

Copy link
Contributor Author

@Nerixyz Nerixyz Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's testing that we don't do this "going up one level" everywhere. In this test, m7 does not overlap m5, so it could be placed in the outer struct:

union {
  m1;
  m2;
  struct {
      m3;
      m4;
      union {
          m5;
          m6;
      };
      m7;
  };
};

Copy link
Contributor

@ZequanWu ZequanWu Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't putting m7 into the struct, where m3 and m4 defined, better than making m6 and m7 into a new struct since it's one less layer?

Copy link
Contributor Author

@Nerixyz Nerixyz Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I wanted to capture the current state with the test. But now that I think about it, we could do something similar for unions...

Copy link
Contributor Author

@Nerixyz Nerixyz Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this.

CollectMember("m1", 0, 4);
CollectMember("m2", 0, 2);
CollectMember("m3", 0, 2);
CollectMember("m4", 2, 4);
CollectMember("m5", 6, 2);
CollectMember("m6", 6, 2);
CollectMember("m7", 8, 2);
ConstructRecord();

// union {
// m1;
// m2;
// struct {
// m3;
// m4;
// union {
// m5;
// m6;
// };
// m7;
// };
// };
Record record;
record.start_offset = 0;
AddField(&record.record, "m1", 0, 4, Member::Field);
AddField(&record.record, "m2", 0, 2, Member::Field);
Member *s = AddField(&record.record, "", 0, 0, Member::Struct);
AddField(s, "m3", 0, 2, Member::Field);
AddField(s, "m4", 2, 4, Member::Field);
Member *u = AddField(s, "", 6, 0, Member::Union);
AddField(u, "m5", 6, 2, Member::Field);
AddField(u, "m6", 6, 2, Member::Field);
AddField(s, "m7", 8, 2, Member::Field);
EXPECT_EQ(WrappedRecord(this->record), WrappedRecord(record));
}
Loading