Page MenuHomePhabricator

Prevent GetNumChildren from transitively walking pointer chains
ClosedPublic

Authored by jarin on May 19 2020, 3:32 PM.

Details

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.

Diff Detail

Event Timeline

jarin created this revision.May 19 2020, 3:32 PM
shafik added a subscriber: shafik.May 19 2020, 4:35 PM
shafik added inline comments.
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 updated this revision to Diff 265141.May 19 2020, 10:55 PM
jarin marked 2 inline comments as done.

Added more assertions, reformatted the test code.

jarin marked an inline comment as not done.May 19 2020, 10:56 PM
jarin added inline comments.
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
5217
teemperor accepted this revision.May 20 2020, 2:18 AM
teemperor added subscribers: labath, clayborg.

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")
This revision is now accepted and ready to land.May 20 2020, 2:18 AM

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 updated this revision to Diff 265237.May 20 2020, 7:29 AM
jarin marked 3 inline comments as done.

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

jarin added a comment.May 20 2020, 7:32 AM

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 added inline comments.May 20 2020, 8:18 AM
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
5217

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

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?

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 accepted this revision.May 22 2020, 3:35 AM

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 accepted this revision.May 22 2020, 10:06 PM

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

Raphael, could you land this for me?

This revision was automatically updated to reflect the committed changes.