[lldb] Print ValueObject when GetObjectDescription fails (#152417)

lldb/include/lldb/DataFormatters/DumpValueObjectOptions.h

@@ -76,7 +76,9 @@ class DumpValueObjectOptions {
 
   DumpValueObjectOptions &SetShowLocation(bool show = false);
 
-  DumpValueObjectOptions &SetUseObjectiveC(bool use = false);
+  DumpValueObjectOptions &DisableObjectDescription();
+
+  DumpValueObjectOptions &SetUseObjectDescription(bool use = false);
 
   DumpValueObjectOptions &SetShowSummary(bool show = true);
 
@@ -143,13 +145,14 @@ class DumpValueObjectOptions {
   ChildPrintingDecider m_child_printing_decider;
   PointerAsArraySettings m_pointer_as_array;
   unsigned m_expand_ptr_type_flags = 0;
+  // The following flags commonly default to false.
   bool m_use_synthetic : 1;
   bool m_scope_already_checked : 1;
   bool m_flat_output : 1;
   bool m_ignore_cap : 1;
   bool m_show_types : 1;
   bool m_show_location : 1;
-  bool m_use_objc : 1;
+  bool m_use_object_desc : 1;
   bool m_hide_root_type : 1;
   bool m_hide_root_name : 1;
   bool m_hide_name : 1;

lldb/include/lldb/DataFormatters/ValueObjectPrinter.h

@@ -75,8 +75,6 @@ class ValueObjectPrinter {
 
   void SetupMostSpecializedValue();
 
-  llvm::Expected<std::string> GetDescriptionForDisplay();
-
   const char *GetRootNameForDisplay();
 
   bool ShouldPrintValueObject();
@@ -108,8 +106,7 @@ class ValueObjectPrinter {
 
   bool PrintValueAndSummaryIfNeeded(bool &value_printed, bool &summary_printed);
 
-  llvm::Error PrintObjectDescriptionIfNeeded(bool value_printed,
-                                             bool summary_printed);
+  void PrintObjectDescriptionIfNeeded(std::optional<std::string> object_desc);
 
   bool
   ShouldPrintChildren(DumpValueObjectOptions::PointerDepth &curr_ptr_depth);
@@ -141,6 +138,7 @@ class ValueObjectPrinter {
 
 private:
   bool ShouldShowName() const;
+  bool ShouldPrintObjectDescription();
 
   ValueObject &m_orig_valobj;
   /// Cache the current "most specialized" value.  Don't use this

lldb/include/lldb/Interpreter/OptionGroupValueObjectDisplay.h

@@ -31,7 +31,7 @@ class OptionGroupValueObjectDisplay : public OptionGroup {
 
   bool AnyOptionWasSet() const {
     return show_types || no_summary_depth != 0 || show_location ||
-           flat_output || use_objc || max_depth != UINT32_MAX ||
+           flat_output || use_object_desc || max_depth != UINT32_MAX ||
            ptr_depth != 0 || !use_synth || be_raw || ignore_cap ||
            run_validator;
   }
@@ -42,7 +42,7 @@ class OptionGroupValueObjectDisplay : public OptionGroup {
       lldb::Format format = lldb::eFormatDefault,
       lldb::TypeSummaryImplSP summary_sp = lldb::TypeSummaryImplSP());
 
-  bool show_types : 1, show_location : 1, flat_output : 1, use_objc : 1,
+  bool show_types : 1, show_location : 1, flat_output : 1, use_object_desc : 1,
       use_synth : 1, be_raw : 1, ignore_cap : 1, run_validator : 1,
       max_depth_is_default : 1;
 

lldb/source/Commands/CommandObjectDWIMPrint.cpp

@@ -92,7 +92,7 @@ void CommandObjectDWIMPrint::DoExecute(StringRef command,
   dump_options.SetHideRootName(suppress_result)
       .SetExpandPointerTypeFlags(lldb::eTypeIsObjC);
 
-  bool is_po = m_varobj_options.use_objc;
+  bool is_po = m_varobj_options.use_object_desc;
 
   StackFrame *frame = m_exe_ctx.GetFramePtr();
 

lldb/source/Commands/CommandObjectExpression.cpp

@@ -220,7 +220,7 @@ EvaluateExpressionOptions
 CommandObjectExpression::CommandOptions::GetEvaluateExpressionOptions(
     const Target &target, const OptionGroupValueObjectDisplay &display_opts) {
   EvaluateExpressionOptions options;
-  options.SetCoerceToId(display_opts.use_objc);
+  options.SetCoerceToId(display_opts.use_object_desc);
   options.SetUnwindOnError(unwind_on_error);
   options.SetIgnoreBreakpoints(ignore_breakpoints);
   options.SetKeepInMemory(true);
@@ -263,11 +263,11 @@ CommandObjectExpression::CommandOptions::GetEvaluateExpressionOptions(
 bool CommandObjectExpression::CommandOptions::ShouldSuppressResult(
     const OptionGroupValueObjectDisplay &display_opts) const {
   // Explicitly disabling persistent results takes precedence over the
-  // m_verbosity/use_objc logic.
+  // m_verbosity/use_object_desc logic.
   if (suppress_persistent_result != eLazyBoolCalculate)
     return suppress_persistent_result == eLazyBoolYes;
 
-  return display_opts.use_objc &&
+  return display_opts.use_object_desc &&
          m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact;
 }
 
@@ -356,7 +356,7 @@ Options *CommandObjectExpression::GetOptions() { return &m_option_group; }
 
 void CommandObjectExpression::HandleCompletion(CompletionRequest &request) {
   EvaluateExpressionOptions options;
-  options.SetCoerceToId(m_varobj_options.use_objc);
+  options.SetCoerceToId(m_varobj_options.use_object_desc);
   options.SetLanguage(m_command_options.language);
   options.SetExecutionPolicy(lldb_private::eExecutionPolicyNever);
   options.SetAutoApplyFixIts(false);

lldb/source/DataFormatters/DumpValueObjectOptions.cpp

@@ -17,7 +17,7 @@ DumpValueObjectOptions::DumpValueObjectOptions()
     : m_summary_sp(), m_root_valobj_name(), m_decl_printing_helper(),
       m_child_printing_decider(), m_pointer_as_array(), m_use_synthetic(true),
       m_scope_already_checked(false), m_flat_output(false), m_ignore_cap(false),
-      m_show_types(false), m_show_location(false), m_use_objc(false),
+      m_show_types(false), m_show_location(false), m_use_object_desc(false),
       m_hide_root_type(false), m_hide_root_name(false), m_hide_name(false),
       m_hide_value(false), m_run_validator(false),
       m_use_type_display_name(true), m_allow_oneliner_mode(true),
@@ -65,8 +65,19 @@ DumpValueObjectOptions &DumpValueObjectOptions::SetShowLocation(bool show) {
   return *this;
 }
 
-DumpValueObjectOptions &DumpValueObjectOptions::SetUseObjectiveC(bool use) {
-  m_use_objc = use;
+DumpValueObjectOptions &DumpValueObjectOptions::DisableObjectDescription() {
+  // Reset these options to their default values.
+  SetUseObjectDescription(false);
+  SetHideRootType(false);
+  SetHideName(false);
+  SetHideValue(false);
+  SetShowSummary(true);
+  return *this;
+}
+
+DumpValueObjectOptions &
+DumpValueObjectOptions::SetUseObjectDescription(bool use) {
+  m_use_object_desc = use;
   return *this;
 }
 

lldb/source/DataFormatters/ValueObjectPrinter.cpp

@@ -14,8 +14,10 @@
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/ValueObject/ValueObject.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/MathExtras.h"
 #include <cstdint>
+#include <optional>
 
 using namespace lldb;
 using namespace lldb_private;
@@ -69,6 +71,18 @@ void ValueObjectPrinter::Init(
   SetupMostSpecializedValue();
 }
 
+static const char *maybeNewline(const std::string &s) {
+  // If the string already ends with a \n don't add another one.
+  if (s.empty() || s.back() != '\n')
+    return "\n";
+  return "";
+}
+
+bool ValueObjectPrinter::ShouldPrintObjectDescription() {
+  return ShouldPrintValueObject() && m_options.m_use_object_desc && !IsNil() &&
+         !IsUninitialized() && !m_options.m_pointer_as_array;
+}
+
 llvm::Error ValueObjectPrinter::PrintValueObject() {
   // 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
@@ -77,6 +91,25 @@ llvm::Error ValueObjectPrinter::PrintValueObject() {
       !m_orig_valobj.GetCompilerType().IsValid())
     return m_orig_valobj.GetError().ToError();
 
+  std::optional<std::string> object_desc;
+  if (ShouldPrintObjectDescription()) {
+    // The object description is invoked now, but not printed until after
+    // value/summary. Calling GetObjectDescription at the outset of printing
+    // allows for early discovery of errors. In the case of an error, the value
+    // object is printed normally.
+    llvm::Expected<std::string> object_desc_or_err =
+        GetMostSpecializedValue().GetObjectDescription();
+    if (!object_desc_or_err) {
+      auto error_msg = toString(object_desc_or_err.takeError());
+      *m_stream << "error: " << error_msg << maybeNewline(error_msg);
+
+      // Print the value object directly.
+      m_options.DisableObjectDescription();
+    } else {
+      object_desc = *object_desc_or_err;
+    }
+  }
+
   if (ShouldPrintValueObject()) {
     PrintLocationIfNeeded();
     m_stream->Indent();
@@ -90,8 +123,10 @@ llvm::Error ValueObjectPrinter::PrintValueObject() {
   m_val_summary_ok =
       PrintValueAndSummaryIfNeeded(value_printed, summary_printed);
 
-  if (m_val_summary_ok)
+  if (m_val_summary_ok) {
+    PrintObjectDescriptionIfNeeded(object_desc);
     return PrintChildrenIfNeeded(value_printed, summary_printed);
+  }
   m_stream->EOL();
 
   return llvm::Error::success();
@@ -144,24 +179,6 @@ void ValueObjectPrinter::SetupMostSpecializedValue() {
          "SetupMostSpecialized value must compute a valid ValueObject");
 }
 
-llvm::Expected<std::string> ValueObjectPrinter::GetDescriptionForDisplay() {
-  ValueObject &valobj = GetMostSpecializedValue();
-  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;
-}
-
 const char *ValueObjectPrinter::GetRootNameForDisplay() {
   const char *root_valobj_name =
       m_options.m_root_valobj_name.empty()
@@ -468,38 +485,14 @@ bool ValueObjectPrinter::PrintValueAndSummaryIfNeeded(bool &value_printed,
   return !error_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 << ' ';
-      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.
-        if (object_desc->empty() || object_desc->back() != '\n')
-          *m_stream << '\n';
-      }
-      return llvm::Error::success();
-    }
-  }
-  return llvm::Error::success();
+void ValueObjectPrinter::PrintObjectDescriptionIfNeeded(
+    std::optional<std::string> object_desc) {
+  if (!object_desc)
+    return;
+
+  if (!m_options.m_hide_value || ShouldShowName())
+    *m_stream << ' ';
+  *m_stream << *object_desc << maybeNewline(*object_desc);
 }
 
 bool DumpValueObjectOptions::PointerDepth::CanAllowExpansion() const {
@@ -524,7 +517,7 @@ bool ValueObjectPrinter::ShouldPrintChildren(
   if (m_options.m_pointer_as_array)
     return true;
 
-  if (m_options.m_use_objc)
+  if (m_options.m_use_object_desc)
     return false;
 
   bool print_children = true;
@@ -819,9 +812,6 @@ bool ValueObjectPrinter::PrintChildrenOneLiner(bool hide_names) {
 
 llvm::Error ValueObjectPrinter::PrintChildrenIfNeeded(bool value_printed,
                                                       bool summary_printed) {
-  auto error = PrintObjectDescriptionIfNeeded(value_printed, summary_printed);
-  if (error)
-    return error;
 
   ValueObject &valobj = GetMostSpecializedValue();
 

lldb/source/Expression/REPL.cpp

@@ -321,7 +321,7 @@ void REPL::IOHandlerInputComplete(IOHandler &io_handler, std::string &code) {
       const bool colorize_err = error_sp->GetFile().GetIsTerminalWithColors();
 
       EvaluateExpressionOptions expr_options = m_expr_options;
-      expr_options.SetCoerceToId(m_varobj_options.use_objc);
+      expr_options.SetCoerceToId(m_varobj_options.use_object_desc);
       expr_options.SetKeepInMemory(true);
       expr_options.SetUseDynamic(m_varobj_options.use_dynamic);
       expr_options.SetGenerateDebugInfo(true);

lldb/source/Interpreter/OptionGroupValueObjectDisplay.cpp

@@ -91,7 +91,7 @@ Status OptionGroupValueObjectDisplay::SetOptionValue(
     flat_output = true;
     break;
   case 'O':
-    use_objc = true;
+    use_object_desc = true;
     break;
   case 'R':
     be_raw = true;
@@ -164,7 +164,7 @@ void OptionGroupValueObjectDisplay::OptionParsingStarting(
   no_summary_depth = 0;
   show_location = false;
   flat_output = false;
-  use_objc = false;
+  use_object_desc = false;
   max_depth = UINT32_MAX;
   max_depth_is_default = true;
   ptr_depth = 0;
@@ -192,14 +192,14 @@ DumpValueObjectOptions OptionGroupValueObjectDisplay::GetAsDumpOptions(
     lldb::Format format, lldb::TypeSummaryImplSP summary_sp) {
   DumpValueObjectOptions options;
   options.SetMaximumPointerDepth(ptr_depth);
-  if (use_objc)
+  if (use_object_desc)
     options.SetShowSummary(false);
   else
     options.SetOmitSummaryDepth(no_summary_depth);
   options.SetMaximumDepth(max_depth, max_depth_is_default)
       .SetShowTypes(show_types)
       .SetShowLocation(show_location)
-      .SetUseObjectiveC(use_objc)
+      .SetUseObjectDescription(use_object_desc)
       .SetUseDynamicType(use_dynamic)
       .SetUseSyntheticValue(use_synth)
       .SetFlatOutput(flat_output)
@@ -209,8 +209,9 @@ DumpValueObjectOptions OptionGroupValueObjectDisplay::GetAsDumpOptions(
 
   if (lang_descr_verbosity ==
       eLanguageRuntimeDescriptionDisplayVerbosityCompact)
-    options.SetHideRootType(use_objc).SetHideName(use_objc).SetHideValue(
-        use_objc);
+    options.SetHideRootType(use_object_desc)
+        .SetHideName(use_object_desc)
+        .SetHideValue(use_object_desc);
 
   if (be_raw)
     options.SetRawDisplay();

lldb/test/API/lang/objc/failing-description/Makefile

@@ -0,0 +1,3 @@
+OBJC_SOURCES := main.m
+LD_EXTRAS := -lobjc -framework Foundation
+include Makefile.rules

lldb/test/API/lang/objc/failing-description/TestObjCFailingDescription.py

@@ -0,0 +1,17 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestCase(TestBase):
+    def test(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.m"))
+        self.expect(
+            "expr -O -- bad", substrs=["error:", "expression interrupted", "(Bad *) 0x"]
+        )
+        self.expect(
+            "dwim-print -O -- bad",
+            substrs=["error:", "expression interrupted", "_lookHere = NO"],
+        )