Skip to content

[lldb] Update dwim-print to show expanded objc instances #117500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lldb/include/lldb/DataFormatters/DumpValueObjectOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ class DumpValueObjectOptions {

DumpValueObjectOptions &SetRevealEmptyAggregates(bool reveal = true);

DumpValueObjectOptions &SetExpandPointerTypeFlags(unsigned flags);

DumpValueObjectOptions &SetElementCount(uint32_t element_count = 0);

DumpValueObjectOptions &
Expand All @@ -142,6 +144,7 @@ class DumpValueObjectOptions {
DeclPrintingHelper m_decl_printing_helper;
ChildPrintingDecider m_child_printing_decider;
PointerAsArraySettings m_pointer_as_array;
unsigned m_expand_ptr_type_flags = 0;
bool m_use_synthetic : 1;
bool m_scope_already_checked : 1;
bool m_flat_output : 1;
Expand Down
3 changes: 2 additions & 1 deletion lldb/source/Commands/CommandObjectDWIMPrint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ void CommandObjectDWIMPrint::DoExecute(StringRef command,

DumpValueObjectOptions dump_options = m_varobj_options.GetAsDumpOptions(
m_expr_options.m_verbosity, m_format_options.GetFormat());
dump_options.SetHideRootName(suppress_result);
dump_options.SetHideRootName(suppress_result)
.SetExpandPointerTypeFlags(lldb::eTypeIsObjC);
Copy link
Member

@Michael137 Michael137 Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something we would want for frame var/expr too? In which case we could just check for ObjC in ValueObjectPrinter::ShouldPrintChildren directly instead of introducing this new API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it definitely should be considered, but I didn't want to impede this change with those possibly more debatable changes. I have more thoughts on this, I'll write them up later today or this week.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my thinking: most of the time, I want top level pointers to be expanded (as c++ references are). But each of the C based language has its considerations.

For ObjC, I think all top level objects should be expanded, and I can't think of any counter arguments.

For C++, I also want top level pointers to be expanded – however raw pointers are less common. In C++ I would like to see top level smart pointers be expanded.

For C, I am less sure what, if anything, to do about top level pointers. First, I don't write a lot of C and have less experience to form opinions. Second, in C a pointer might be just as likely to be a buffer/array, and expanding a buffer as a single pointer would be misleading.

Coming back to your question: I would like to see C++ raw pointers expanded, and hopefully smart pointers too. I would leave C pointers alone. I guess I need to raise this on the forums, and find some buy in. As part of that, we can determine should pointer expansion be specific to dwim-print, or should some/all of it apply to frame var/expr too.

Copy link
Member

@Michael137 Michael137 Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually sure what I'd prefer for C++. Usually when I print a pointer I only what to see the pointer value, not it's dereferenced form. But maybe that's because I've gotten used to the LLDB CLI and the way frame var works.

This thread here might be relevant: #117755 (review)

Limiting this change to ObjC for now seems fine to me. But yea, asking a wider audience might be a good idea.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something we would want for frame var/expr too? In which case we could just check for ObjC in ValueObjectPrinter::ShouldPrintChildren directly instead of introducing this new API

Should/Could this perhaps be an (NSObject *) data formatter, doing this work, so it's applied consistently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adrian-prantl I tried this (with a Python implementation) and that didn't work for two reasons:

  1. Setting a synthetic for NSObject * did not result in that formatter being used with Subclass *
  2. An objc pointer is still not expanded, thus the production of children doesn't get triggered unless I explicitly expand the pointer with v -P1 <variable>


bool is_po = m_varobj_options.use_objc;

Expand Down
6 changes: 6 additions & 0 deletions lldb/source/DataFormatters/DumpValueObjectOptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,12 @@ DumpValueObjectOptions::SetRevealEmptyAggregates(bool reveal) {
return *this;
}

DumpValueObjectOptions &
DumpValueObjectOptions::SetExpandPointerTypeFlags(unsigned flags) {
m_expand_ptr_type_flags = flags;
return *this;
}

DumpValueObjectOptions &
DumpValueObjectOptions::SetElementCount(uint32_t element_count) {
m_pointer_as_array = PointerAsArraySettings(element_count);
Expand Down
14 changes: 8 additions & 6 deletions lldb/source/DataFormatters/ValueObjectPrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -554,12 +554,14 @@ bool ValueObjectPrinter::ShouldPrintChildren(
return false;

const bool is_root_level = m_curr_depth == 0;

if (is_ref && is_root_level && print_children) {
// If this is the root object (depth is zero) that we are showing and
// it is a reference, and no pointer depth has been supplied print out
// what it references. Don't do this at deeper depths otherwise we can
// end up with infinite recursion...
const bool is_expanded_ptr =
is_ptr && m_type_flags.Test(m_options.m_expand_ptr_type_flags);

if ((is_ref || is_expanded_ptr) && is_root_level && print_children) {
// If this is the root object (depth is zero) that we are showing and it
// is either a reference or a preferred type of pointer, then print it.
// Don't do this at deeper depths otherwise we can end up with infinite
// recursion...
return true;
}

Expand Down
3 changes: 3 additions & 0 deletions lldb/test/API/commands/dwim-print/objc/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
OBJC_SOURCES := main.m
LD_EXTRAS := -framework Foundation
include Makefile.rules
27 changes: 27 additions & 0 deletions lldb/test/API/commands/dwim-print/objc/TestDWIMPrintObjC.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
"""
Test dwim-print with objc instances.
"""

import lldb
from lldbsuite.test.lldbtest import *
from lldbsuite.test.decorators import *
import lldbsuite.test.lldbutil as lldbutil


class TestCase(TestBase):
@skipUnlessDarwin
def test(self):
self.build()
lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.m"))
self.expect("dwim-print parent", substrs=["_child = 0x"])
self.expect(
"dwim-print parent.child", patterns=[r'_name = 0x[0-9a-f]+ @"Seven"']
)

@skipUnlessDarwin
def test_with_summary(self):
self.build()
lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.m"))
self.runCmd("type summary add -s 'Parent of ${var._child._name}' 'Parent *'")
self.expect("dwim-print parent", matching=False, substrs=["_child = 0x"])
self.expect("dwim-print parent", substrs=['Parent of @"Seven"'])
24 changes: 24 additions & 0 deletions lldb/test/API/commands/dwim-print/objc/main.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#import <Foundation/Foundation.h>

@interface Child : NSObject
@property(nonatomic, copy) NSString *name;
@end

@implementation Child
@end

@interface Parent : NSObject
@property(nonatomic, strong) Child *child;
@end

@implementation Parent
@end

int main(int argc, char **argv) {
Child *child = [Child new];
child.name = @"Seven";
Parent *parent = [Parent new];
parent.child = child;
puts("break here");
return 0;
}