kpdev42 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;
----------------
Michael137 wrote:
> 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?
I think that lldb jitter relies on value of `DW_AT_member` location when/if
empty structure address is taken, so assigning no_unique_address attribute
shouldn't, in my opinion, affect anything. Also, as I understand, AST obtained
from DWARF will not (and cannot) be identical to the source (e.g. because of
optimizations)
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2219
+ if (is_empty_field)
+ field->addAttr(clang::NoUniqueAddressAttr::Create(
+ m_ast.getASTContext(), clang::SourceRange()));
----------------
Michael137 wrote:
> 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.
We can probably use keys of `DenseMap` in `layout_info.base_offsets` to stay
safe, can't we?
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2231
+ if (is_empty)
+ record_decl->markEmpty();
+ }
----------------
Michael137 wrote:
> Why do we need to mark the parents empty here again? Wouldn't they have been
> marked in `ParseStructureLikeDIE`?
`ParseStructureLikeDIE` marks only trivially empty records (with no children or
with only children being template parameters). All non-trivially empty structs
(with only children being other empty structs) are marked here.
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
