[LLDB] Fix GetIndexOfChildMemberWithName to handle anonymous structs. #138487
Conversation
When handling anonymous structs, GetIndexOfChildMemberWithName needs to add the number of non-empty base classes to the child index, to get the actual correct index. It was not doing so. This fixes that.
|
@llvm/pr-subscribers-lldb Author: None (cmtice) ChangesWhen handling anonymous structs, GetIndexOfChildMemberWithName needs to add the number of non-empty base classes to the child index, to get the actual correct index. It was not doing so. This fixes that. Full diff: https://github.com/llvm/llvm-project/pull/138487.diff 4 Files Affected:
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 1a2b3d4133e51..dd8d144510056 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -6743,7 +6743,14 @@ size_t TypeSystemClang::GetIndexOfChildMemberWithName(
if (field_name.empty()) {
CompilerType field_type = GetType(field->getType());
std::vector<uint32_t> save_indices = child_indexes;
- child_indexes.push_back(child_idx);
+ if (field_type.IsAnonymousType())
+ // We have to add on the number of non-empty base classes to this
+ // index!
+ child_indexes.push_back(
+ child_idx +
+ TypeSystemClang::GetNumBaseClasses(cxx_record_decl, true));
+ else
+ child_indexes.push_back(child_idx);
if (field_type.GetIndexOfChildMemberWithName(
name, omit_empty_base_classes, child_indexes))
return child_indexes.size();
diff --git a/lldb/test/API/lang/cpp/type_lookup_anon_struct/Makefile b/lldb/test/API/lang/cpp/type_lookup_anon_struct/Makefile
new file mode 100644
index 0000000000000..99998b20bcb05
--- /dev/null
+++ b/lldb/test/API/lang/cpp/type_lookup_anon_struct/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/type_lookup_anon_struct/TestCppTypeLookupAnonStruct.py b/lldb/test/API/lang/cpp/type_lookup_anon_struct/TestCppTypeLookupAnonStruct.py
new file mode 100644
index 0000000000000..2295ecfe81ff7
--- /dev/null
+++ b/lldb/test/API/lang/cpp/type_lookup_anon_struct/TestCppTypeLookupAnonStruct.py
@@ -0,0 +1,27 @@
+"""
+Test that we properly print multiple types.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import decorators
+
+
+class TestTypeLookupAnonStruct(TestBase):
+ def test_lookup_anon_struct(self):
+ self.build()
+ lldbutil.run_to_source_breakpoint(
+ self, '// Set breakpoint here', lldb.SBFileSpec('main.cpp')
+ )
+
+ self.expect_var_path('unnamed_derived.y', value='2')
+ self.expect_var_path('unnamed_derived.z', value='13')
+ self.expect('frame variable "derb.x"', error=True,
+ substrs=['"x" is not a member of "(DerivedB) derb"'])
+ self.expect('frame variable "derb.y"', error=True,
+ substrs=['"y" is not a member of "(DerivedB) derb"'])
+ self.expect_var_path('derb.w', value='14')
+ self.expect_var_path('derb.k', value='15')
+ self.expect_var_path('derb.a.x', value='1')
+ self.expect_var_path('derb.a.y', value='2')
diff --git a/lldb/test/API/lang/cpp/type_lookup_anon_struct/main.cpp b/lldb/test/API/lang/cpp/type_lookup_anon_struct/main.cpp
new file mode 100644
index 0000000000000..18dd997ea9563
--- /dev/null
+++ b/lldb/test/API/lang/cpp/type_lookup_anon_struct/main.cpp
@@ -0,0 +1,33 @@
+int main(int argc, char** argv) {
+ struct A {
+ struct {
+ int x = 1;
+ };
+ int y = 2;
+ } a;
+
+ struct B {
+ // Anonymous struct inherits another struct.
+ struct : public A {
+ int z = 3;
+ };
+ int w = 4;
+ A a;
+ } b;
+
+ struct : public A {
+ struct {
+ int z = 13;
+ };
+ } unnamed_derived;
+
+ struct DerivedB : public B {
+ struct {
+ // `w` in anonymous struct overrides `w` from `B`.
+ int w = 14;
+ int k = 15;
+ };
+ } derb;
+
+ return 0; // Set breakpoint here
+}
|
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp -- lldb/test/API/lang/cpp/type_lookup_anon_struct/main.cpp lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cppView the diff from clang-format here.diff --git a/lldb/test/API/lang/cpp/type_lookup_anon_struct/main.cpp b/lldb/test/API/lang/cpp/type_lookup_anon_struct/main.cpp
index a9288e646..6bb2000e3 100644
--- a/lldb/test/API/lang/cpp/type_lookup_anon_struct/main.cpp
+++ b/lldb/test/API/lang/cpp/type_lookup_anon_struct/main.cpp
@@ -15,9 +15,7 @@ int main(int argc, char **argv) {
A a;
} b;
-
- struct EmptyBase {
- };
+ struct EmptyBase {};
struct : public A {
struct {
|
You can test this locally with the following command:darker --check --diff -r HEAD~1...HEAD lldb/test/API/lang/cpp/type_lookup_anon_struct/TestCppTypeLookupAnonStruct.pyView the diff from darker here.--- TestCppTypeLookupAnonStruct.py 2025-05-08 03:34:04.000000 +0000
+++ TestCppTypeLookupAnonStruct.py 2025-05-08 03:37:33.205966 +0000
@@ -18,16 +18,16 @@
self.expect_var_path("unnamed_derived.y", value="2")
self.expect_var_path("unnamed_derived.z", value="13")
self.expect(
'frame variable "derb.x"',
error=True,
- substrs=['"x" is not a member of "(DerivedB) derb"']
+ substrs=['"x" is not a member of "(DerivedB) derb"'],
)
self.expect(
'frame variable "derb.y"',
error=True,
- substrs=['"y" is not a member of "(DerivedB) derb"']
+ substrs=['"y" is not a member of "(DerivedB) derb"'],
)
self.expect_var_path("derb.w", value="14")
self.expect_var_path("derb.k", value="15")
self.expect_var_path("derb.a.x", value="1")
self.expect_var_path("derb.a.y", value="2")
|
|
I'm not intimately familiar with this code, but I am somewhat suspicious of implementation. Adding the number of base classes to the result makes sense to me. What surprises me is that this should be done only for fields with anonymous types. Can you explain why should the index of a field depend on whether its type is anonymous or not? I am also surprised by the hardcoding of |
I knew that I needed to add it for anonymous structs; I wasn't sure about other cases. I've tested it more now, and it seems good to add it in all cases, so I will do that.
I knew that in the case of anonymous structs we did not want to count empty base classes; since this line was for anonymous structs, I hard-coded it to make sure. Now that I've updated the code to always add the number of base classes, I'm removing that hard-coding. |
structs). Remove hard-codimg for omitting empty base classes.
There was a problem hiding this comment.
This looks good now. Thanks. I'm surprised that something like derb.x doesn't work -- both for "frame var" and in C++. You're not changing that, so it bears no relation to this patch, but overall, but overall, I think it wouldn't be unreasonable to make these kinds of expressions work in "frame var".
|
|
||
| struct DerivedB : public B { | ||
| struct { | ||
| // `w` in anonymous struct overrides `w` from `B`. |
There was a problem hiding this comment.
I think the precise term is "shadows"
| // We have to add on the number of non-empty base classes to this | ||
| // index! |
There was a problem hiding this comment.
This comment is redundant. The non-empty base classes is no longer true now that we're forwarding omit_empty_base_classes
There was a problem hiding this comment.
I will remove the comment
| @@ -0,0 +1,33 @@ | |||
| int main(int argc, char** argv) { | |||
There was a problem hiding this comment.
Can we add a test that involves empty base classes?
…llvm#138487) When handling anonymous structs, GetIndexOfChildMemberWithName needs to add the number of non-empty base classes to the child index, to get the actual correct index. It was not doing so. This fixes that.
When handling anonymous structs, GetIndexOfChildMemberWithName needs to add the number of non-empty base classes to the child index, to get the actual correct index. It was not doing so. This fixes that.