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