Make ValueObjectPrinter's handling of its ValueObject pointers more principled (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.

Author: jimingham

lldb/include/lldb/DataFormatters/ValueObjectPrinter.h

@@ -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;
@@ -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();
 
@@ -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);
@@ -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.
+                                /// Don't use this directly, use
+                                /// GetMostSpecializedValue.
   Stream *m_stream;
   DumpValueObjectOptions m_options;
   Flags m_type_flags;

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, &result.GetOutputStream(),
                                options);
     printer.PrintValueObject();
   }

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();
 }
 

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;