[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:459
+      uint64_t end_offset = offset + fields.back()->bit_size;
+      parent->fields.push_back(fields.back());
+      end_offset_map[end_offset].push_back(parent);
----------------
labath wrote:
> Can `parent` be a union here? Would the algorithm still be correct?
`parent` could only be union when the top level record is a union (at this line `Member *parent = &record;`). That's the only case when we add an union into `end_offset_map`. In that case, all the fields would start at the same offset and we only iterate the loop `for (auto &pair : fields) {` once and then we are done. 
Otherwise, we only insert {end offset, struct/field} into `end_offset_map` where field must be within an union.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:471
+      } else {
+        for (auto &field : fields) {
+          parent->fields.push_back(field);
----------------
labath wrote:
> 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?
This is a special case to handle the case when top level record is an union. That's the only case we would reach here. This is to avoid adding unnecessary nested union. For example, the unit test `TestAnonymousUnionInUnion` would become the following if we always add an anonymous union:
```
union {
  union {
    m1;
    m2;
    m3;
    m4;
  }
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134849