[Lldb-commits] [lldb] 83bd2c4 - Prevent GetNumChildren from transitively walking pointer chains

[Raphael Isemann via lldb-commits]

Author: Jaroslav Sevcik
Date: 2020-05-25T11:30:22+02:00
New Revision: 83bd2c4a06803fa9af7f92a474b1d37cb70397cc

URL: https://github.com/llvm/llvm-project/commit/83bd2c4a06803fa9af7f92a474b1d37cb70397cc
DIFF: https://github.com/llvm/llvm-project/commit/83bd2c4a06803fa9af7f92a474b1d37cb70397cc.diff

LOG: Prevent GetNumChildren from transitively walking pointer chains

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.

Reviewers: teemperor, jingham, clayborg

Reviewed By: teemperor, clayborg

Subscribers: clayborg, labath, shafik, lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D80254

Added: 
    lldb/test/API/functionalities/pointer_num_children/Makefile
    lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py
    lldb/test/API/functionalities/pointer_num_children/main.cpp

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

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 6e8946e23104..c687251ed5dc 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5172,12 +5172,15 @@ uint32_t TypeSystemClang::GetNumChildren(lldb::opaque_compiler_type_t type,
     }
     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;
@@ -5209,8 +5212,11 @@ uint32_t TypeSystemClang::GetNumChildren(lldb::opaque_compiler_type_t type,
     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
@@ -5219,20 +5225,6 @@ uint32_t TypeSystemClang::GetNumChildren(lldb::opaque_compiler_type_t type,
       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;
   }

diff  --git a/lldb/test/API/functionalities/pointer_num_children/Makefile b/lldb/test/API/functionalities/pointer_num_children/Makefile
new file mode 100644
index 000000000000..99998b20bcb0
--- /dev/null
+++ b/lldb/test/API/functionalities/pointer_num_children/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

diff  --git a/lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py b/lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py
new file mode 100644
index 000000000000..aaeaef75810c
--- /dev/null
+++ b/lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py
@@ -0,0 +1,28 @@
+"""
+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())

diff  --git a/lldb/test/API/functionalities/pointer_num_children/main.cpp b/lldb/test/API/functionalities/pointer_num_children/main.cpp
new file mode 100644
index 000000000000..a17182092a67
--- /dev/null
+++ b/lldb/test/API/functionalities/pointer_num_children/main.cpp
@@ -0,0 +1,16 @@
+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
+}