Prevent GetNumChildren from transitively walking pointer chains

[jarin]

Summary

This is an attempt to fix https://bugs.llvm.org/show_bug.cgi?id=45988, where SBValue::GetNumChildren returns 2, but SBValue::GetChildAtIndex(1) returns an invalid value sentinel.

The root cause of this seems to be that GetNumChildren can return the number of children of a wrong value. In particular, for pointers GetNumChildren just recursively calls itself on the pointee type, so it effectively walks chains of pointers. This is different from the logic of GetChildAtIndex, which only recurses if pointee.IsAggregateType() returns true (IsAggregateType is false for pointers and references), so it never follows chain of pointers.

This patch aims to make GetNumChildren (more) consistent with GetChildAtIndex by only recursively calling GetNumChildren for aggregate types.

Ideally, GetNumChildren and GetChildAtIndex would share the code that decides which pointers/references are followed, but that is a bit more invasive change.

[shafik]

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

5217

I am curious what cases are pointers aggregates? Do we test this case?

lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py

21

We should also check

result.GetChildAtIndex(0).GetNumChildren()

same below

[jarin]

Added more assertions, reformatted the test code.

[jarin]

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

5217

As far as I understand, pointers are never aggregates, see [ https://github.com/llvm/llvm-project/blob/58684fbb6f2e6871f3f96ac8c7166a7d7486e971/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp#L2683 | IsAggregate ]].

[teemperor]

I think this looks good beside some minor nit-picks. Maybe @labath should take a second look too.

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

5227–5228

If you feel like refactoring this (by deleting code): You can also just delete all this and add the RValueReference and LValueReference to the ObjCObjectPointer above. GetPointeeType there is doing the same as the lines below and handles all ptrs/references. But that's optional.

lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py

2

I guess this was just a placeholder?

17

You can also do self.frame() and then you don't need to capture the thread above or declare a variable here. And I don't think you need to assign self.main_source_file (I believe this has no special meaning, other tests just did this because they liked to share the setup code in one setUp method).

lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.cpp"))

result = self.frame().FindVariable("Ref")

[labath]

Looks fine to me too. The way this test is phrased, it would probably make more sense under test/API/python_api/value, than here. (I mean, it's not technically wrong because everything is a "functionality", but i'd like to avoid putting stuff here precisely because the category is so vague.)

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

5227–5228

+1 for merging this stuff

lldb/test/API/functionalities/pointer_num_children/main.cpp

14–15

Since this test doesn't deal with auto in any way, I think we should just follow the llvm style guide here and spell out the type explicitly.

[jarin]

Merged the ObjC pointer case with the reference case, simplified the test.

[jarin]

In D80254#2046275, @labath wrote:

Looks fine to me too. The way this test is phrased, it would probably make more sense under test/API/python_api/value, than here. (I mean, it's not technically wrong because everything is a "functionality", but i'd like to avoid putting stuff here precisely because the category is so vague.)

I was actually considering lang/cpp since this is about Clang's type system and how it handles pointers and references. In any case, test/API/python_api/value is C, so I could not even test the reference case there. Would you prefer creating a new subdirectory in python_api?

[shafik]

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

5217

Apologies, I misread that as pointer_clang_type , makes more sense now.

[clayborg]

This looks good, thanks for subscribing me. We need to have GetNumChildren and GetChildAtIndex agreeing on things and we definitely shouldn't be walking more than on pointer recursively. My only question is do we need helper functions added to TypeSystemClang to avoid this issue since we have GetNumChildren and GetChildAtIndex doing things differently? Some function both could/should be calling so that things can't get out of sync?

[jarin]

In D80254#2047982, @clayborg wrote:

This looks good, thanks for subscribing me. We need to have GetNumChildren and GetChildAtIndex agreeing on things and we definitely shouldn't be walking more than on pointer recursively. My only question is do we need helper functions added to TypeSystemClang to avoid this issue since we have GetNumChildren and GetChildAtIndex doing things differently? Some function both could/should be calling so that things can't get out of sync?

Yeah, that is what I suggested in the patch's summary. Do you want to block landing the fix until the refactoring is done? In my experience, the discussions about such refactorings can take weeks, I am not sure if I can commit to doing that. I am happy to put out a separate patch for the refactoring once this one is landed.

[teemperor]

I think the refactoring is a good idea, but IMHO this patch can go in as-is. It's not adding any more debt to the existing approach (it even deletes code) and it adds tests, so I don't see a drawback of merging this. Also having this bug fixed with a small back portable patch seems better than doing it with some big refactor.

[clayborg]

Major changes not required. Thanks for finding and fixing this!

[jarin]

Raphael, could you land this for me?

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

if (GetCompleteQualType(&getASTContext(), qual_type)) {
           }
 
           num_children += class_interface_decl->ivar_size();
         }
       }
     }
     break;
 
+  case clang::Type::LValueReference:
+  case clang::Type::RValueReference:
   case clang::Type::ObjCObjectPointer: {
-    const clang::ObjCObjectPointerType *pointer_type =
-        llvm::cast<clang::ObjCObjectPointerType>(qual_type.getTypePtr());
-    clang::QualType pointee_type = pointer_type->getPointeeType();
-    uint32_t num_pointee_children =
-        GetType(pointee_type).GetNumChildren(omit_empty_base_classes, exe_ctx);
+    CompilerType pointee_clang_type(GetPointeeType(type));
+
+    uint32_t num_pointee_children = 0;
+    if (pointee_clang_type.IsAggregateType())
+      num_pointee_children =
+          pointee_clang_type.GetNumChildren(omit_empty_base_classes, exe_ctx);
     // If this type points to a simple type, then it has 1 child
     if (num_pointee_children == 0)
       num_children = 1;
     else
       num_children = num_pointee_children;
   } break;
 
   case clang::Type::Vector:
if (auto array_info =
                          ? array_info->element_orders.back()
                          : 0;
     break;
 
   case clang::Type::Pointer: {
     const clang::PointerType *pointer_type =
         llvm::cast<clang::PointerType>(qual_type.getTypePtr());
     clang::QualType pointee_type(pointer_type->getPointeeType());
-    uint32_t num_pointee_children =
-        GetType(pointee_type).GetNumChildren(omit_empty_base_classes, exe_ctx);
+    CompilerType pointee_clang_type(GetType(pointee_type));
+    uint32_t num_pointee_children = 0;
+    if (pointee_clang_type.IsAggregateType())
+      num_pointee_children =
+          pointee_clang_type.GetNumChildren(omit_empty_base_classes, exe_ctx);
     if (num_pointee_children == 0) {
       // We have a pointer to a pointee type that claims it has no children. We
       // will want to look at
       num_children = GetNumPointeeChildren(pointee_type);
     } else
       num_children = num_pointee_children;
   } break;
 
-  case clang::Type::LValueReference:
-  case clang::Type::RValueReference: {
-    const clang::ReferenceType *reference_type =
-        llvm::cast<clang::ReferenceType>(qual_type.getTypePtr());
-    clang::QualType pointee_type = reference_type->getPointeeType();
-    uint32_t num_pointee_children =
-        GetType(pointee_type).GetNumChildren(omit_empty_base_classes, exe_ctx);
-    // If this type points to a simple type, then it has 1 child
-    if (num_pointee_children == 0)
-      num_children = 1;
-    else
-      num_children = num_pointee_children;
-  } break;
-
   default:
     break;
   }
   return num_children;
 }
 
 CompilerType TypeSystemClang::GetBuiltinTypeByName(ConstString name) {
   return GetBasicType(GetBasicTypeEnumeration(name));
 }

lldb/test/API/functionalities/pointer_num_children/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py

+"""
+Test children counts of pointer values.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestPointerNumChilden(TestBase):
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test_pointer_num_children(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.cpp"))
+
+        result = self.frame().FindVariable("Ref")
+        self.assertEqual(1, result.GetNumChildren())
+        self.assertEqual(2, result.GetChildAtIndex(0).GetNumChildren())
+        self.assertEqual("42", result.GetChildAtIndex(0).GetChildAtIndex(0).GetValue())
+        self.assertEqual("56", result.GetChildAtIndex(0).GetChildAtIndex(1).GetValue())
+
+        result = self.frame().FindVariable("Ptr")
+        self.assertEqual(1, result.GetNumChildren())
+        self.assertEqual(2, result.GetChildAtIndex(0).GetNumChildren())
+        self.assertEqual("42", result.GetChildAtIndex(0).GetChildAtIndex(0).GetValue())
+        self.assertEqual("56", result.GetChildAtIndex(0).GetChildAtIndex(1).GetValue())

lldb/test/API/functionalities/pointer_num_children/main.cpp

+struct Inner {
+  int a;
+  int b;
+};
+
+struct Outer {
+  Inner *inner;
+};
+
+int main() {
+  Inner inner{42, 56};
+  Outer outer{&inner};
+  Inner **Ptr = &(outer.inner);
+  Inner *&Ref = outer.inner;
+  return 0; // break here
+}