From 22628e5e4ea56a5e06c8550a66cb86d1470cba0b Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Mon, 3 Nov 2025 22:31:35 +0100 Subject: [PATCH 1/6] [LLDB][NativePDB] Add non-overlapping fields in root struct --- .../NativePDB/UdtRecordCompleter.cpp | 13 ++++- .../NativePDB/UdtRecordCompleterTests.cpp | 55 ++++++++++++++++++- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp b/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp index 1c575e90bd72c..1458079483278 100644 --- a/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp +++ b/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp @@ -442,6 +442,10 @@ void UdtRecordCompleter::Record::ConstructRecord() { // The end offset to a vector of field/struct that ends at the offset. std::map> 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; @@ -463,7 +467,14 @@ void UdtRecordCompleter::Record::ConstructRecord() { if (iter->second.empty()) continue; parent = iter->second.back(); - iter->second.pop_back(); + + // For structs, if the new fields come after the already added ones + // without overlap, go back to the root struct. + if (parent != &record && iter->first <= offset && + record.kind == Member::Struct && is_last_end_offset(iter)) + parent = &record; + else + 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. diff --git a/lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp b/lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp index 17284b61b9a6e..3432e03bbf853 100644 --- a/lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp +++ b/lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp @@ -99,7 +99,7 @@ Member *AddField(Member *member, StringRef name, uint64_t byte_offset, std::make_unique(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(); } @@ -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 { @@ -120,6 +123,11 @@ TEST_F(UdtRecordCompleterRecordTests, TestAnonymousUnionInStruct) { // m3; // m4; // }; + // m5; + // union { + // m6; + // m7; + // }; // }; Record record; record.start_offset = 0; @@ -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)); } @@ -243,3 +255,44 @@ TEST_F(UdtRecordCompleterRecordTests, TestNestedUnionStructInUnion) { AddField(s2, "m4", 2, 4, Member::Field); EXPECT_EQ(WrappedRecord(this->record), WrappedRecord(record)); } + +TEST_F(UdtRecordCompleterRecordTests, TestNestedUnionStructInUnion2) { + SetKind(Member::Kind::Union); + 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; + // struct { + // 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 *s1 = AddField(&record.record, "", 0, 0, Member::Struct); + AddField(s1, "m3", 0, 2, Member::Field); + AddField(s1, "m4", 2, 4, Member::Field); + Member *u1 = AddField(s1, "", 6, 0, Member::Union); + AddField(u1, "m5", 6, 2, Member::Field); + Member *s2 = AddField(u1, "", 0, 0, Member::Struct, 6); + AddField(s2, "m6", 6, 2, Member::Field); + AddField(s2, "m7", 8, 2, Member::Field); + EXPECT_EQ(WrappedRecord(this->record), WrappedRecord(record)); +} From fc1478f60e9ad5262503f19cf79150012cb94eb1 Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Tue, 4 Nov 2025 10:01:22 +0100 Subject: [PATCH 2/6] fix: class layout test --- lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp b/lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp index 36bfdb9a8e565..83ed533eb13e3 100644 --- a/lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp +++ b/lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp @@ -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: } @@ -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 { @@ -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: }; @@ -72,6 +71,7 @@ // CHECK-NEXT: char c2; // CHECK-NEXT: }; // CHECK-NEXT: }; +// CHECK-NEXT: S3 s1; // CHECK-NEXT: } From 5643c377eab0e4c8c9203ac2622ae38487fa9934 Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Tue, 4 Nov 2025 10:04:06 +0100 Subject: [PATCH 3/6] fix: move `parent` assignment to else branch --- .../Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp b/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp index 1458079483278..8e544d5f2c505 100644 --- a/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp +++ b/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp @@ -466,15 +466,16 @@ void UdtRecordCompleter::Record::ConstructRecord() { } if (iter->second.empty()) continue; - parent = iter->second.back(); // For structs, if the new fields come after the already added ones // without overlap, go back to the root struct. - if (parent != &record && iter->first <= offset && - record.kind == Member::Struct && is_last_end_offset(iter)) + if (iter->first <= offset && record.kind == Member::Struct && + is_last_end_offset(iter)) parent = &record; - else + 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. From 2e0b691295b04e04eddeabe4ae1760d4d381cae9 Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Tue, 4 Nov 2025 10:05:21 +0100 Subject: [PATCH 4/6] fix: test name --- lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp b/lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp index 3432e03bbf853..81566807ab0c6 100644 --- a/lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp +++ b/lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp @@ -256,7 +256,7 @@ TEST_F(UdtRecordCompleterRecordTests, TestNestedUnionStructInUnion) { EXPECT_EQ(WrappedRecord(this->record), WrappedRecord(record)); } -TEST_F(UdtRecordCompleterRecordTests, TestNestedUnionStructInUnion2) { +TEST_F(UdtRecordCompleterRecordTests, TestNestedStructInUnionInStructInUnion) { SetKind(Member::Kind::Union); CollectMember("m1", 0, 4); CollectMember("m2", 0, 2); From 8207a5fd8e3e374e8f1d457ffe8157fe4c21193c Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Tue, 4 Nov 2025 22:36:07 +0100 Subject: [PATCH 5/6] fix: handle unions as well --- .../NativePDB/UdtRecordCompleter.cpp | 19 +++++++++++------ .../NativePDB/UdtRecordCompleterTests.cpp | 21 ++++++++----------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp b/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp index 8e544d5f2c505..01674d5993dcd 100644 --- a/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp +++ b/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp @@ -467,12 +467,19 @@ void UdtRecordCompleter::Record::ConstructRecord() { if (iter->second.empty()) continue; - // For structs, if the new fields come after the already added ones - // without overlap, go back to the root struct. - if (iter->first <= offset && record.kind == Member::Struct && - is_last_end_offset(iter)) - parent = &record; - else { + // 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()); + // For unions, append the field to the last struct + parent = record.fields.back().get(); + } + } else { parent = iter->second.back(); iter->second.pop_back(); } diff --git a/lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp b/lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp index 81566807ab0c6..cd6db5fcb1f4c 100644 --- a/lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp +++ b/lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp @@ -275,24 +275,21 @@ TEST_F(UdtRecordCompleterRecordTests, TestNestedStructInUnionInStructInUnion) { // m4; // union { // m5; - // struct { - // m6; - // m7; - // }; + // 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 *s1 = AddField(&record.record, "", 0, 0, Member::Struct); - AddField(s1, "m3", 0, 2, Member::Field); - AddField(s1, "m4", 2, 4, Member::Field); - Member *u1 = AddField(s1, "", 6, 0, Member::Union); - AddField(u1, "m5", 6, 2, Member::Field); - Member *s2 = AddField(u1, "", 0, 0, Member::Struct, 6); - AddField(s2, "m6", 6, 2, Member::Field); - AddField(s2, "m7", 8, 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)); } From 8135db868e5cc668727a06db5fa053d3c4e94621 Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Wed, 5 Nov 2025 11:31:59 +0100 Subject: [PATCH 6/6] fix: parentheses and assert --- .../SymbolFile/NativePDB/UdtRecordCompleter.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp b/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp index 01674d5993dcd..46cf9b8524ede 100644 --- a/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp +++ b/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp @@ -470,12 +470,12 @@ void UdtRecordCompleter::Record::ConstructRecord() { // 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) + if (record.kind == Member::Struct) { parent = &record; - else { - lldbassert(record.kind == Member::Union && - "Current record must be a union"); - lldbassert(!record.fields.empty()); + } else { + assert(record.kind == Member::Union && + "Current record must be a union"); + assert(!record.fields.empty()); // For unions, append the field to the last struct parent = record.fields.back().get(); }