[PATCH] D143347: [lldb][DWARF] Infer no_unique_address attribute

[Michael Buch via Phabricator via cfe-commits]

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