[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:
> 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".)
Thanks for writing the counter example and making better error message. I appended your changes in this file.
I updated to handle the case when parent is an union here by creating an anonymous struct to hold it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134849