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