[lldb] Print ValueObject when GetObjectDescription fails

[kastiglione]

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.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Dave Lee (kastiglione)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/152417.diff

1 Files Affected:

• (modified) lldb/source/DataFormatters/ValueObjectPrinter.cpp (+6)

diff --git a/lldb/source/DataFormatters/ValueObjectPrinter.cpp b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
index c2f8bb3ea05bc..c5a6de2de35f2 100644
--- a/lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -9,6 +9,7 @@
 #include "lldb/DataFormatters/ValueObjectPrinter.h"
 
 #include "lldb/DataFormatters/DataVisualization.h"
+#include "lldb/Expression/DiagnosticManager.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/Target.h"
@@ -150,6 +151,11 @@ llvm::Expected<std::string> ValueObjectPrinter::GetDescriptionForDisplay() {
   if (maybe_str)
     return maybe_str;
 
+  if (maybe_str.errorIsA<lldb_private::ExpressionError>())
+    // Propagate expression errors to expose diagnostics to the user.
+    // Without this early exit, the summary/value may be shown without errors.
+    return maybe_str;
+
   const char *str = nullptr;
   if (!str)
     str = valobj.GetSummaryAsCString();

[kastiglione]

If this change is accepted, I will also revert #151374

[adrian-prantl]

Would this be testable by running an expression that fails reliably? For example by implementing a [-description] method that crashes?

[kastiglione]

Unfortunately no

[kastiglione]

@adrian-prantl I've added a test for this with the recent reworking. See TestObjCFailingDescription.py

[jimingham]

The way we've been doing the po implementations for a while now is: "If the object description succeeds, print the object description, otherwise print the value." The routine you are changing (GetDescriptionForDisplay) gets used here:

  llvm::Expected<std::string> object_desc =
      (value_printed || summary_printed)
          ? GetMostSpecializedValue().GetObjectDescription()
          : GetDescriptionForDisplay();

So it's supposed to handle the case where we haven't yet printed any value or summary for the po result. By returning early if you see an expression error, you're not running the code in that routine that would have printed the summary or value. That's changing this UI to NOT print the value if the po expression fails for some reason.

Is that your intent?

[jimingham]

I think it would be more in line with user's expectations if, instead of directly returning maybe_string in this case, you build a new string that has:

Object Description Error:
  <contents of maybe_str>
foo = 10

or something like that.

[jimingham]

Our experience is that people think po is the way to show them values, and so they will ALWAYS want to see the value regardless of where it comes from.

[kastiglione]

The ultimate intent is to show errors preventing an expected successful object description. If producing an object description fails, printing an object's address (value) is not what users expect. We can change this to print the error and the value, but I don't expect any users to find any use from the value/address when their intended po has failed.

[kastiglione]

That's changing this UI to NOT print the value if the po expression fails for some reason.

Is that your intent?

it's my intent when the failure is a compiler reason – which is to make a distinction that this is a more narrow change than "some reason".

[jimingham]

I don't like the usage pattern, but I know there are a substantial number of Darwin developers who ONLY know po. They really don't use p. Having to fall back to p to get whatever we can get out of the value for them is not what they want. They want po to always give them some value.

So I'm not sure that slice of our users would be happy with this change.

OTOH, since I'm not a fan of that usage pattern, I don't myself care one way or the other. I'll happily direct their complaints to you ;-)

[kastiglione]

Following a offline conversation with Jim, I've updated this PR. See the description for details on the changes.

[jimingham]

You said above that this code wouldn't get used if the description expression crashed. Why is that? Even if we can't get a diagnostic error from the crash process, it would be nice to use this same codepath to do:

(lldb) po bogus_description
Warning: po description expression crashed, printing value
(BogusDescription *) 0x12345

or something? If it did that we could also write a test here, which would make this feel better...

The code looks okay except it would be nice to change all the places where we're saying "DisableObjectiveC" to say "DisableObjectDescription" or something (and m_use_objc -> m_use_object_desc). You didn't start that but you're adding another instance, and it would be good not to extend the mistake... We call it the right thing in all the public facing API's, the confusing name is just internal.

It will look really silly when this goes to the swift fork and we're using m_use_objc or DisableObjectiveC() to say whether or not we want the description of a swift object (or any other language that has some kind of object description machinery.)

[kastiglione]

You said above that this code wouldn't get used if the description expression crashed.

Originally, that was the case.

it would be nice to use this same codepath to do:

agree, and it does so. I was in the process of writing tests, which I've now added.

[kastiglione]

I've renamed the various use_objc names to use_object_desc.

[jimingham]

LGTM

[kastiglione]

(lldb) po bogus_description
Warning: po description expression crashed, printing value
(BogusDescription *) 0x12345

This case looks like:

(lldb) po bogus_description
error: could not evaluate print object function: expression interrupted
(BogusDescription *) 0x12345