[Lldb-commits] [lldb] Make ValueObjectPrinter's handling of its ValueObject pointers more principled (NFC) (PR #81314)
via lldb-commits
lldb-commits at lists.llvm.org
Fri Feb 9 12:29:18 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: None (jimingham)
<details>
<summary>Changes</summary>
I get a small but fairly steady stream of crash reports which I can only explain by ValueObjectPrinter trying to access its m_valobj field, and finding it NULL. I have never been able to reproduce any of these, and the reports show a state too long after the fact to know what went wrong.
I've read through this section of lldb a bunch of times trying to figure out how this could happen, but haven't ever found anything actually wrong that could cause this. OTOH, ValueObjectPrinter is somewhat sloppy about how it handles the ValueObject it is printing.
a) lldb allows you to make a ValueObjectPrinter with a Null incoming ValueObject. However, there's no affordance to set the ValueObject in the Printer after the fact, and it doesn't really make sense to do that. So I change the ValueObjectPrinter API's to take a ValueObject reference, rather than a pointer. All the places that make ValueObjectPrinters already check the non-null status of their ValueObject's before making the ValueObjectPrinter, so sadly, I didn't find the bug, but this will enforce the intent.
b) The next step in printing the ValueObject is deciding which of the associated DynamicValue/SyntheticValue we are actually printing (based on the use_dynamic and use_synthetic settings in the original ValueObject. This was put in a pointer by GetMostSpecializedValue, but most of the printer code just accessed the pointer, and it was hard to reason out whether we were guaranteed to always call this before using m_valobj. So far as I could see we always do (sigh, didn't find the bug there either) but this was way too hard to reason about.
In fact, we figure out once which ValueObject we're going to print and don't change that through the life of the printer. So I changed this to both set the "most specialized value" in the constructor, and then to always access it through GetMostSpecializedValue(). That makes it easier to reason about the use of this ValueObject as well.
This is an NFC change, all it does is make the code easier to reason about.
---
Patch is 29.30 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/81314.diff
5 Files Affected:
- (modified) lldb/include/lldb/DataFormatters/ValueObjectPrinter.h (+25-9)
- (modified) lldb/source/Commands/CommandObjectFrame.cpp (+4-1)
- (modified) lldb/source/Core/ValueObject.cpp (+1-1)
- (modified) lldb/source/DataFormatters/TypeSummary.cpp (+4-1)
- (modified) lldb/source/DataFormatters/ValueObjectPrinter.cpp (+112-110)
``````````diff
diff --git a/lldb/include/lldb/DataFormatters/ValueObjectPrinter.h b/lldb/include/lldb/DataFormatters/ValueObjectPrinter.h
index 2b3936eaa707f2..91963b4d126eba 100644
--- a/lldb/include/lldb/DataFormatters/ValueObjectPrinter.h
+++ b/lldb/include/lldb/DataFormatters/ValueObjectPrinter.h
@@ -22,9 +22,9 @@ namespace lldb_private {
class ValueObjectPrinter {
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;
@@ -39,7 +39,7 @@ 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,
@@ -47,13 +47,27 @@ class ValueObjectPrinter {
// 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();
const char *GetDescriptionForDisplay();
@@ -95,13 +109,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);
@@ -121,8 +135,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.
+ /// Don't use this directly, use
+ /// GetMostSpecializedValue.
Stream *m_stream;
DumpValueObjectOptions m_options;
Flags m_type_flags;
diff --git a/lldb/source/Commands/CommandObjectFrame.cpp b/lldb/source/Commands/CommandObjectFrame.cpp
index 17a7e858b24e58..53886140fd2d6c 100644
--- a/lldb/source/Commands/CommandObjectFrame.cpp
+++ b/lldb/source/Commands/CommandObjectFrame.cpp
@@ -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");
+ ValueObjectPrinter printer(*(valobj_sp.get()), &result.GetOutputStream(),
options);
printer.PrintValueObject();
}
diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index 9208047be36668..e80042826f7d64 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -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();
}
diff --git a/lldb/source/DataFormatters/TypeSummary.cpp b/lldb/source/DataFormatters/TypeSummary.cpp
index c09ed31d0338fd..3707d2d879d33a 100644
--- a/lldb/source/DataFormatters/TypeSummary.cpp
+++ b/lldb/source/DataFormatters/TypeSummary.cpp
@@ -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;
diff --git a/lldb/source/DataFormatters/ValueObjectPrinter.cpp b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
index 074d0b648e6fa9..46e50a8d421a71 100644
--- a/lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -18,39 +18,35 @@
using namespace lldb;
using namespace lldb_private;
-ValueObjectPrinter::ValueObjectPrinter(ValueObject *valobj, Stream *s) {
- if (valobj) {
- DumpValueObjectOptions options(*valobj);
- Init(valobj, s, options, m_options.m_max_ptr_depth, 0, nullptr);
- } else {
- DumpValueObjectOptions options;
- Init(valobj, s, options, m_options.m_max_ptr_depth, 0, nullptr);
- }
+ValueObjectPrinter::ValueObjectPrinter(ValueObject &valobj, Stream *s)
+ : m_orig_valobj(valobj) {
+ DumpValueObjectOptions options(valobj);
+ Init(valobj, s, options, m_options.m_max_ptr_depth, 0, nullptr);
}
-ValueObjectPrinter::ValueObjectPrinter(ValueObject *valobj, Stream *s,
- const DumpValueObjectOptions &options) {
+ValueObjectPrinter::ValueObjectPrinter(ValueObject &valobj, Stream *s,
+ const DumpValueObjectOptions &options)
+ : m_orig_valobj(valobj) {
Init(valobj, s, options, m_options.m_max_ptr_depth, 0, nullptr);
}
ValueObjectPrinter::ValueObjectPrinter(
- ValueObject *valobj, Stream *s, const DumpValueObjectOptions &options,
+ ValueObject &valobj, Stream *s, const DumpValueObjectOptions &options,
const DumpValueObjectOptions::PointerDepth &ptr_depth, uint32_t curr_depth,
- InstancePointersSetSP printed_instance_pointers) {
+ InstancePointersSetSP printed_instance_pointers)
+ : m_orig_valobj(valobj) {
Init(valobj, s, options, ptr_depth, curr_depth, printed_instance_pointers);
}
void ValueObjectPrinter::Init(
- ValueObject *valobj, Stream *s, const DumpValueObjectOptions &options,
+ ValueObject &valobj, Stream *s, const DumpValueObjectOptions &options,
const DumpValueObjectOptions::PointerDepth &ptr_depth, uint32_t curr_depth,
InstancePointersSetSP printed_instance_pointers) {
- m_orig_valobj = valobj;
- m_valobj = nullptr;
+ m_cached_valobj = nullptr;
m_stream = s;
m_options = options;
m_ptr_depth = ptr_depth;
m_curr_depth = curr_depth;
- assert(m_orig_valobj && "cannot print a NULL ValueObject");
assert(m_stream && "cannot print to a NULL Stream");
m_should_print = eLazyBoolCalculate;
m_is_nil = eLazyBoolCalculate;
@@ -68,23 +64,18 @@ void ValueObjectPrinter::Init(
printed_instance_pointers
? printed_instance_pointers
: InstancePointersSetSP(new InstancePointersSet());
+ SetupMostSpecializedValue();
}
bool ValueObjectPrinter::PrintValueObject() {
- if (!m_orig_valobj)
- return false;
-
// If the incoming ValueObject is in an error state, the best we're going to
// get out of it is its type. But if we don't even have that, just print
// the error and exit early.
- if (m_orig_valobj->GetError().Fail()
- && !m_orig_valobj->GetCompilerType().IsValid()) {
- m_stream->Printf("Error: '%s'", m_orig_valobj->GetError().AsCString());
+ if (m_orig_valobj.GetError().Fail() &&
+ !m_orig_valobj.GetCompilerType().IsValid()) {
+ m_stream->Printf("Error: '%s'", m_orig_valobj.GetError().AsCString());
return true;
}
-
- if (!GetMostSpecializedValue() || m_valobj == nullptr)
- return false;
if (ShouldPrintValueObject()) {
PrintLocationIfNeeded();
@@ -107,66 +98,68 @@ bool ValueObjectPrinter::PrintValueObject() {
return true;
}
-bool ValueObjectPrinter::GetMostSpecializedValue() {
- if (m_valobj)
- return true;
- bool update_success = m_orig_valobj->UpdateValueIfNeeded(true);
- if (!update_success) {
- m_valobj = m_orig_valobj;
- } else {
- if (m_orig_valobj->IsDynamic()) {
+ValueObject &ValueObjectPrinter::GetMostSpecializedValue() {
+ assert(m_cached_valobj && "ValueObjectPrinter must have a valid ValueObject");
+ return *m_cached_valobj;
+}
+
+void ValueObjectPrinter::SetupMostSpecializedValue() {
+ bool update_success = m_orig_valobj.UpdateValueIfNeeded(true);
+ // If we can't find anything better, we'll fall back on the original
+ // ValueObject.
+ m_cached_valobj = &m_orig_valobj;
+ if (update_success) {
+ if (m_orig_valobj.IsDynamic()) {
if (m_options.m_use_dynamic == eNoDynamicValues) {
- ValueObject *static_value = m_orig_valobj->GetStaticValue().get();
+ ValueObject *static_value = m_orig_valobj.GetStaticValue().get();
if (static_value)
- m_valobj = static_value;
- else
- m_valobj = m_orig_valobj;
- } else
- m_valobj = m_orig_valobj;
+ m_cached_valobj = static_value;
+ }
} else {
if (m_options.m_use_dynamic != eNoDynamicValues) {
ValueObject *dynamic_value =
- m_orig_valobj->GetDynamicValue(m_options.m_use_dynamic).get();
+ m_orig_valobj.GetDynamicValue(m_options.m_use_dynamic).get();
if (dynamic_value)
- m_valobj = dynamic_value;
- else
- m_valobj = m_orig_valobj;
- } else
- m_valobj = m_orig_valobj;
+ m_cached_valobj = dynamic_value;
+ }
}
- if (m_valobj->IsSynthetic()) {
+ if (m_cached_valobj->IsSynthetic()) {
if (!m_options.m_use_synthetic) {
- ValueObject *non_synthetic = m_valobj->GetNonSyntheticValue().get();
+ ValueObject *non_synthetic =
+ m_cached_valobj->GetNonSyntheticValue().get();
if (non_synthetic)
- m_valobj = non_synthetic;
+ m_cached_valobj = non_synthetic;
}
} else {
if (m_options.m_use_synthetic) {
- ValueObject *synthetic = m_valobj->GetSyntheticValue().get();
+ ValueObject *synthetic = m_cached_valobj->GetSyntheticValue().get();
if (synthetic)
- m_valobj = synthetic;
+ m_cached_valobj = synthetic;
}
}
}
- m_compiler_type = m_valobj->GetCompilerType();
+ m_compiler_type = m_cached_valobj->GetCompilerType();
m_type_flags = m_compiler_type.GetTypeInfo();
- return true;
+ assert(m_cached_valobj &&
+ "SetupMostSpecialized value must compute a valid ValueObject");
}
const char *ValueObjectPrinter::GetDescriptionForDisplay() {
- const char *str = m_valobj->GetObjectDescription();
+ ValueObject &valobj = GetMostSpecializedValue();
+ const char *str = valobj.GetObjectDescription();
if (!str)
- str = m_valobj->GetSummaryAsCString();
+ str = valobj.GetSummaryAsCString();
if (!str)
- str = m_valobj->GetValueAsCString();
+ str = valobj.GetValueAsCString();
return str;
}
const char *ValueObjectPrinter::GetRootNameForDisplay() {
- const char *root_valobj_name = m_options.m_root_valobj_name.empty()
- ? m_valobj->GetName().AsCString()
- : m_options.m_root_valobj_name.c_str();
+ const char *root_valobj_name =
+ m_options.m_root_valobj_name.empty()
+ ? GetMostSpecializedValue().GetName().AsCString()
+ : m_options.m_root_valobj_name.c_str();
return root_valobj_name ? root_valobj_name : "";
}
@@ -181,14 +174,16 @@ bool ValueObjectPrinter::ShouldPrintValueObject() {
bool ValueObjectPrinter::IsNil() {
if (m_is_nil == eLazyBoolCalculate)
- m_is_nil = m_valobj->IsNilReference() ? eLazyBoolYes : eLazyBoolNo;
+ m_is_nil =
+ GetMostSpecializedValue().IsNilReference() ? eLazyBoolYes : eLazyBoolNo;
return m_is_nil == eLazyBoolYes;
}
bool ValueObjectPrinter::IsUninitialized() {
if (m_is_uninit == eLazyBoolCalculate)
- m_is_uninit =
- m_valobj->IsUninitializedReference() ? eLazyBoolYes : eLazyBoolNo;
+ m_is_uninit = GetMostSpecializedValue().IsUninitializedReference()
+ ? eLazyBoolYes
+ : eLazyBoolNo;
return m_is_uninit == eLazyBoolYes;
}
@@ -213,19 +208,20 @@ bool ValueObjectPrinter::IsAggregate() {
bool ValueObjectPrinter::IsInstancePointer() {
// you need to do this check on the value's clang type
+ ValueObject &valobj = GetMostSpecializedValue();
if (m_is_instance_ptr == eLazyBoolCalculate)
- m_is_instance_ptr = (m_valobj->GetValue().GetCompilerType().GetTypeInfo() &
+ m_is_instance_ptr = (valobj.GetValue().GetCompilerType().GetTypeInfo() &
eTypeInstanceIsPointer) != 0
? eLazyBoolYes
: eLazyBoolNo;
- if ((eLazyBoolYes == m_is_instance_ptr) && m_valobj->IsBaseClass())
+ if ((eLazyBoolYes == m_is_instance_ptr) && valobj.IsBaseClass())
m_is_instance_ptr = eLazyBoolNo;
return m_is_instance_ptr == eLazyBoolYes;
}
bool ValueObjectPrinter::PrintLocationIfNeeded() {
if (m_options.m_show_location) {
- m_stream->Printf("%s: ", m_valobj->GetLocationAsCString());
+ m_stream->Printf("%s: ", GetMostSpecializedValue().GetLocationAsCString());
return true;
}
return false;
@@ -244,6 +240,8 @@ void ValueObjectPrinter::PrintDecl() {
(m_curr_depth == 0 && !m_options.m_flat_output);
StreamString typeName;
+ // Figure out which ValueObject we're acting on
+ ValueObject &valobj = GetMostSpecializedValue();
// always show the type at the root level if it is invalid
if (show_type) {
@@ -252,8 +250,8 @@ void ValueObjectPrinter::PrintDecl() {
ConstString type_name;
if (m_compiler_type.IsValid()) {
type_name = m_options.m_use_type_display_name
- ? m_valobj->GetDisplayTypeName()
- : m_valobj->GetQualifiedTypeName();
+ ? valobj.GetDisplayTypeName()
+ : valobj.GetQualifiedTypeName();
} else {
// only show an invalid type name if the user explicitly triggered
// show_type
@@ -277,7 +275,7 @@ void ValueObjectPrinter::PrintDecl() {
if (ShouldShowName()) {
if (m_options.m_flat_output)
- m_valobj->GetExpressionPath(varName);
+ valobj.GetExpressionPath(varName);
else
varName << GetRootNameForDisplay();
}
@@ -289,7 +287,7 @@ void ValueObjectPrinter::PrintDecl() {
// one for the ValueObject
lldb::LanguageType lang_type =
(m_options.m_varformat_language == lldb::eLanguageTypeUnknown)
- ? m_valobj->GetPreferredDisplayLanguage()
+ ? valobj.GetPreferredDisplayLanguage()
: m_options.m_varformat_language;
if (Language *lang_plugin = Language::FindPlugin(lang_type)) {
m_options.m_decl_printing_helper = lang_plugin->GetDeclPrintingHelper();
@@ -327,14 +325,15 @@ void ValueObjectPrinter::PrintDecl() {
bool ValueObjectPrinter::CheckScopeIfNeeded() {
if (m_options.m_scope_already_checked)
return true;
- return m_valobj->IsInScope();
+ return GetMostSpecializedValue().IsInScope();
}
TypeSummaryImpl *ValueObjectPrinter::GetSummaryFormatter(bool null_if_omitted) {
if (!m_summary_formatter.second) {
- TypeSummaryImpl *entry = m_options.m_summary_sp
- ? m_options.m_summary_sp.get()
- : m_valobj->GetSummaryFormat().get();
+ TypeSummaryImpl *entry =
+ m_options.m_summary_sp
+ ? m_options.m_summary_sp.get()
+ : GetMostSpecializedValue().GetSummaryFormat().get();
if (m_options.m_omit_summary_depth > 0)
entry = nullptr;
@@ -357,18 +356,19 @@ void ValueObjectPrinter::GetValueSummaryError(std::string &value,
std::string &summary,
std::string &error) {
lldb::Format format = m_options.m_format;
+ ValueObject &valobj = GetMostSpecializedValue();
// if I am printing synthetized elements, apply the format to those elements
// only
if (m_options.m_pointer_as_array)
- m_valobj->GetValueAsCString(lldb::eFormatDefault, value);
- else if (format != eFormatDefault && format != m_valobj->GetFormat())
- m_valobj->GetValueAsCString(format, value);
+ valobj.GetValueAsCString(lldb::eFormatDefault, value);
+ else if (format != eFormatDefault && format != valobj.GetFormat())
+ valobj.GetValueAsCString(format, value);
else {
- const char *val_cstr = m_valobj->GetValueAsCString();
+ const char *val_cstr = valobj.GetValueAsCString();
if (val_cstr)
value.assign(val_cstr);
}
- const char *err_cstr = m_valobj->GetError().AsCString();
+ const char *err_cstr = valobj.GetError().AsCString();
if (err_cstr)
error.assign(err_cstr);
@@ -378,7 +378,7 @@ void ValueObjectPrinter::GetValueSummaryError(std::string &value,
if (IsNil()) {
lldb::LanguageType lang_type =
(m_options.m_varformat_language == lldb::eLanguageTypeUnknown)
- ? m_valobj->GetPreferredDisplayLanguage()
+ ? valobj.GetPreferredDisplayLanguage()
: m_options.m_varformat_language;
if (Language *lang_plugin = Language::FindPlugin(lang_type)) {
summary.assign(lang_plugin->GetNilReferenceSummaryString().str());
@@ -392,11 +392,11 @@ void ValueObjectPrinter::GetValueSummaryError(std::string &value,
} else if (m_options.m_omit_summary_depth == 0) {
TypeSummaryImpl *entry = GetSummaryFormatter();
if (entry) {
- m_valobj->GetSummaryAsCString(entry, summary,
- m_options.m_varformat_language);
+ valobj.GetSummaryAsCString(entry, summary,
+ m_options.m_varformat_language);
} else {
const char *sum_cstr =
- m_valobj->GetSummaryAsCString(m_options.m_varformat_language);
+ valobj.GetSummaryAsCString(m_options.m_varformat_language);
if (sum_cstr)
summary.assign(sum_cstr);
}
@@ -431,16 +431,17 @@ bool ValueObjectPrinter::PrintValueAndSummaryIfNeeded(bool &value_printed,
// this thing is nil (but show the value if the user passes a format
// explicitly)
TypeSummaryImpl *entry = GetSummaryFormatter();
+ ValueObject &valobj = GetMostSpecializedValue();
const bool has_nil_or_uninitialized_summary =
(IsNil() || IsUninitialized()) && !m_summary.empty();
if (!has_nil_or_uninitialized_summary && !m_value.empty() &&
(entry == nullptr ||
- (entry->DoesPrintValue(m_valobj) ||
+ (entry->Does...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/81314
More information about the lldb-commits
mailing list