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

[Michael Buch via Phabricator via llvm-commits]

Michael137 added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1481
+    // To fix this we scan base classes in reverse order to determine
+    // overlapping offsets. Wnen found we consider such class as empty
+    // base with all fields having [[no_unique_address]] attribute.
----------------



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1483
+    // base with all fields having [[no_unique_address]] attribute.
+    for (auto it = base_classes.rbegin(); it != base_classes.rend(); ++it) {
+      clang::CXXRecordDecl *prev_base_decl =
----------------
The main problem I still see with this is that if we have something like:
```
struct A : C, B {

};
```

then we mark `C`'s fields as empty and leave `B` as is. This still leads to the same crash later on.

Perhaps we should mark we could check the size of the struct and decide based on that which one is the "empty" one


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1498
+            ast->getASTContext(), clang::SourceRange()));
+    }
     layout_info.base_offsets.insert(std::make_pair(
----------------
I think it would still be nice to have this as a private helper function on `DWARFASTParserClang`. But don't feel very strongly about it so feel free to ignore


================
Comment at: lldb/test/API/types/TestEmptyBase.py:2
+"""
+Test that recursive types are handled correctly.
+"""
----------------
Description needs fixing


================
Comment at: lldb/test/API/types/TestEmptyBase.py:22
+
+        self.sources = {
+            'CXX_SOURCES': 'empty_base_type.cpp'}
----------------
Ideally this test would live in `lldb/test/API/lang/cpp/no_unique_address/`

The usual structure is that we have a `Makefile` in the same directory which consists of:
```
CXX_SOURCES := main.cpp
                       
include Makefile.rules 
```

Then in the `test(self)` you just call `self.build()`. You don't need the `setUp(self)` and `setTearDownCleanup(...)` calls then.

Check `lldb/test/API/commands/expression/inline-namespace/TestInlineNamespace.py` for example.


================
Comment at: lldb/test/API/types/empty_base_type.cpp:1
+struct C
+{
----------------
Can we add more test cases. E.g.,:

```
struct A : C, B {
 ...
};
```


================
Comment at: lldb/test/API/types/empty_base_type.cpp:3
+{
+ long c,d;
+};
----------------
Just for sanity checking in the test


================
Comment at: lldb/test/API/types/empty_base_type.cpp:23
+{
+ long a,b;
+} _a;
----------------
same as above


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143347/new/

https://reviews.llvm.org/D143347