[Lldb-commits] [PATCH] D144664: Preserve SBValue's that only have an Error state

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: JDevlieghere, clayborg, labath, jasonmolenda.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

SBValues carry their own errors with them (SBValue::GetError).  That means that even if an SBValue was made in a situation where we couldn't actually make a value (e.g. you called `lldb.frame.EvaluateExpression` while the target is running) the error is still useful.  We were using the IsValid check to decide whether to actually return the underlying ValueObject to the SBValue.

This could have been implemented two ways.  One is to add "Has an error" to the conditions that would cause IsValid to return true.  I chose not to do that because previously you could use IsValid to mean "has a valid Value" and so this would have been a semantic change and could break scripts.

Instead I just add the few "or has error" checks needed to preserve "error only SBValues" in the SBValue interface, and fixed up a couple places in ValueObject printing where we needed to print the error if the ValueObject had no other useful state.

This patch doesn't have a test.  I have a follow-on commit which is what motivated this change and the test for that was what necessitated, and will verify, this change.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144664

Files:
  lldb/source/API/SBValue.cpp
  lldb/source/Core/ValueObject.cpp
  lldb/source/DataFormatters/ValueObjectPrinter.cpp


Index: lldb/source/DataFormatters/ValueObjectPrinter.cpp
===================================================================
--- lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -71,6 +71,18 @@
 }
 
 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;
 
Index: lldb/source/Core/ValueObject.cpp
===================================================================
--- lldb/source/Core/ValueObject.cpp
+++ lldb/source/Core/ValueObject.cpp
@@ -1173,6 +1173,15 @@
     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 @@
     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());
Index: lldb/source/API/SBValue.cpp
===================================================================
--- lldb/source/API/SBValue.cpp
+++ lldb/source/API/SBValue.cpp
@@ -114,6 +114,10 @@
     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::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();
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D144664.499940.patch
Type: text/x-patch
Size: 2971 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20230223/80fa8959/attachment.bin>