[lldb] Fix dynamic type resolution for types parsed from declarations #137974
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThis fixes a regression caused by us starting to parse types from declarations. The code in TypeSystemClang was assuming that the value held in ClangASTMetadata was authoritative, but this isn't (and cannot) be the case when the type is parsed from a forward-declaration. For the fix, I add a new "don't know" state to ClangASTMetadata, and make sure DWARFASTParserClang sets it only when it encounters a forward declaration. In this case, the type system will fall back to completing the type. This does mean that we will be completing more types than before, but I'm hoping this will offset by the fact that we don't search for definition DIEs eagerly. In particular, I don't make a difference in -fstandalone-debug scenarios, since types will nearly always be present as definitions. To avoid this cost, we'd need to create some sort of a back channel to query the DWARFASTParser about the dynamicness of the type without actually completing it. I'd like to avoid that if it is not necessary. Full diff: https://github.com/llvm/llvm-project/pull/137974.diff 9 Files Affected:
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTMetadata.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTMetadata.cpp
index 42933c78b0277..2c5dacb60a9b8 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTMetadata.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTMetadata.cpp
@@ -11,6 +11,22 @@
using namespace lldb_private;
+std::optional<bool> ClangASTMetadata::GetIsDynamicCXXType() const {
+ switch (m_is_dynamic_cxx) {
+ case 0:
+ return std::nullopt;
+ case 1:
+ return false;
+ case 2:
+ return true;
+ }
+ llvm_unreachable("Invalid m_is_dynamic_cxx value");
+}
+
+void ClangASTMetadata::SetIsDynamicCXXType(std::optional<bool> b) {
+ m_is_dynamic_cxx = b ? *b + 1 : 0;
+}
+
void ClangASTMetadata::Dump(Stream *s) {
lldb::user_id_t uid = GetUserID();
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTMetadata.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTMetadata.h
index d3bcde2ced799..ecaa2e0ddf97c 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTMetadata.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTMetadata.h
@@ -12,6 +12,7 @@
#include "lldb/Core/dwarf.h"
#include "lldb/lldb-defines.h"
#include "lldb/lldb-enumerations.h"
+#include "lldb/lldb-private-enumerations.h"
namespace lldb_private {
@@ -19,12 +20,14 @@ class ClangASTMetadata {
public:
ClangASTMetadata()
: m_user_id(0), m_union_is_user_id(false), m_union_is_isa_ptr(false),
- m_has_object_ptr(false), m_is_self(false), m_is_dynamic_cxx(true),
- m_is_forcefully_completed(false) {}
+ m_has_object_ptr(false), m_is_self(false),
+ m_is_forcefully_completed(false) {
+ SetIsDynamicCXXType(std::nullopt);
+ }
- bool GetIsDynamicCXXType() const { return m_is_dynamic_cxx; }
+ std::optional<bool> GetIsDynamicCXXType() const;
- void SetIsDynamicCXXType(bool b) { m_is_dynamic_cxx = b; }
+ void SetIsDynamicCXXType(std::optional<bool> b);
void SetUserID(lldb::user_id_t user_id) {
m_user_id = user_id;
@@ -101,8 +104,8 @@ class ClangASTMetadata {
uint64_t m_isa_ptr;
};
- bool m_union_is_user_id : 1, m_union_is_isa_ptr : 1, m_has_object_ptr : 1,
- m_is_self : 1, m_is_dynamic_cxx : 1, m_is_forcefully_completed : 1;
+ unsigned m_union_is_user_id : 1, m_union_is_isa_ptr : 1, m_has_object_ptr : 1,
+ m_is_self : 1, m_is_dynamic_cxx : 2, m_is_forcefully_completed : 1;
};
} // namespace lldb_private
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index f22fcbab557a0..a3e809f44ed23 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1837,7 +1837,8 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
ClangASTMetadata metadata;
metadata.SetUserID(die.GetID());
- metadata.SetIsDynamicCXXType(dwarf->ClassOrStructIsVirtual(die));
+ if (!attrs.is_forward_declaration)
+ metadata.SetIsDynamicCXXType(dwarf->ClassOrStructIsVirtual(die));
TypeSystemClang::TemplateParameterInfos template_param_infos;
if (ParseTemplateParameterInfos(die, template_param_infos)) {
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index cb0ecd6ebd406..1a2b3d4133e51 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -3657,13 +3657,17 @@ bool TypeSystemClang::IsPossibleDynamicType(lldb::opaque_compiler_type_t type,
bool success;
if (cxx_record_decl->isCompleteDefinition())
success = cxx_record_decl->isDynamicClass();
- else if (std::optional<ClangASTMetadata> metadata =
- GetMetadata(cxx_record_decl))
- success = metadata->GetIsDynamicCXXType();
- else if (GetType(pointee_qual_type).GetCompleteType())
- success = cxx_record_decl->isDynamicClass();
- else
- success = false;
+ else {
+ std::optional<ClangASTMetadata> metadata = GetMetadata(cxx_record_decl);
+ std::optional<bool> is_dynamic =
+ metadata ? metadata->GetIsDynamicCXXType() : std::nullopt;
+ if (is_dynamic)
+ success = *is_dynamic;
+ else if (GetType(pointee_qual_type).GetCompleteType())
+ success = cxx_record_decl->isDynamicClass();
+ else
+ success = false;
+ }
if (success)
set_dynamic_pointee_type(pointee_qual_type);
diff --git a/lldb/test/API/lang/cpp/dynamic-value/Makefile b/lldb/test/API/lang/cpp/dynamic-value/Makefile
index ce91dc63f473f..24346ccfae510 100644
--- a/lldb/test/API/lang/cpp/dynamic-value/Makefile
+++ b/lldb/test/API/lang/cpp/dynamic-value/Makefile
@@ -1,3 +1,3 @@
-CXX_SOURCES := pass-to-base.cpp anonymous-b.cpp
+CXX_SOURCES := pass-to-base.cpp anonymous-b.cpp forward-a.cpp
include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py b/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py
index 32ef009279713..c0e7c52e5396e 100644
--- a/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py
+++ b/lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py
@@ -266,3 +266,15 @@ def examine_value_object_of_this_ptr(
contained_b_static_addr = int(contained_b_static.GetValue(), 16)
self.assertLess(contained_b_addr, contained_b_static_addr)
+
+ @no_debug_info_test
+ def test_from_forward_decl(self):
+ """Test fetching C++ dynamic values forward-declared types. It's
+ imperative that this is a separate test so that we don't end up parsing
+ a definition of A from somewhere else."""
+ self.build()
+ lldbutil.run_to_name_breakpoint(self, "take_A")
+ self.expect(
+ "frame var -d run-target --ptr-depth=1 --show-types a",
+ substrs = ["(B *) a", "m_b_value = 10"],
+ )
diff --git a/lldb/test/API/lang/cpp/dynamic-value/a.h b/lldb/test/API/lang/cpp/dynamic-value/a.h
index 708cbb79fee5c..c8895ab095d82 100644
--- a/lldb/test/API/lang/cpp/dynamic-value/a.h
+++ b/lldb/test/API/lang/cpp/dynamic-value/a.h
@@ -22,4 +22,6 @@ class A {
A *make_anonymous_B();
+A *take_A(A *a);
+
#endif
diff --git a/lldb/test/API/lang/cpp/dynamic-value/forward-a.cpp b/lldb/test/API/lang/cpp/dynamic-value/forward-a.cpp
new file mode 100644
index 0000000000000..8a212d928d5c5
--- /dev/null
+++ b/lldb/test/API/lang/cpp/dynamic-value/forward-a.cpp
@@ -0,0 +1,3 @@
+class A;
+
+A *take_A(A *a) { return a; }
diff --git a/lldb/test/API/lang/cpp/dynamic-value/pass-to-base.cpp b/lldb/test/API/lang/cpp/dynamic-value/pass-to-base.cpp
index be763390cc6f9..9b601084cdc57 100644
--- a/lldb/test/API/lang/cpp/dynamic-value/pass-to-base.cpp
+++ b/lldb/test/API/lang/cpp/dynamic-value/pass-to-base.cpp
@@ -45,5 +45,7 @@ main (int argc, char **argv)
myB.doSomething(*make_anonymous_B());
+ take_A(&myB);
+
return 0;
}
|
|
✅ With the latest revision this PR passed the Python code formatter. |
This fixes a regression caused by us starting to parse types from declarations. The code in TypeSystemClang was assuming that the value held in ClangASTMetadata was authoritative, but this isn't (and cannot) be the case when the type is parsed from a forward-declaration. For the fix, I add a new "don't know" state to ClangASTMetadata, and make sure DWARFASTParserClang sets it only when it encounters a forward declaration. In this case, the type system will fall back to completing the type. This does mean that we will be completing more types than before, but I'm hoping this will offset by the fact that we don't search for definition DIEs eagerly. In particular, I don't make a difference in -fstandalone-debug scenarios, since types will nearly always be present as definitions. To avoid this cost, we'd need to create some sort of a back channel to query the DWARFASTParser about the dynamicness of the type without actually completing it. I'd like to avoid that if it is not necessary.
Michael137
approved these changes
Apr 30, 2025
There was a problem hiding this comment.
Makes sense!
On the topic of ClangASTMetadata. Another thing I noticed a while ago that we might want to fix is that when we import decls from an origin, we don't copy over the metadata into the target typesystem. I forget why exactly that was causing issues, but at the very least it probably means we're recomputing some properties
| @@ -11,6 +11,22 @@ | |||
|
|
|||
| using namespace lldb_private; | |||
|
|
|||
| std::optional<bool> ClangASTMetadata::GetIsDynamicCXXType() const { | |||
| switch (m_is_dynamic_cxx) { | |||
| case 0: | |||
There was a problem hiding this comment.
Should we make these enum cases for readability? Don't feel strongly either way.
There was a problem hiding this comment.
I don't think that's necessary given that the only one who needs to know about them are these two short functions. And then I would be able to do the bool+1 trick :P
I don't know what to say about that. In the cases I was looking at at least, the type was completed by the expression command anyway, so dynamic type resolution already worked on those types. |
omjavaid
added a commit
that referenced
this pull request
May 5, 2025
The newly added test test_from_forward_decl in TestDynamicValue.py by PR #137974 is failing on Windows due to issues with dynamic type resolution. This is a known issue tracked in PR24663. LLDB Windows on Arm Buildbot Failure: https://lab.llvm.org/buildbot/#/builders/141/builds/8391 This change marks the test as XFAIL on Windows using the consistent with how similar tests in the same file are handled.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…llvm#137974) This fixes a regression caused by us starting to parse types from declarations. The code in TypeSystemClang was assuming that the value held in ClangASTMetadata was authoritative, but this isn't (and cannot) be the case when the type is parsed from a forward-declaration. For the fix, I add a new "don't know" state to ClangASTMetadata, and make sure DWARFASTParserClang sets it only when it encounters a forward declaration. In this case, the type system will fall back to completing the type. This does mean that we will be completing more types than before, but I'm hoping this will offset by the fact that we don't search for definition DIEs eagerly. In particular, I don't expect it to make a difference in -fstandalone-debug scenarios, since types will nearly always be present as definitions. To avoid this cost, we'd need to create some sort of a back channel to query the DWARFASTParser about the dynamicness of the type without actually completing it. I'd like to avoid that if it is not necessary.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…llvm#137974) This fixes a regression caused by us starting to parse types from declarations. The code in TypeSystemClang was assuming that the value held in ClangASTMetadata was authoritative, but this isn't (and cannot) be the case when the type is parsed from a forward-declaration. For the fix, I add a new "don't know" state to ClangASTMetadata, and make sure DWARFASTParserClang sets it only when it encounters a forward declaration. In this case, the type system will fall back to completing the type. This does mean that we will be completing more types than before, but I'm hoping this will offset by the fact that we don't search for definition DIEs eagerly. In particular, I don't expect it to make a difference in -fstandalone-debug scenarios, since types will nearly always be present as definitions. To avoid this cost, we'd need to create some sort of a back channel to query the DWARFASTParser about the dynamicness of the type without actually completing it. I'd like to avoid that if it is not necessary.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…llvm#137974) This fixes a regression caused by us starting to parse types from declarations. The code in TypeSystemClang was assuming that the value held in ClangASTMetadata was authoritative, but this isn't (and cannot) be the case when the type is parsed from a forward-declaration. For the fix, I add a new "don't know" state to ClangASTMetadata, and make sure DWARFASTParserClang sets it only when it encounters a forward declaration. In this case, the type system will fall back to completing the type. This does mean that we will be completing more types than before, but I'm hoping this will offset by the fact that we don't search for definition DIEs eagerly. In particular, I don't expect it to make a difference in -fstandalone-debug scenarios, since types will nearly always be present as definitions. To avoid this cost, we'd need to create some sort of a back channel to query the DWARFASTParser about the dynamicness of the type without actually completing it. I'd like to avoid that if it is not necessary.
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
…llvm#137974) This fixes a regression caused by us starting to parse types from declarations. The code in TypeSystemClang was assuming that the value held in ClangASTMetadata was authoritative, but this isn't (and cannot) be the case when the type is parsed from a forward-declaration. For the fix, I add a new "don't know" state to ClangASTMetadata, and make sure DWARFASTParserClang sets it only when it encounters a forward declaration. In this case, the type system will fall back to completing the type. This does mean that we will be completing more types than before, but I'm hoping this will offset by the fact that we don't search for definition DIEs eagerly. In particular, I don't expect it to make a difference in -fstandalone-debug scenarios, since types will nearly always be present as definitions. To avoid this cost, we'd need to create some sort of a back channel to query the DWARFASTParser about the dynamicness of the type without actually completing it. I'd like to avoid that if it is not necessary.
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
The newly added test test_from_forward_decl in TestDynamicValue.py by PR llvm#137974 is failing on Windows due to issues with dynamic type resolution. This is a known issue tracked in PR24663. LLDB Windows on Arm Buildbot Failure: https://lab.llvm.org/buildbot/#/builders/141/builds/8391 This change marks the test as XFAIL on Windows using the consistent with how similar tests in the same file are handled.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a regression caused by us starting to parse types from declarations. The code in TypeSystemClang was assuming that the value held in ClangASTMetadata was authoritative, but this isn't (and cannot) be the case when the type is parsed from a forward-declaration.
For the fix, I add a new "don't know" state to ClangASTMetadata, and make sure DWARFASTParserClang sets it only when it encounters a forward declaration. In this case, the type system will fall back to completing the type.
This does mean that we will be completing more types than before, but I'm hoping this will offset by the fact that we don't search for definition DIEs eagerly. In particular, I don't expect it to make a difference in -fstandalone-debug scenarios, since types will nearly always be present as definitions.
To avoid this cost, we'd need to create some sort of a back channel to query the DWARFASTParser about the dynamicness of the type without actually completing it. I'd like to avoid that if it is not necessary.