Michael137 added inline comments.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2212
m_ast.GetAsCXXRecordDecl(clang_type.GetOpaqueQualType());
- if (record_decl)
+ if (record_decl) {
+ bool is_empty = true;
----------------
Generally I'm not sure if attaching a `clang::NoUniqueAddressAttr` to every
empty field is the right approach. That goes slightly against our attempts to
construct an AST that's faithful to the source to avoid unpredictable behaviour
(which isn't always possible but for the most part we try). This approach was
considered in https://reviews.llvm.org/D101237 but concern was raised about it
affecting ABI, etc., leading to subtle issues down the line.
Based on the the discussion in https://reviews.llvm.org/D101237 it seemed to me
like the only two viable solutions are:
1. Add a `DW_AT_byte_size` of `0` to the empty field
2. Add a `DW_AT_no_unique_address`
AFAICT Jan tried to implement (1) but never seemed to be able to fully add
support for this in the ASTImporter/LLDB. Another issue I see with this is that
sometimes the byte-size of said field is not `0`, depending on the context in
which the structure is used.
I'm still leaning towards proposing a `DW_AT_no_unique_address`. Which is
pretty easy to implement and also reason about from LLDB's perspective.
@dblaikie @aprantl does that sound reasonable to you?
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2219
+ if (is_empty_field)
+ field->addAttr(clang::NoUniqueAddressAttr::Create(
+ m_ast.getASTContext(), clang::SourceRange()));
----------------
Typically the call to `record_decl->fields()` below would worry me, because if
the decl `!hasLoadedFieldsFromExternalStorage()` then we'd start another
`ASTImport` process, which could lead to some unpredictable behaviour if we are
already in the middle of an import. But since
`CompleteTagDeclarationDefinition` sets
`setHasLoadedFieldsFromExternalStorage(true)` I *think* we'd be ok. Might
warrant a comment.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2231
+ if (is_empty)
+ record_decl->markEmpty();
+ }
----------------
Why do we need to mark the parents empty here again? Wouldn't they have been
marked in `ParseStructureLikeDIE`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143347/new/
https://reviews.llvm.org/D143347
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
