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