[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG83bd2c4a0680: Prevent GetNumChildren from transitively 
walking pointer chains (authored by jarin, committed by teemperor).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  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

Index: lldb/test/API/functionalities/pointer_num_children/main.cpp
===
--- /dev/null
+++ 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 **Ptr = &(outer.inner);
+  Inner * = outer.inner;
+  return 0; // break here
+}
Index: lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py
===
--- /dev/null
+++ 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())
Index: lldb/test/API/functionalities/pointer_num_children/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/pointer_num_children/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5172,12 +5172,15 @@
 }
 break;
 
+  case clang::Type::LValueReference:
+  case clang::Type::RValueReference:
   case clang::Type::ObjCObjectPointer: {
-const clang::ObjCObjectPointerType *pointer_type =
-llvm::cast(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 @@
 const clang::PointerType *pointer_type =
 llvm::cast(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 @@
   num_children = num_pointee_children;
   } break;
 
-  case clang::Type::LValueReference:
-  case clang::Type::RValueReference: {
-const clang::ReferenceType *reference_type =
-llvm::cast(qual_type.getTypePtr());
-clang::QualType pointee_type = reference_type->getPointeeType();
-uint32_t num_pointee_children =
-

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin added a comment.

Raphael, could you land this for me?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:5215
+uint32_t num_pointee_children = 0;
+if (pointee_clang_type.IsAggregateType())
+  num_pointee_children =

jarin wrote:
> shafik wrote:
> > I am curious what cases are pointers aggregates? Do we test this case?
> 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 ]].
Apologies, I misread that as `pointer_clang_type` , makes more sense now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin updated this revision to Diff 265237.
jarin marked 3 inline comments as done.
jarin added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  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

Index: lldb/test/API/functionalities/pointer_num_children/main.cpp
===
--- /dev/null
+++ 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 **Ptr = &(outer.inner);
+  Inner * = outer.inner;
+  return 0; // break here
+}
Index: lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py
===
--- /dev/null
+++ 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())
Index: lldb/test/API/functionalities/pointer_num_children/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/pointer_num_children/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5172,12 +5172,15 @@
 }
 break;
 
+  case clang::Type::LValueReference:
+  case clang::Type::RValueReference:
   case clang::Type::ObjCObjectPointer: {
-const clang::ObjCObjectPointerType *pointer_type =
-llvm::cast(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 @@
 const clang::PointerType *pointer_type =
 llvm::cast(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 @@
   num_children = num_pointee_children;
   } break;
 
-  case clang::Type::LValueReference:
-  case clang::Type::RValueReference: {
-const clang::ReferenceType *reference_type =
-llvm::cast(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 

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin added a comment.

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`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.)




Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:5228
   case clang::Type::RValueReference: {
 const clang::ReferenceType *reference_type =
 llvm::cast(qual_type.getTypePtr());

teemperor wrote:
> 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.
+1 for merging this stuff



Comment at: lldb/test/API/functionalities/pointer_num_children/main.cpp:13-14
+  Outer outer{};
+  auto Ptr = &(outer.inner);
+  auto  = outer.inner;
+  return 0; // break here

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added subscribers: labath, clayborg.
teemperor added a comment.
This revision is now accepted and ready to land.

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




Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:5228
   case clang::Type::RValueReference: {
 const clang::ReferenceType *reference_type =
 llvm::cast(qual_type.getTypePtr());

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.



Comment at: 
lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py:1
+"""
+"""

I guess this was just a placeholder?



Comment at: 
lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py:16
+(_, _, thread, _) = lldbutil.run_to_source_breakpoint(self, "// break 
here", self.main_source_file)
+frame = thread.GetSelectedFrame()
+

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).

```
lang=python
lldbutil.run_to_source_breakpoint(self, "// break here", 
lldb.SBFileSpec("main.cpp"))

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin marked an inline comment as not done.
jarin added inline comments.



Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:5215
+uint32_t num_pointee_children = 0;
+if (pointee_clang_type.IsAggregateType())
+  num_pointee_children =

shafik wrote:
> I am curious what cases are pointers aggregates? Do we test this case?
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 ]].


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin updated this revision to Diff 265141.
jarin marked 2 inline comments as done.
jarin added a comment.

Added more assertions, reformatted the test code.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  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

Index: lldb/test/API/functionalities/pointer_num_children/main.cpp
===
--- /dev/null
+++ 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{};
+  auto Ptr = &(outer.inner);
+  auto  = outer.inner;
+  return 0; // break here
+}
Index: lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py
===
--- /dev/null
+++ lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py
@@ -0,0 +1,28 @@
+"""
+"""
+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.main_source_file = lldb.SBFileSpec("main.cpp")
+self.build()
+(_, _, thread, _) = lldbutil.run_to_source_breakpoint(self, "// break here", self.main_source_file)
+frame = thread.GetSelectedFrame()
+
+result = 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 = 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())
Index: lldb/test/API/functionalities/pointer_num_children/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/pointer_num_children/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5173,11 +5173,12 @@
 break;
 
   case clang::Type::ObjCObjectPointer: {
-const clang::ObjCObjectPointerType *pointer_type =
-llvm::cast(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 +5210,11 @@
 const clang::PointerType *pointer_type =
 llvm::cast(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
@@ -5223,9 +5227,11 @@
   case clang::Type::RValueReference: {
 const clang::ReferenceType *reference_type =
 llvm::cast(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);
+CompilerType pointee_clang_type = GetType(reference_type->getPointeeType());
+uint32_t num_pointee_children = 0;
+if (pointee_clang_type.IsAggregateType())
+  num_pointee_children =
+

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:5215
+uint32_t num_pointee_children = 0;
+if (pointee_clang_type.IsAggregateType())
+  num_pointee_children =

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



Comment at: 
lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py:20
+self.assertEqual("42", 
result.GetChildAtIndex(0).GetChildAtIndex(0).GetValue())
+self.assertEqual(1, result.GetNumChildren())
+

We should also check

```
result.GetChildAtIndex(0).GetNumChildren()
```

same below


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80254/new/

https://reviews.llvm.org/D80254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin created this revision.
jarin added reviewers: teemperor, jingham.
jarin added a project: LLDB.
Herald added a subscriber: lldb-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80254

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  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

Index: lldb/test/API/functionalities/pointer_num_children/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/pointer_num_children/main.cpp
@@ -0,0 +1,19 @@
+struct Inner
+{
+  int a;
+  int b;
+};
+
+struct Outer
+{
+  Inner *inner;
+};
+
+int main()
+{
+  Inner inner{42, 56};
+  Outer outer{};
+  auto Ptr = &(outer.inner);
+  auto& Ref = outer.inner;
+  return 0;  // break here
+}
Index: lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py
===
--- /dev/null
+++ lldb/test/API/functionalities/pointer_num_children/TestPointerNumChildren.py
@@ -0,0 +1,24 @@
+"""
+"""
+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.main_source_file = lldb.SBFileSpec("main.cpp")
+self.build()
+(_, _, thread, _) = lldbutil.run_to_source_breakpoint(self, "// break here", self.main_source_file)
+frame = thread.GetSelectedFrame()
+
+result = frame.FindVariable("Ref")
+self.assertEqual("42", result.GetChildAtIndex(0).GetChildAtIndex(0).GetValue())
+self.assertEqual(1, result.GetNumChildren())
+
+result = frame.FindVariable("Ptr")
+self.assertEqual("42", result.GetChildAtIndex(0).GetChildAtIndex(0).GetValue())
+self.assertEqual(1, result.GetNumChildren())
Index: lldb/test/API/functionalities/pointer_num_children/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/pointer_num_children/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -5173,11 +5173,12 @@
 break;
 
   case clang::Type::ObjCObjectPointer: {
-const clang::ObjCObjectPointerType *pointer_type =
-llvm::cast(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 +5210,11 @@
 const clang::PointerType *pointer_type =
 llvm::cast(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