From e8a2fd5e7be23e4087e7ff58efa354dfae1a17c4 Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham@apple.com>
Date: Tue, 28 Feb 2023 16:38:50 -0800
Subject: [PATCH] An SBValue whose underlying ValueObject has no valid value,
 but does hold an error should:

(a) return false for IsValid, since that's the current behavior and is
    a convenient way to check "should I get the value for this".
(b) preserve the error when an SBValue is made from it, and print the
    error in the ValueObjectPrinter.

Make that happen.

Differential Revision: https://reviews.llvm.org/D144664
---
 lldb/source/API/SBValue.cpp                       | 11 ++++++++++-
 lldb/source/Core/ValueObject.cpp                  | 11 +++++++++++
 lldb/source/DataFormatters/ValueObjectPrinter.cpp | 12 ++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index f9e03172a4d0..ecd16ccf83a8 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -114,6 +114,10 @@ public:
     lldb::ValueObjectSP value_sp = m_valobj_sp;
 
     Target *target = value_sp->GetTargetSP().get();
+    // If this ValueObject holds an error, then it is valuable for that.
+    if (value_sp->GetError().Fail()) 
+      return value_sp;
+
     if (!target)
       return ValueObjectSP();
 
@@ -1047,7 +1051,12 @@ lldb::SBFrame SBValue::GetFrame() {
 }
 
 lldb::ValueObjectSP SBValue::GetSP(ValueLocker &locker) const {
-  if (!m_opaque_sp || !m_opaque_sp->IsValid()) {
+  // IsValid means that the SBValue has a value in it.  But that's not the
+  // only time that ValueObjects are useful.  We also want to return the value
+  // if there's an error state in it.
+  if (!m_opaque_sp || (!m_opaque_sp->IsValid() 
+      && (m_opaque_sp->GetRootSP() 
+          && !m_opaque_sp->GetRootSP()->GetError().Fail()))) {
     locker.GetError().SetErrorString("No value");
     return ValueObjectSP();
   }
diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index 38645b087435..6e79a04d024e 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -1173,6 +1173,15 @@ bool ValueObject::DumpPrintableRepresentation(
     Stream &s, ValueObjectRepresentationStyle val_obj_display,
     Format custom_format, PrintableRepresentationSpecialCases special,
     bool do_dump_error) {
+    
+  // If the ValueObject has an error, we might end up dumping the type, which
+  // is useful, but if we don't even have a type, then don't examine the object
+  // further as that's not meaningful, only the error is.
+  if (m_error.Fail() && !GetCompilerType().IsValid()) {
+    if (do_dump_error)
+      s.Printf("<%s>", m_error.AsCString());
+    return false;
+  }
 
   Flags flags(GetTypeInfo());
 
@@ -1374,6 +1383,8 @@ bool ValueObject::DumpPrintableRepresentation(
     if (!str.empty())
       s << str;
     else {
+      // We checked for errors at the start, but do it again here in case
+      // realizing the value for dumping produced an error.
       if (m_error.Fail()) {
         if (do_dump_error)
           s.Printf("<%s>", m_error.AsCString());
diff --git a/lldb/source/DataFormatters/ValueObjectPrinter.cpp b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
index c4221a5ac7cc..17e9877bd524 100644
--- a/lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -71,6 +71,18 @@ void ValueObjectPrinter::Init(
 }
 
 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());
+    return true;
+  }
+   
   if (!GetMostSpecializedValue() || m_valobj == nullptr)
     return false;