[Lldb-commits] [PATCH] D143347: [lldb][DWARF] Infer no_unique_address attribute

[Pavel Kosov via Phabricator via lldb-commits]

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