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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
