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

When anonymous unions are used in a struct or vice versa, their fields
are merged into the parent record when using PDB. LLDB tries to recreate
the original definition of the record _with_ the anonymous
unions/structs.

For tagged unions (like `std::optional`) where the tag followed the
anonymous union, the result was suboptimal:

```cpp
// input:
struct Foo {
  union {
    Bar b;
    char c;
  };
  bool tag;
};

// reconstructed:
struct Foo {
  union {
    Bar b;
    struct {
      char c;
      bool tag;
    };
  };
};
```

Once the algorithm is in some nested union, it can't get out.

In the above case, we can get to the correct reconstructed record if we
always add fields that don't overlap others in the root struct. So when
we see `tag`, we'll see that it comes after all other fields, so it's
possible to add it in the root `Foo`.

Author: Nerixyz

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<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;
@@ -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 {
+          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();
+        }
+      } 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.

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: }
 
 

lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp

@@ -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();
 }
@@ -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,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);
+  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));
+}