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

[Pavel Labath via Phabricator via lldb-commits]

labath 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);
----------------
zequanwu wrote:
> 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.
Are you sure about that? I've created what I think is a [[ https://reviews.llvm.org/D135768 | counterexample ]] to these statements. Here a top-level union contains a bunch of sequential fields, which is perfectly possible if the only member of that union is an anonymous struct which contains those fields.

I don't think that's what supposed to happen in this case, but I'm open to being convinced otherwise.

(I've also rewritten the test logic so it produces better error messages than "false is not true".)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134849