Skip to content

Make ValueObjectPrinter's handling of its ValueObject pointers more principled (NFC) #81314

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.

Sign up for GitHub

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
merged 3 commits into from
Feb 12, 2024
Merged

Make ValueObjectPrinter's handling of its ValueObject pointers more principled (NFC) #81314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d24f11
It makes no sense to make a ValueObjectPrinter with a null ValueObject,
jimingham Feb 9, 2024
dc2c387
Change to use the shared ptr "*" operator.
jimingham Feb 9, 2024
08ab7f1
Add a comment describing ValueObjectPrinter's intended use.
jimingham Feb 9, 2024
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
41 changes: 32 additions & 9 deletions lldb/include/lldb/DataFormatters/ValueObjectPrinter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,15 @@
namespace lldb_private {

class ValueObjectPrinter {
/// The ValueObjectPrinter is a one-shot printer for ValueObjects. It
/// does not retain the ValueObject it is printing, that is the job of
/// its caller. It also doesn't attempt to track changes in the
/// ValueObject, e.g. changing synthetic child providers or changing
/// dynamic vrs. static vrs. synthetic settings.
public:
ValueObjectPrinter(ValueObject *valobj, Stream *s);
ValueObjectPrinter(ValueObject &valobj, Stream *s);

ValueObjectPrinter(ValueObject *valobj, Stream *s,
ValueObjectPrinter(ValueObject &valobj, Stream *s,
const DumpValueObjectOptions &options);

~ValueObjectPrinter() = default;
Expand All @@ -39,21 +44,37 @@ class ValueObjectPrinter {

// only this class (and subclasses, if any) should ever be concerned with the
// depth mechanism
ValueObjectPrinter(ValueObject *valobj, Stream *s,
ValueObjectPrinter(ValueObject &valobj, Stream *s,
const DumpValueObjectOptions &options,
const DumpValueObjectOptions::PointerDepth &ptr_depth,
uint32_t curr_depth,
InstancePointersSetSP printed_instance_pointers);

// we should actually be using delegating constructors here but some versions
// of GCC still have trouble with those
void Init(ValueObject *valobj, Stream *s,
void Init(ValueObject &valobj, Stream *s,
const DumpValueObjectOptions &options,
const DumpValueObjectOptions::PointerDepth &ptr_depth,
uint32_t curr_depth,
InstancePointersSetSP printed_instance_pointers);

bool GetMostSpecializedValue();
/// Cache the ValueObject we are actually going to print. If this
/// ValueObject has a Dynamic type, we return that, if either the original
/// ValueObject or its Dynamic type has a Synthetic provider, return that.
/// This will never return an empty ValueObject, since we use the ValueObject
/// to carry errors.
/// Note, this gets called when making the printer object, and uses the
/// use dynamic and use synthetic settings of the ValueObject being printed,
/// so changes made to these settings won't affect already made
/// ValueObjectPrinters. SetupMostSpecializedValue();

/// Access the cached "most specialized value" - that is the one to use for
/// printing the value object's value. However, be sure to use
/// GetValueForChildGeneration when you are generating the children of this
/// value.
ValueObject &GetMostSpecializedValue();

void SetupMostSpecializedValue();

const char *GetDescriptionForDisplay();

Expand Down Expand Up @@ -95,13 +116,13 @@ class ValueObjectPrinter {

bool ShouldExpandEmptyAggregates();

ValueObject *GetValueObjectForChildrenGeneration();
ValueObject &GetValueObjectForChildrenGeneration();

void PrintChildrenPreamble(bool value_printed, bool summary_printed);

void PrintChildrenPostamble(bool print_dotdotdot);

lldb::ValueObjectSP GenerateChild(ValueObject *synth_valobj, size_t idx);
lldb::ValueObjectSP GenerateChild(ValueObject &synth_valobj, size_t idx);

void PrintChild(lldb::ValueObjectSP child_sp,
const DumpValueObjectOptions::PointerDepth &curr_ptr_depth);
Expand All @@ -121,8 +142,10 @@ class ValueObjectPrinter {
private:
bool ShouldShowName() const;

ValueObject *m_orig_valobj;
ValueObject *m_valobj;
ValueObject &m_orig_valobj;
ValueObject *m_cached_valobj; /// Cache the current "most specialized" value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should start using std::weak_ptr<ValueObject> instead of raw pointers. Someone is storing a shared pointer to the object somewhere in the value object tree, so having a weak pointer could help us to catch these things as we can ask if (m_cached_valobj_wp.expired() == true) and this would tell us that this was assigned to a valid object at one point, but it has been freed. The expired call will return false if the weak pointer was never set or if the weak pointer was cleared.

/// Don't use this directly, use
/// GetMostSpecializedValue.
Stream *m_stream;
DumpValueObjectOptions m_options;
Flags m_type_flags;
Expand Down
5 changes: 4 additions & 1 deletion lldb/source/Commands/CommandObjectFrame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,10 @@ class CommandObjectFrameDiagnose : public CommandObjectParsed {

DumpValueObjectOptions options;
options.SetDeclPrintingHelper(helper);
ValueObjectPrinter printer(valobj_sp.get(), &result.GetOutputStream(),
// We've already handled the case where the value object sp is null, so
// this is just to make sure future changes don't skip that:
assert(valobj_sp.get() && "Must have a valid ValueObject to print");
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is fine to assert here, but we should not crash if valobj_sp is NULL. I would rather log or report an error that crash if possible.

Copy link
Collaborator Author

@jimingham jimingham Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm of two minds about this. The current code does:

if (!valobj_sp) {
// do some error handling
return;
}
// enough code (using valobj_sp) to push this check off the top of your monitor screen

Then this code. So I could have just duplicated the check & error handling again before printing the thing, but that looked bogus to me. OTOH, I don't want someone to delete the check for some reason, so I put in the assert. I'm really defending against one of us removing a necessary check.

But if that bugs you I'm happy to cut and paste the error handling here again. It just looked weird to me.

ValueObjectPrinter printer(*valobj_sp, &result.GetOutputStream(),
options);
printer.PrintValueObject();
}
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Core/ValueObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2525,7 +2525,7 @@ ValueObjectSP ValueObject::GetValueForExpressionPath_Impl(
void ValueObject::Dump(Stream &s) { Dump(s, DumpValueObjectOptions(*this)); }

void ValueObject::Dump(Stream &s, const DumpValueObjectOptions &options) {
ValueObjectPrinter printer(this, &s, options);
ValueObjectPrinter printer(*this, &s, options);
printer.PrintValueObject();
}

Expand Down
5 changes: 4 additions & 1 deletion lldb/source/DataFormatters/TypeSummary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,10 @@ bool StringSummaryFormat::FormatObject(ValueObject *valobj, std::string &retval,
sc = frame->GetSymbolContext(lldb::eSymbolContextEverything);

if (IsOneLiner()) {
ValueObjectPrinter printer(valobj, &s, DumpValueObjectOptions());
// We've already checked the case of a NULL valobj above. Let's put in an
// assert here to make sure someone doesn't take that out:
assert(valobj && "Must have a valid ValueObject to summarize");
ValueObjectPrinter printer(*valobj, &s, DumpValueObjectOptions());
printer.PrintChildrenOneLiner(HideNames(valobj));
retval = std::string(s.GetString());
return true;
Expand Down
Loading