Make ValueObjectPrinter's handling of its ValueObject pointers more principled (NFC) #81314
Conversation
make that not possible. Also, be more principled about ensuring that the ValueObject we choose to print from the original ValueObject can't get set to null.
jimingham
requested review from
clayborg and
kastiglione
and removed request for
JDevlieghere
|
@llvm/pr-subscribers-lldb Author: None (jimingham) ChangesI 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. 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:
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]
|
There was a problem hiding this comment.
See inlined comments, but maybe we need to start storing a std::weak_ptr<ValueObject> instead of raw pointers to things, and possibly always passsing a ValueObjectSP object into the ValueObjectPrinter so it holds a strong reference to the value object while printing. I could see dead pointers causing problems and crashes.
It would be nice to also not crash, right now we do things like:
assert(valobj);
valobj->CrashNow();
If we use std::weak_ptr<ValueObject> instead of raw pointers or references, we can then call the bool std::weak_ptr<T>::expired() const function to see if the weak pointer was valid before, but the main shared pointer keeping it alive has been freed. This will allow us to detect issues where our backing object has been freed.
| ValueObject *m_orig_valobj; | ||
| ValueObject *m_valobj; | ||
| ValueObject &m_orig_valobj; | ||
| ValueObject *m_cached_valobj; /// Cache the current "most specialized" value. |
There was a problem hiding this comment.
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.
| 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"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| DumpValueObjectOptions options; | ||
| Init(valobj, s, options, m_options.m_max_ptr_depth, 0, nullptr); | ||
| } | ||
| ValueObjectPrinter::ValueObjectPrinter(ValueObject &valobj, Stream *s) |
There was a problem hiding this comment.
Would it make sense to pass in a "ValueObjectSP" here instead? Could some of our crashes be due to someone creating a ValueObjectPrinter and then somehow the ValueObject gets freed behind out backs?
There was a problem hiding this comment.
For this series of crashes, the EXC_BAD_ACCESS address is the correct offset for the ivar being accessed. So the address of the ValueObject pointer has to be 0x0. If the pointer had gotten freed, the printer's pointer value wouldn't change, so the access would just be some random looking address.
Also, see the more general comment about uses of the ValueObjectPrinter.
|
Replying to a bunch of Greg's comments in sum. The way we use the ValueObjectPrinter is that you have a ValueObject and want to print it, so you make a ValueObjectPrinter passing in that ValueObject, then tell it to print it, then that ValueObjectPrinter is done. We don't preserve ValueObjectPrinters or try to reuse them. There's no attempt to track changes in the ValueObject, for instance if you changed the use dynamic setting of the underlying ValueObject that won't effect the ValueObjectPrinter you made before you changed the setting. It doesn't track the SyntheticChildProviderGeneration, so if you changed synthetic child providers on that ValueObject, it wouldn't track that, etc. I think that's a useful simplification. We don't have to reason about lifecycles in the ValueObjectPrinter. The caller is responsible for the lifecycle of the ValueObject being printed - and that ValueObject's ClusterManager is responsible for all the children & synthetic children of that ValueObject. The ValueObjectPrinter gets a reference to it because it expects to do some work on the ValueObject and then get discarded, which is how we always use it. I can put in a comment to this effect, though the fact that we're taking in a Another way to finesse this is to note that the only thing people do with ValueObjectPrinters is: { or in one case PrintValueObjectOneLine is used instead. So we could just have the ValueObjectPrinter have no public methods but the constructor, and print the ValueObject in the constructor. That way it would be 100% clear these are one use objects. I didn't do it that way because I don't think we have a lot of places where so much of the work is done in the constructor. |
Also don't fumble-finger delete the declaration of SetupMostSpecializedValue.
You can test this locally with the following command:git-clang-format --diff ff8c865838b46d0202963b816fbed50aaf96a7f4 08ab7f13600b848d79dfdd5515402a22aa996bc0 -- lldb/include/lldb/DataFormatters/ValueObjectPrinter.h lldb/source/Commands/CommandObjectFrame.cpp lldb/source/Core/ValueObject.cpp lldb/source/DataFormatters/TypeSummary.cpp lldb/source/DataFormatters/ValueObjectPrinter.cppView the diff from clang-format here.diff --git a/lldb/source/Commands/CommandObjectFrame.cpp b/lldb/source/Commands/CommandObjectFrame.cpp
index a4d3fb66e8..9f4b9fca3a 100644
--- a/lldb/source/Commands/CommandObjectFrame.cpp
+++ b/lldb/source/Commands/CommandObjectFrame.cpp
@@ -180,8 +180,7 @@ protected:
// 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, &result.GetOutputStream(),
- options);
+ ValueObjectPrinter printer(*valobj_sp, &result.GetOutputStream(), options);
printer.PrintValueObject();
}
|
|
Note, these really are transitory objects and need to be quick to make and delete because in the course of printing a complex object we make a ValueObjectPrinter for each node we print. So we're making a lot of them as we descend and then directly throwing them away when we're done. So it makes sense to treat them as transitory in all their uses, IMO. |
|
None of what I suggested was mandatory. I was mainly trying to make sure things don't go away on us without us knowing about it. If we have crashes it is always hard to tell if simple heap corruption is the issue or if we are having lifetime issues. If you are not concerned about lifetime issues then you can ignore the Running a lldb with libgmalloc is a good way to watch for pointer lifetime issues and things being freed if someone ever has a repro case that they add to a bug. |
|
I don't think this is a lifetime issue. If it were I would see more of the crashes at random addresses.
Whatever's going on is something odd, so I'm just trying to make the code easier to reason about. I think in the ValueObjectPrinter's case, it's code will be cleaner if it expects to be used as it is currently used, as a one shot printer that is not responsible for the lifecycle of the ValueObject it's printing. That was the original intent; I'm not really changing that. I just got frustrated while trying to find the bug by inspection. I kept going "ooh, there's an unchecked use of m_valobj, I wonder if that could cause problems?" only to follow all the paths through the code and end up convincing myself that you couldn't get here with a NULL pointer, so that wasn't the problem. If we make it so internally to ValueObjectPrinter you can't get your hands on something that might be null, and I see another of these crashes, I'll be able to just look outside ValueObjectPrinter for the answer.
Jim
… On Feb 9, 2024, at 2:56 PM, Greg Clayton ***@***.***> wrote:
None of what I suggested was mandatory. I was mainly trying to make sure things don't go away on us without us knowing about it. If we have crashes it is always hard to tell if simple heap corruption is the issue or if we are having lifetime issues. If you are not concerned about lifetime issues then you can ignore the std::weak_ptr<T> suggestion, but if you are, this would be a good way to tell as we can check std::weak_ptr<T>::expired().
Running a lldb with libgmalloc is a good way to watch for pointer lifetime issues and things being freed if someone ever has a repro case that they add to a bug.
—
Reply to this email directly, view it on GitHub <#81314 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWZQBEBTGPQVNJFPZN3YS2SQFAVCNFSM6AAAAABDCBDGDSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZWG4YTCMBTHA>.
You are receiving this because you authored the thread.
|
|
The other thing I'm trying to eliminate is problem arising from the way the old code started with |
…rincipled (NFC) (llvm#81314) 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. (cherry picked from commit c2b01c8)
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.