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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The test looks great, and the comments have helped. I still have a couple of questions about the algorithm though.



================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:431
+    uint64_t offset = pair.first;
+    auto &fields = pair.second;
+    lldbassert(offset >= start_offset);
----------------
This shadowing the fields member confused me for quite some time. Could you choose a different name for one of them?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:459
+      uint64_t end_offset = offset + fields.back()->bit_size;
+      parent->fields.push_back(fields.back());
+      end_offset_map[end_offset].push_back(parent);
----------------
Can `parent` be a union here? Would the algorithm still be correct?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:471
+      } else {
+        for (auto &field : fields) {
+          parent->fields.push_back(field);
----------------
IIUC, this code is reached when the `parent` object is a union. However, the parent is chosen such that it ends before the start of these new fields? Therefore its start address will be before the start of these fields as well. Is it correct to add the fields to the union under these circumstances, or am I misunderstanding something?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:64
+    uint64_t base_offset;
+    llvm::SmallVector<FieldSP, 1> fields;
+
----------------
zequanwu wrote:
> 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.
You can't have a `Field` member, but you can have a Field*, unique_ptr<Field> and std::vector<Field> members. SmallVector<Field> is also not possible, for reasons that are mostly obvious, but then again, storing a pointer inside a SmallVector negates most of the benefits that one could hope to gain by using it.

My point is that that using a shared pointer makes it harder to understand the relationships between the objects because it obfuscates the ownership aspect. Sometimes that is unavoidable, like when there just isn't a single object that can own some other object (although llvm mostly avoids that by putting the ownership inside some "context" object), but it's not clear to me why that would be the case here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134849