[lldb][SymbolFileDWARF] Share GetDIEToType between SymbolFiles of a SymbolFileDWARFDebugMap #120569
Conversation
…DIE's SymbolFile The problem here manifests as follows: 1. We are stopped in main.o, so the first `ParseTypeFromDWARF` on `FooImpl<char>` gets called on `main.o`'s SymbolFile. This adds a mapping from *declaration die* -> `TypeSP` into `main.o`'s `GetDIEToType` map. 2. We then `CompleteType(FooImpl<char>)`. Depending on the order of entries in the debug-map, this might call `CompleteType` on `lib.o`'s SymbolFile. In which case, `GetDIEToType().lookup(decl_die)` will return a `nullptr`. This is already a bit iffy because surrounding code used to assume we don't call `CompleteTypeFromDWARF` with a `nullptr` `Type*`. This used to be more of an issue when `CompleteRecordType` actually made use of the `Type*` parameter (see llvm#120456). 3. While in `CompleteTypeFromDWARF`, we call `ParseTypeFromDWARF` again. This will parse the member function `FooImpl::Create` and its return type which is a typedef to `FooImpl*`. But now we're inside `lib.o`'s SymbolFile, so we call it on the definition DIE. In step (2) we just inserted a `nullptr` into `GetDIEToType` for the definition DIE, so we trivially return a `nullptr` from `ParseTypeFromDWARF`. Instead of reporting back this parse failure to the user LLDB trucks on and marks `FooImpl::Ref` to be `void*`. This test-case will trigger an assert in `TypeSystemClang::VerifyDecl` even if we just `frame var` (but only in debug-builds). In release builds where this function is a no-op, we'll create an incorrect Clang AST node for the `Ref` typedef.
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) Changes…DIE's SymbolFile The problem here manifests as follows:
This test-case will trigger an assert in The proposed fix here is to check the Full diff: https://github.com/llvm/llvm-project/pull/120569.diff 6 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 68e50902d641a2..936a44865b4a7e 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1593,7 +1593,9 @@ bool SymbolFileDWARF::CompleteType(CompilerType &compiler_type) {
DWARFASTParser *dwarf_ast = GetDWARFParser(*def_die.GetCU());
if (!dwarf_ast)
return false;
- Type *type = GetDIEToType().lookup(decl_die.GetDIE());
+ Type *type = decl_die.GetDWARF()->GetDIEToType().lookup(decl_die.GetDIE());
+ assert (type);
+
if (decl_die != def_die) {
GetDIEToType()[def_die.GetDIE()] = type;
DWARFASTParserClang *ast_parser =
diff --git a/lldb/test/API/lang/cpp/typedef-to-outer-fwd/Makefile b/lldb/test/API/lang/cpp/typedef-to-outer-fwd/Makefile
new file mode 100644
index 00000000000000..d9db5666f9b6cd
--- /dev/null
+++ b/lldb/test/API/lang/cpp/typedef-to-outer-fwd/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := lib.cpp main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/typedef-to-outer-fwd/TestTypedefToOuterFwd.py b/lldb/test/API/lang/cpp/typedef-to-outer-fwd/TestTypedefToOuterFwd.py
new file mode 100644
index 00000000000000..ad26d503c545b2
--- /dev/null
+++ b/lldb/test/API/lang/cpp/typedef-to-outer-fwd/TestTypedefToOuterFwd.py
@@ -0,0 +1,32 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestCaseTypedefToOuterFwd(TestBase):
+ '''
+ We are stopped in main.o, which only sees a forward declaration
+ of FooImpl. We then try to get the FooImpl::Ref typedef (whose
+ definition is in lib.o). Make sure we correctly resolve this
+ typedef.
+ '''
+ def test(self):
+ self.build()
+ (_, _, thread, _) = lldbutil.run_to_source_breakpoint(
+ self, "return", lldb.SBFileSpec("main.cpp")
+ )
+
+ foo = thread.frames[0].FindVariable('foo')
+ self.assertSuccess(foo.GetError(), "Found foo")
+
+ foo_type = foo.GetType()
+ self.assertTrue(foo_type)
+
+ impl = foo_type.GetPointeeType()
+ self.assertTrue(impl)
+
+ ref = impl.FindDirectNestedType('Ref')
+ self.assertTrue(ref)
+
+ self.assertEqual(ref.GetCanonicalType(), foo_type)
diff --git a/lldb/test/API/lang/cpp/typedef-to-outer-fwd/lib.cpp b/lldb/test/API/lang/cpp/typedef-to-outer-fwd/lib.cpp
new file mode 100644
index 00000000000000..2bc38f6503396d
--- /dev/null
+++ b/lldb/test/API/lang/cpp/typedef-to-outer-fwd/lib.cpp
@@ -0,0 +1,15 @@
+#include "lib.h"
+
+template <typename T> struct FooImpl {
+ using Ref = FooImpl<T> *;
+
+ Ref Create() { return new FooImpl<T>(); }
+};
+
+Foo getString() {
+ FooImpl<char> impl;
+ Foo ret;
+ ret.impl = impl.Create();
+
+ return ret;
+}
diff --git a/lldb/test/API/lang/cpp/typedef-to-outer-fwd/lib.h b/lldb/test/API/lang/cpp/typedef-to-outer-fwd/lib.h
new file mode 100644
index 00000000000000..d5dfddf2246209
--- /dev/null
+++ b/lldb/test/API/lang/cpp/typedef-to-outer-fwd/lib.h
@@ -0,0 +1,10 @@
+#ifndef LIB_H_IN
+#define LIB_H_IN
+
+template <typename T> struct FooImpl;
+
+struct Foo {
+ FooImpl<char> *impl = nullptr;
+};
+
+#endif // LIB_H_IN
diff --git a/lldb/test/API/lang/cpp/typedef-to-outer-fwd/main.cpp b/lldb/test/API/lang/cpp/typedef-to-outer-fwd/main.cpp
new file mode 100644
index 00000000000000..61c819ca0d31a1
--- /dev/null
+++ b/lldb/test/API/lang/cpp/typedef-to-outer-fwd/main.cpp
@@ -0,0 +1,8 @@
+#include "lib.h"
+
+extern Foo getString();
+
+int main() {
+ FooImpl<char> *foo = getString().impl;
+ return 0;
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
✅ With the latest revision this PR passed the Python code formatter. |
|
This opens a couple cans of worms (though I think this fix still makes sense in isolation):
|
Michael137
changed the title
| @@ -1593,7 +1593,9 @@ bool SymbolFileDWARF::CompleteType(CompilerType &compiler_type) { | |||
| DWARFASTParser *dwarf_ast = GetDWARFParser(*def_die.GetCU()); | |||
| if (!dwarf_ast) | |||
| return false; | |||
| Type *type = GetDIEToType().lookup(decl_die.GetDIE()); | |||
| Type *type = decl_die.GetDWARF()->GetDIEToType().lookup(decl_die.GetDIE()); | |||
There was a problem hiding this comment.
Can you edit down your commit message into a comment that explains the double-lookup here?
Should we bail out early if the Type* is null and return false to tell |
Yea that might be even simpler. Let me try that. Also, the other crash i alluded to seems to come from the fact that |
I agree that checking |
It's causing issues when we call
|
Ah, I missed this part. Thanks for the explanation. Also thanks for fixing those bugs introduced by the delay def die searching change. |
What would be the alternative? We might be able to log something or somehow report an error, but I don't think we can exclude the possibility of failure because of how the lazy parsing works.
Maybe, but I don't expect the test to be pretty. What I (sometimes) do in these cases is to make the test bidirectional, i.e., have two compile unit where each one has a definition for a type declared/used in the other one. This way, you're guaranteed to hit the error code path regardless of the order of the two units. This is easier to do if you don't actually need to run the binary (which you probably don't need to do here).
The way I interpret |
Just tried this. Unfortunately we remove the CompilerType from the I guess we could put it back on failure? Not sure if it's worth the potential trouble |
Yea I considered that, but I was concerned with the unforeseen complications this might have. Particularly I'm not sure how the |
Great idea, let me try this out |
On second thought, this does feel more consistent with how all the other bookkeeping structures already work. And re. the ownership, my current patch pretty much achieves the same thing (but in a less apparent manner). I'm still mixing |
Michael137
changed the title
We can move the the retrieval and null check of |
Good to hear that works and making |
|
|
||
| self.assertEqual(ref.GetCanonicalType(), var_type) | ||
|
|
||
| def test_definition_in_main(self): |
There was a problem hiding this comment.
Maybe merge these into one so we don't end up building the binary twice (for each debug info flavour)
Michael137
force-pushed
the
bugfix/lldb-typedef-to-outer
branch
from
8397543 to
d87abe7
Compare
…queDWARFASTTypeList in C++ (#120809) This addresses an issue encountered when investigating #120569. In `ParseTypeFromDWARF`, we insert the parsed type into `UniqueDWARFASTTypeMap` using the typename and `Declaration` obtained with `GetUniqueTypeNameAndDeclaration`. For C++ `GetUniqueTypeNameAndDeclaration` will zero the `Declaration` info (presumably because forward declaration may not have it, and for C++, ODR means we can rely on fully qualified typenames for uniqueing). When we then called `CompleteType`, we would first `MapDeclDIEToDefDIE`, updating the `UniqueDWARFASTType` we inserted previously with the `Declaration` of the definition DIE (without zeroing it). We would then call into `ParseTypeFromDWARF` for the same type again, and search the `UniqueDWARFASTTypeMap`. But we do the search using a zeroed declaration we get from `GetUniqueTypeNameAndDeclaration`. This particular example was fixed by #120569 but this still feels a little inconsistent. I'm not entirely sure we can get into that situation after that patch anymore. So the only test-case I could come up with was the unit-test which calls `MapDeclDIEToDefDIE` directly.
…ymbolFileDWARFDebugMap (llvm#120569) The problem here manifests as follows: 1. We are stopped in main.o, so the first `ParseTypeFromDWARF` on `FooImpl<char>` gets called on `main.o`'s SymbolFile. This adds a mapping from *declaration die* -> `TypeSP` into `main.o`'s `GetDIEToType` map. 2. We then `CompleteType(FooImpl<char>)`. Depending on the order of entries in the debug-map, this might call `CompleteType` on `lib.o`'s SymbolFile. In which case, `GetDIEToType().lookup(decl_die)` will return a `nullptr`. This is already a bit iffy because some of the surrounding code assumes we don't call `CompleteTypeFromDWARF` with a `nullptr` `Type*`. E.g., `CompleteEnumType` blindly dereferences it (though enums will never encounter this because their definition is fetched in ParseEnum, unlike for structures). 3. While in `CompleteTypeFromDWARF`, we call `ParseTypeFromDWARF` again. This will parse the member function `FooImpl::Create` and its return type which is a typedef to `FooImpl*`. But now we're inside `lib.o`'s SymbolFile, so we call it on the definition DIE. In step (2) we just inserted a `nullptr` into `GetDIEToType` for the definition DIE, so we trivially return a `nullptr` from `ParseTypeFromDWARF`. Instead of reporting back this parse failure to the user LLDB trucks on and marks `FooImpl::Ref` to be `void*`. This test-case will trigger an assert in `TypeSystemClang::VerifyDecl` even if we just `frame var` (but only in debug-builds). In release builds where this function is a no-op, we'll create an incorrect Clang AST node for the `Ref` typedef. The proposed fix here is to share the `GetDIEToType` map between SymbolFiles if a debug-map exists. **Alternatives considered** * Check the `GetDIEToType` map of the `SymbolFile` that the declaration DIE belongs to. The assumption here being that if we called `ParseTypeFromDWARF` on a declaration, the `GetDIEToType` map that the result was inserted into was the one on that DIE's SymbolFile. This was the first version of this patch, but that felt like a weaker version sharing the map. It complicates the code in `CompleteType` and is less consistent with the other bookkeeping structures we already share between SymbolFiles * Return from `SymbolFileDWARF::CompleteType` if there is no type in the current `GetDIEToType`. Then `SymbolFileDWARFDebugMap::CompleteType` could continue to the next `SymbolFile` which does own the type. But that didn't quite work because we remove the `GetForwardCompilerTypeToDie` entry in `SymbolFile::CompleteType`, which `SymbolFileDWARFDebugMap::CompleteType` relies upon for iterating (cherry picked from commit 28d1490)
…queDWARFASTTypeList in C++ (llvm#120809) This addresses an issue encountered when investigating llvm#120569. In `ParseTypeFromDWARF`, we insert the parsed type into `UniqueDWARFASTTypeMap` using the typename and `Declaration` obtained with `GetUniqueTypeNameAndDeclaration`. For C++ `GetUniqueTypeNameAndDeclaration` will zero the `Declaration` info (presumably because forward declaration may not have it, and for C++, ODR means we can rely on fully qualified typenames for uniqueing). When we then called `CompleteType`, we would first `MapDeclDIEToDefDIE`, updating the `UniqueDWARFASTType` we inserted previously with the `Declaration` of the definition DIE (without zeroing it). We would then call into `ParseTypeFromDWARF` for the same type again, and search the `UniqueDWARFASTTypeMap`. But we do the search using a zeroed declaration we get from `GetUniqueTypeNameAndDeclaration`. This particular example was fixed by llvm#120569 but this still feels a little inconsistent. I'm not entirely sure we can get into that situation after that patch anymore. So the only test-case I could come up with was the unit-test which calls `MapDeclDIEToDefDIE` directly. (cherry picked from commit c660b28)
…through UniqueDWARFASTTypeList in C++ (#120809) This addresses an issue encountered when investigating llvm/llvm-project#120569. In `ParseTypeFromDWARF`, we insert the parsed type into `UniqueDWARFASTTypeMap` using the typename and `Declaration` obtained with `GetUniqueTypeNameAndDeclaration`. For C++ `GetUniqueTypeNameAndDeclaration` will zero the `Declaration` info (presumably because forward declaration may not have it, and for C++, ODR means we can rely on fully qualified typenames for uniqueing). When we then called `CompleteType`, we would first `MapDeclDIEToDefDIE`, updating the `UniqueDWARFASTType` we inserted previously with the `Declaration` of the definition DIE (without zeroing it). We would then call into `ParseTypeFromDWARF` for the same type again, and search the `UniqueDWARFASTTypeMap`. But we do the search using a zeroed declaration we get from `GetUniqueTypeNameAndDeclaration`. This particular example was fixed by llvm/llvm-project#120569 but this still feels a little inconsistent. I'm not entirely sure we can get into that situation after that patch anymore. So the only test-case I could come up with was the unit-test which calls `MapDeclDIEToDefDIE` directly.
The problem here manifests as follows:
ParseTypeFromDWARFonFooImpl<char>gets called onmain.o's SymbolFile. This adds a mapping from declaration die ->TypeSPintomain.o'sGetDIEToTypemap.CompleteType(FooImpl<char>). Depending on the order of entries in the debug-map, this might callCompleteTypeonlib.o's SymbolFile. In which case,GetDIEToType().lookup(decl_die)will return anullptr. This is already a bit iffy because some of the surrounding code assumes we don't callCompleteTypeFromDWARFwith anullptrType*. E.g.,CompleteEnumTypeblindly dereferences it (though enums will never encounter this because their definition is fetched in ParseEnum, unlike for structures).CompleteTypeFromDWARF, we callParseTypeFromDWARFagain. This will parse the member functionFooImpl::Createand its return type which is a typedef toFooImpl*. But now we're insidelib.o's SymbolFile, so we call it on the definition DIE. In step (2) we just inserted anullptrintoGetDIEToTypefor the definition DIE, so we trivially return anullptrfromParseTypeFromDWARF. Instead of reporting back this parse failure to the user LLDB trucks on and marksFooImpl::Refto bevoid*.This test-case will trigger an assert in
TypeSystemClang::VerifyDecleven if we justframe var(but only in debug-builds). In release builds where this function is a no-op, we'll create an incorrect Clang AST node for theReftypedef.The proposed fix here is to share the
GetDIEToTypemap between SymbolFiles if a debug-map exists.Alternatives considered
GetDIEToTypemap of theSymbolFilethat the declaration DIE belongs to. The assumption here being that if we calledParseTypeFromDWARFon a declaration, theGetDIEToTypemap that the result was inserted into was the one on that DIE's SymbolFile. This was the first version of this patch, but that felt like a weaker version sharing the map. It complicates the code inCompleteTypeand is less consistent with the other bookkeeping structures we already share between SymbolFilesSymbolFileDWARF::CompleteTypeif there is no type in the currentGetDIEToType. ThenSymbolFileDWARFDebugMap::CompleteTypecould continue to the nextSymbolFilewhich does own the type. But that didn't quite work because we remove theGetForwardCompilerTypeToDieentry inSymbolFile::CompleteType, whichSymbolFileDWARFDebugMap::CompleteTyperelies upon for iterating