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

This fixes a few bugs, effectively through a fallback to `p` when `po` fails.

The motivating bug this fixes is when an error within the compiler causes `po` to fail.
Previously when that happened, only its value (typically an object's address) was
printed – and problematically, no compiler diagnostics were shown. With this change,
compiler diagnostics are shown, _and_ the object is fully printed (ie `p`).

Another bug this fixes is when `po` is used on a type that doesn't provide an object
description (such as a struct). Again, the normal `ValueObject` printing is used.

Additionally, this also improves how lldb handles an object description method that
fails in some way. Now an error will be shown (it wasn't before), and the value will be
printed normally.

lldb/include/lldb/DataFormatters/DumpValueObjectOptions.h

@@ -76,7 +76,9 @@ public:
 
   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 @@ public:
   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 @@ protected:
 
   void SetupMostSpecializedValue();
 
-  llvm::Expected<std::string> GetDescriptionForDisplay();
-
   const char *GetRootNameForDisplay();
 
   bool ShouldPrintValueObject();
@@ -108,8 +106,7 @@ protected:
 
   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 @@ protected:
 
 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 @@ public:
 
   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 @@ public:
       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

@@ -93,7 +93,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

@@ -202,7 +202,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);
@@ -241,11 +241,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;
 }
 
@@ -332,7 +332,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,9 +14,11 @@
 #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 <memory>
+#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

@@ -90,7 +90,7 @@ Status OptionGroupValueObjectDisplay::SetOptionValue(
     flat_output = true;
     break;
   case 'O':
-    use_objc = true;
+    use_object_desc = true;
     break;
   case 'R':
     be_raw = true;
@@ -163,7 +163,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;
@@ -191,14 +191,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)
@@ -208,8 +208,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"],
+        )

lldb/test/API/lang/objc/failing-description/main.m

@@ -0,0 +1,21 @@
+#import <Foundation/Foundation.h>
+
+@interface Bad : NSObject
+@end
+
+@implementation Bad {
+  BOOL _lookHere;
+}
+
+- (NSString *)description {
+  int *i = NULL;
+  *i = 0;
+  return @"surprise";
+}
+@end
+
+int main() {
+  Bad *bad = [Bad new];
+  printf("break here\n");
+  return 0;
+}

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

@@ -0,0 +1,2 @@
+OBJC_SOURCES := main.m
+include Makefile.rules

lldb/test/API/lang/objc/struct-description/TestObjCStructDescription.py

@@ -0,0 +1,18 @@
+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(
+            "vo pair",
+            substrs=["error:", "not a pointer type", "(Pair) pair = (f = 2, e = 3)"],
+        )
+        self.expect(
+            "expr -O -- pair",
+            substrs=["error:", "not a pointer type", "(Pair)  (f = 2, e = 3)"],
+        )

lldb/test/API/lang/objc/struct-description/main.m

@@ -0,0 +1,12 @@
+#include <stdio.h>
+
+typedef struct Pair {
+  int f;
+  int e;
+} Pair;
+
+int main() {
+  Pair pair = {2, 3};
+  printf("break here\n");
+  return 0;
+}