Refactor GetObjectDescription() to return llvm::Expected (NFC)

adrian-prantl · adrian-prantl · commit f900644ae2b6 · 2024-06-20T10:32:06.000-07:00
This is de facto an NFC change for Objective-C but will benefit the
Swift language plugin.
diff --git a/lldb/include/lldb/Core/ValueObject.h b/lldb/include/lldb/Core/ValueObject.h
@@ -537,7 +537,7 @@ class ValueObject {
                            std::string &destination,
                            const TypeSummaryOptions &options);
 
-  const char *GetObjectDescription();
+  llvm::Expected<std::string> GetObjectDescription();
 
   bool HasSpecialPrintableRepresentation(
       ValueObjectRepresentationStyle val_obj_display,
diff --git a/lldb/include/lldb/DataFormatters/ValueObjectPrinter.h b/lldb/include/lldb/DataFormatters/ValueObjectPrinter.h
@@ -75,7 +75,7 @@ class ValueObjectPrinter {
 
   void SetupMostSpecializedValue();
 
-  const char *GetDescriptionForDisplay();
+  llvm::Expected<std::string> GetDescriptionForDisplay();
 
   const char *GetRootNameForDisplay();
 
@@ -108,7 +108,8 @@ class ValueObjectPrinter {
 
   bool PrintValueAndSummaryIfNeeded(bool &value_printed, bool &summary_printed);
 
-  bool PrintObjectDescriptionIfNeeded(bool value_printed, bool summary_printed);
+  llvm::Error PrintObjectDescriptionIfNeeded(bool value_printed,
+                                             bool summary_printed);
 
   bool
   ShouldPrintChildren(DumpValueObjectOptions::PointerDepth &curr_ptr_depth);
@@ -132,7 +133,7 @@ class ValueObjectPrinter {
   PrintChildren(bool value_printed, bool summary_printed,
                 const DumpValueObjectOptions::PointerDepth &curr_ptr_depth);
 
-  void PrintChildrenIfNeeded(bool value_printed, bool summary_printed);
+  llvm::Error PrintChildrenIfNeeded(bool value_printed, bool summary_printed);
 
   bool PrintChildrenOneLiner(bool hide_names);
 
diff --git a/lldb/include/lldb/Target/LanguageRuntime.h b/lldb/include/lldb/Target/LanguageRuntime.h
@@ -73,11 +73,12 @@ class LanguageRuntime : public Runtime, public PluginInterface {
     return nullptr;
   }
 
-  virtual bool GetObjectDescription(Stream &str, ValueObject &object) = 0;
-
-  virtual bool GetObjectDescription(Stream &str, Value &value,
-                                    ExecutionContextScope *exe_scope) = 0;
+  virtual llvm::Error GetObjectDescription(Stream &str,
+                                           ValueObject &object) = 0;
 
+  virtual llvm::Error
+  GetObjectDescription(Stream &str, Value &value,
+                       ExecutionContextScope *exe_scope) = 0;
 
   struct VTableInfo {
     Address addr; /// Address of the vtable's virtual function table
diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
@@ -379,7 +379,10 @@ const char *SBValue::GetObjectDescription() {
   if (!value_sp)
     return nullptr;
 
-  return ConstString(value_sp->GetObjectDescription()).GetCString();
+  llvm::Expected<std::string> str = value_sp->GetObjectDescription();
+  if (!str)
+    return ConstString("error: " + toString(str.takeError())).AsCString();
+  return ConstString(*str).AsCString();
 }
 
 SBType SBValue::GetType() {
diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
@@ -989,41 +989,46 @@ ValueObject::ReadPointedString(lldb::WritableDataBufferSP &buffer_sp,
   return {total_bytes_read, was_capped};
 }
 
-const char *ValueObject::GetObjectDescription() {
+llvm::Expected<std::string> ValueObject::GetObjectDescription() {
   if (!UpdateValueIfNeeded(true))
-    return nullptr;
+    return llvm::createStringError("could not update value");
 
   // Return cached value.
   if (!m_object_desc_str.empty())
-    return m_object_desc_str.c_str();
+    return m_object_desc_str;
 
   ExecutionContext exe_ctx(GetExecutionContextRef());
   Process *process = exe_ctx.GetProcessPtr();
   if (!process)
-    return nullptr;
+    return llvm::createStringError("no process");
 
   // Returns the object description produced by one language runtime.
-  auto get_object_description = [&](LanguageType language) -> const char * {
+  auto get_object_description =
+      [&](LanguageType language) -> llvm::Expected<std::string> {
     if (LanguageRuntime *runtime = process->GetLanguageRuntime(language)) {
       StreamString s;
-      if (runtime->GetObjectDescription(s, *this)) {
-        m_object_desc_str.append(std::string(s.GetString()));
-        return m_object_desc_str.c_str();
-      }
+      if (llvm::Error error = runtime->GetObjectDescription(s, *this))
+        return error;
+      m_object_desc_str = s.GetString();
+      return m_object_desc_str;
     }
-    return nullptr;
+    return llvm::createStringError("no native language runtime");
   };
 
   // Try the native language runtime first.
   LanguageType native_language = GetObjectRuntimeLanguage();
-  if (const char *desc = get_object_description(native_language))
+  llvm::Expected<std::string> desc = get_object_description(native_language);
+  if (desc)
     return desc;
 
   // Try the Objective-C language runtime. This fallback is necessary
   // for Objective-C++ and mixed Objective-C / C++ programs.
-  if (Language::LanguageIsCFamily(native_language))
+  if (Language::LanguageIsCFamily(native_language)) {
+    // We're going to try again, so let's drop the first error.
+    llvm::consumeError(desc.takeError());
     return get_object_description(eLanguageTypeObjC);
-  return nullptr;
+  }
+  return desc;
 }
 
 bool ValueObject::GetValueAsCString(const lldb_private::TypeFormatImpl &format,
@@ -1472,9 +1477,16 @@ bool ValueObject::DumpPrintableRepresentation(
       str = GetSummaryAsCString();
       break;
 
-    case eValueObjectRepresentationStyleLanguageSpecific:
-      str = GetObjectDescription();
-      break;
+    case eValueObjectRepresentationStyleLanguageSpecific: {
+      llvm::Expected<std::string> desc = GetObjectDescription();
+      if (!desc) {
+        strm << "error: " << toString(desc.takeError());
+        str = strm.GetString();
+      } else {
+        strm << *desc;
+        str = strm.GetString();
+      }
+    } break;
 
     case eValueObjectRepresentationStyleLocation:
       str = GetLocationAsCString();
diff --git a/lldb/source/DataFormatters/ValueObjectPrinter.cpp b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -91,9 +91,8 @@ llvm::Error ValueObjectPrinter::PrintValueObject() {
       PrintValueAndSummaryIfNeeded(value_printed, summary_printed);
 
   if (m_val_summary_ok)
-    PrintChildrenIfNeeded(value_printed, summary_printed);
-  else
-    m_stream->EOL();
+    return PrintChildrenIfNeeded(value_printed, summary_printed);
+  m_stream->EOL();
 
   return llvm::Error::success();
 }
@@ -145,13 +144,21 @@ void ValueObjectPrinter::SetupMostSpecializedValue() {
          "SetupMostSpecialized value must compute a valid ValueObject");
 }
 
-const char *ValueObjectPrinter::GetDescriptionForDisplay() {
+llvm::Expected<std::string> ValueObjectPrinter::GetDescriptionForDisplay() {
   ValueObject &valobj = GetMostSpecializedValue();
-  const char *str = valobj.GetObjectDescription();
+  llvm::Expected<std::string> maybe_str = valobj.GetObjectDescription();
+  if (maybe_str)
+    return maybe_str;
+
+  const char *str = nullptr;
   if (!str)
     str = valobj.GetSummaryAsCString();
   if (!str)
     str = valobj.GetValueAsCString();
+
+  if (!str)
+    return maybe_str;
+  llvm::consumeError(maybe_str.takeError());
   return str;
 }
 
@@ -461,34 +468,38 @@ bool ValueObjectPrinter::PrintValueAndSummaryIfNeeded(bool &value_printed,
   return !error_printed;
 }
 
-bool ValueObjectPrinter::PrintObjectDescriptionIfNeeded(bool value_printed,
-                                                        bool summary_printed) {
+llvm::Error
+ValueObjectPrinter::PrintObjectDescriptionIfNeeded(bool value_printed,
+                                                   bool summary_printed) {
   if (ShouldPrintValueObject()) {
     // let's avoid the overly verbose no description error for a nil thing
     if (m_options.m_use_objc && !IsNil() && !IsUninitialized() &&
         (!m_options.m_pointer_as_array)) {
       if (!m_options.m_hide_value || ShouldShowName())
-        m_stream->Printf(" ");
-      const char *object_desc = nullptr;
-      if (value_printed || summary_printed)
-        object_desc = GetMostSpecializedValue().GetObjectDescription();
-      else
-        object_desc = GetDescriptionForDisplay();
-      if (object_desc && *object_desc) {
+        *m_stream << ' ';
+      llvm::Expected<std::string> object_desc =
+          (value_printed || summary_printed)
+              ? GetMostSpecializedValue().GetObjectDescription()
+              : GetDescriptionForDisplay();
+      if (!object_desc) {
+        // If no value or summary was printed, surface the error.
+        if (!value_printed && !summary_printed)
+          return object_desc.takeError();
+        // Otherwise gently nudge the user that they should have used
+        // `p` instead of `po`. Unfortunately we cannot be more direct
+        // about this, because we don't actually know what the user did.
+        *m_stream << "warning: no object description available\n";
+        llvm::consumeError(object_desc.takeError());
+      } else {
+        *m_stream << *object_desc;
         // If the description already ends with a \n don't add another one.
-        size_t object_end = strlen(object_desc) - 1;
-        if (object_desc[object_end] == '\n')
-          m_stream->Printf("%s", object_desc);
-        else
-          m_stream->Printf("%s\n", object_desc);
-        return true;
-      } else if (!value_printed && !summary_printed)
-        return true;
-      else
-        return false;
+        if (object_desc->empty() || object_desc->back() != '\n')
+          *m_stream << '\n';
+      }
+      return llvm::Error::success();
     }
   }
-  return true;
+  return llvm::Error::success();
 }
 
 bool DumpValueObjectOptions::PointerDepth::CanAllowExpansion() const {
@@ -812,9 +823,12 @@ bool ValueObjectPrinter::PrintChildrenOneLiner(bool hide_names) {
   return true;
 }
 
-void ValueObjectPrinter::PrintChildrenIfNeeded(bool value_printed,
-                                               bool summary_printed) {
-  PrintObjectDescriptionIfNeeded(value_printed, summary_printed);
+llvm::Error ValueObjectPrinter::PrintChildrenIfNeeded(bool value_printed,
+                                                      bool summary_printed) {
+  auto error = PrintObjectDescriptionIfNeeded(value_printed, summary_printed);
+  if (error)
+    return error;
+
   ValueObject &valobj = GetMostSpecializedValue();
 
   DumpValueObjectOptions::PointerDepth curr_ptr_depth = m_ptr_depth;
@@ -830,7 +844,7 @@ void ValueObjectPrinter::PrintChildrenIfNeeded(bool value_printed,
     if (m_printed_instance_pointers->count(instance_ptr_value)) {
       // We already printed this instance-is-pointer thing, so don't expand it.
       m_stream->PutCString(" {...}\n");
-      return;
+      return llvm::Error::success();
     } else {
       // Remember this guy for future reference.
       m_printed_instance_pointers->emplace(instance_ptr_value);
@@ -858,6 +872,7 @@ void ValueObjectPrinter::PrintChildrenIfNeeded(bool value_printed,
           .SetReachedMaximumDepth();
   } else
     m_stream->EOL();
+  return llvm::Error::success();
 }
 
 bool ValueObjectPrinter::HasReachedMaximumDepth() {
diff --git a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
@@ -47,16 +47,17 @@ bool CPPLanguageRuntime::IsAllowedRuntimeValue(ConstString name) {
   return name == g_this || name == g_promise || name == g_coro_frame;
 }
 
-bool CPPLanguageRuntime::GetObjectDescription(Stream &str,
-                                              ValueObject &object) {
+llvm::Error CPPLanguageRuntime::GetObjectDescription(Stream &str,
+                                                     ValueObject &object) {
   // C++ has no generic way to do this.
-  return false;
+  return llvm::createStringError("C++ does not support object descriptions");
 }
 
-bool CPPLanguageRuntime::GetObjectDescription(
-    Stream &str, Value &value, ExecutionContextScope *exe_scope) {
+llvm::Error
+CPPLanguageRuntime::GetObjectDescription(Stream &str, Value &value,
+                                         ExecutionContextScope *exe_scope) {
   // C++ has no generic way to do this.
-  return false;
+  return llvm::createStringError("C++ does not support object descriptions");
 }
 
 bool contains_lambda_identifier(llvm::StringRef &str_ref) {
diff --git a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.h b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.h
@@ -59,10 +59,10 @@ class CPPLanguageRuntime : public LanguageRuntime {
         process.GetLanguageRuntime(lldb::eLanguageTypeC_plus_plus));
   }
 
-  bool GetObjectDescription(Stream &str, ValueObject &object) override;
+  llvm::Error GetObjectDescription(Stream &str, ValueObject &object) override;
 
-  bool GetObjectDescription(Stream &str, Value &value,
-                            ExecutionContextScope *exe_scope) override;
+  llvm::Error GetObjectDescription(Stream &str, Value &value,
+                                   ExecutionContextScope *exe_scope) override;
 
   /// Obtain a ThreadPlan to get us into C++ constructs such as std::function.
   ///
diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.h b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.h
diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/GNUstepObjCRuntime/GNUstepObjCRuntime.cpp b/lldb/source/Plugins/LanguageRuntime/ObjC/GNUstepObjCRuntime/GNUstepObjCRuntime.cpp
diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/GNUstepObjCRuntime/GNUstepObjCRuntime.h b/lldb/source/Plugins/LanguageRuntime/ObjC/GNUstepObjCRuntime/GNUstepObjCRuntime.h
