[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.

[Zequan Wu via Phabricator via lldb-commits]

zequanwu added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:392-443
+  // For anonymous unions in a struct, msvc generated pdb doesn't have the
+  // entity for that union. So, we need to construct anonymous union and struct
+  // based on field offsets. The final AST is likely not matching the exact
+  // original AST, but the memory layout is preseved.
+
+  // The end offset to a field/struct/union that ends at the offset.
+  std::map<uint64_t, Field*> end_offset_map;
----------------
labath wrote:
> This could use a high-level explanation of the algorithm. Like, I know it tries to stuff the fields into structs and unions, but I wasn't able to figure out how it does that, and what are the operating invariants.
> 
> The thing I like about this algorithm, is that the most complicated part (the thing I highlighted) is basically just playing with numbers and it is independent of any complex objects and state. If this part is separated out into a separate function, then it would be perfectly suitable for unit testing, and the unit tests could also serve as documentation/examples of the kinds of transformations that the algorithm does.
Moved into a separate function and added unit tests.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:64
+    uint64_t base_offset;
+    llvm::SmallVector<FieldSP, 1> fields;
+
----------------
labath wrote:
> Can we drop the `SP` part? I think that the owning references (I guess that's this field) could be just plain Field instances (unique_ptr<Field> at worst), and the rest could just be plain pointers and references.
The Field object is shared between fields and m_fields. And we can't have Field member instance inside Field class.


================
Comment at: lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp:53-59
+// CHECK-NEXT: | |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
+// CHECK-NEXT: | | |-DefaultConstructor exists trivial needs_implicit
+// CHECK-NEXT: | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
+// CHECK-NEXT: | | |-MoveConstructor exists simple trivial needs_implicit
+// CHECK-NEXT: | | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
+// CHECK-NEXT: | | |-MoveAssignment exists simple trivial needs_implicit
+// CHECK-NEXT: | | `-Destructor simple irrelevant trivial needs_implicit
----------------
labath wrote:
> I think we should exclude all of these DefinitionData fields from the test expectation, as they are largely irrelevant to the test (and they also obfuscate it very well). Otherwise, people will have to update these whenever a new field gets added.
> 
> I don't know whether it contains the thing you wanted to test (as I don't know what that is), but the `type lookup C` output will contain the general structure of the type, and it will be a lot more readable.
Thanks, I didn't know that before.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134849/new/

https://reviews.llvm.org/D134849