[lldb] Improve function caller error message

[JDevlieghere]

Summary

When trying to run an expression after a process has existed, you currently are shown the following error message:

(lldb) p strlen("")
error: Can't make a function caller while the process is running

This error is wrong and pretty uninformative. After this patch, the following error message is shown:

(lldb) p strlen("")
error: unable to evaluate expression while the process is exited: the process must be stopped because the expression might requires allocating memory.

rdar://109731325

[bulbazord]

I think it's good to improve the error messaging but I think we can probably do better. "Function caller" is specific to the internals of LLDB and isn't really meaningful for many users. It's also somewhat confusing from a user's perspective when the expression you're running isn't calling a function. For example:

alex@alangford build % ./bin/lldb ~/tmp/foo
(lldb) target create "/Users/alex/tmp/foo"
Current executable set to '/Users/alex/tmp/foo' (arm64).
(lldb) b main
Breakpoint 1: where = foo`main + 28 at foo.c:6:7, address = 0x0000000100003f68
(lldb) r
Process 44467 launched: '/Users/alex/tmp/foo' (arm64)
Process 44467 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x0000000100003f68 foo`main at foo.c:6:7
   1    #include <stdio.h>
   2
   3    int g_foo = 5;
   4
   5    int main() {
-> 6      int val = 7;
   7      printf("Hello World!: %d\n", val);
   8      return 0;
   9    }
(lldb) c
Process 44467 resuming
Hello World!: 7
Process 44467 exited with status = 0 (0x00000000)
(lldb) p val
error: Can't make a function caller while the process is running

If I were an end user who didn't know much about LLDB, I would think "I'm trying to print a variable, what's this about a function?" and "Why is memory allocation involved?". I would suggest changing the error message to something like: "Unable to evaluate expression while the process is $STATE: the process must be running and stopped to evaluate this expression".

What do you think?

[mib]

In D151497#4374080, @bulbazord wrote:

I think it's good to improve the error messaging but I think we can probably do better. "Function caller" is specific to the internals of LLDB and isn't really meaningful for many users. It's also somewhat confusing from a user's perspective when the expression you're running isn't calling a function. For example:

alex@alangford build % ./bin/lldb ~/tmp/foo
(lldb) target create "/Users/alex/tmp/foo"
Current executable set to '/Users/alex/tmp/foo' (arm64).
(lldb) b main
Breakpoint 1: where = foo`main + 28 at foo.c:6:7, address = 0x0000000100003f68
(lldb) r
Process 44467 launched: '/Users/alex/tmp/foo' (arm64)
Process 44467 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x0000000100003f68 foo`main at foo.c:6:7
   1    #include <stdio.h>
   2
   3    int g_foo = 5;
   4
   5    int main() {
-> 6      int val = 7;
   7      printf("Hello World!: %d\n", val);
   8      return 0;
   9    }
(lldb) c
Process 44467 resuming
Hello World!: 7
Process 44467 exited with status = 0 (0x00000000)
(lldb) p val
error: Can't make a function caller while the process is running

If I were an end user who didn't know much about LLDB, I would think "I'm trying to print a variable, what's this about a function?" and "Why is memory allocation involved?". I would suggest changing the error message to something like: "Unable to evaluate expression while the process is $STATE: the process must be running and stopped to evaluate this expression".

What do you think?

I do agree on rephrasing the error message to be more user-friendly. I'd also assume that if the process has exited, may be dwim-print would do target variable instead of evaluating an expression, am I right @kastiglione ?

[JDevlieghere]

In D151497#4374080, @bulbazord wrote:

I think it's good to improve the error messaging but I think we can probably do better. "Function caller" is specific to the internals of LLDB and isn't really meaningful for many users. It's also somewhat confusing from a user's perspective when the expression you're running isn't calling a function.

If I were an end user who didn't know much about LLDB, I would think "I'm trying to print a variable, what's this about a function?" and "Why is memory allocation involved?". I would suggest changing the error message to something like: "Unable to evaluate expression while the process is $STATE: the process must be running and stopped to evaluate this expression".

What do you think?

I very much agree that error messages should first and foremost be helpful to our users. In this particular patch, we have two places where we emit this error. In UtilityFunction::MakeFunctionCaller I believe the current error is totally appropriate. That doesn't mean that I think it should be shown to the user as such. I didn't look at how this function is called, but if it trips, I would like to see an error along the lines of:

error: Couldn't run utility function. Can't make a function caller while the process is stopped: the process must be stopped to allocate memory.

On the other hand, in UserExpression::Evaluate I think it's totally appropriate to rephrase this, but at the same time I don't think we need to dumb this down.

error: Unable to evaluate expression while the process is $STATE: the process must be stopped because the expression might requires allocating memory.

I'll update the error messages accordingly.

[JDevlieghere]

Rephrase error message for UserExpression::Evaluate

[JDevlieghere]

lldb/source/Expression/UserExpression.cpp

211

This is intentionally lowercase to match the other error messages coming from UserExpression::Evaluate

[bulbazord]

In D151497#4374495, @JDevlieghere wrote:

In D151497#4374080, @bulbazord wrote:

I think it's good to improve the error messaging but I think we can probably do better. "Function caller" is specific to the internals of LLDB and isn't really meaningful for many users. It's also somewhat confusing from a user's perspective when the expression you're running isn't calling a function.

If I were an end user who didn't know much about LLDB, I would think "I'm trying to print a variable, what's this about a function?" and "Why is memory allocation involved?". I would suggest changing the error message to something like: "Unable to evaluate expression while the process is $STATE: the process must be running and stopped to evaluate this expression".

What do you think?

I very much agree that error messages should first and foremost be helpful to our users. In this particular patch, we have two places where we emit this error. In UtilityFunction::MakeFunctionCaller I believe the current error is totally appropriate. That doesn't mean that I think it should be shown to the user as such. I didn't look at how this function is called, but if it trips, I would like to see an error along the lines of:

error: Couldn't run utility function. Can't make a function caller while the process is stopped: the process must be stopped to allocate memory.

On the other hand, in UserExpression::Evaluate I think it's totally appropriate to rephrase this, but at the same time I don't think we need to dumb this down.

error: Unable to evaluate expression while the process is $STATE: the process must be stopped because the expression might requires allocating memory.

I'll update the error messages accordingly.

This makes sense to me. I don't want to dumb things down per-se, but now that I look back at my suggestion it may have been too simplistic. I think the message you've written is an excellent. Thank you for working on this.

lldb/source/Expression/UserExpression.cpp

207–208

lldb/source/Expression/UtilityFunction.cpp

68–69

lldb/source/Expression/UserExpression.cpp

 #include "lldb/Target/Process.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/ThreadPlan.h"
 #include "lldb/Target/ThreadPlanCallUserExpression.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
+#include "lldb/Utility/State.h"
 #include "lldb/Utility/StreamString.h"
 
 using namespace lldb_private;
 
 char UserExpression::ID;
 
 UserExpression::UserExpression(ExecutionContextScope &exe_scope,
                                llvm::StringRef expr, llvm::StringRef prefix,
UserExpression::Evaluate(ExecutionContext &exe_ctx,
   if (process == nullptr && execution_policy == eExecutionPolicyAlways) {
     LLDB_LOG(log, "== [UserExpression::Evaluate] No process, but the policy is "
                   "eExecutionPolicyAlways");
 
     error.SetErrorString("expression needed to run but couldn't: no process");
 
     return execution_results;
   }
-  // Since we might need to call allocate memory and maybe call code to make
-  // the caller, we need to be stopped.
+
+  // Since we might need to allocate memory, we need to be stopped to run
+  // an expression.
   if (process != nullptr && process->GetState() != lldb::eStateStopped) {
-    error.SetErrorString("Can't make a function caller while the process is " 
-                          "running");
+    error.SetErrorStringWithFormatv(
+        "unable to evaluate expression while the process is {0}: the process "
+        "must be stopped because the expression might require allocating "
+        "memory.",
+        StateAsCString(process->GetState()));
     return execution_results;
   }
 
-
   // Explicitly force the IR interpreter to evaluate the expression when the
   // there is no process that supports running the expression for us. Don't
   // change the execution policy if we have the special top-level policy that
   // doesn't contain any expression and there is nothing to interpret.
   if (execution_policy != eExecutionPolicyTopLevel &&
       (process == nullptr || !process->CanJIT()))
     execution_policy = eExecutionPolicyNever;
 

lldb/source/Expression/UtilityFunction.cpp

 #include "lldb/Expression/IRExecutionUnit.h"
 #include "lldb/Expression/UtilityFunction.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Log.h"
+#include "lldb/Utility/State.h"
 #include "lldb/Utility/Stream.h"
 
 using namespace lldb_private;
 using namespace lldb;
 
 char UtilityFunction::ID;
 
 /// Constructor
FunctionCaller *UtilityFunction::MakeFunctionCaller(
   if (m_caller_up)
     return m_caller_up.get();
 
   ProcessSP process_sp = m_jit_process_wp.lock();
   if (!process_sp) {
     error.SetErrorString("Can't make a function caller without a process.");
     return nullptr;
   }
-  // Since we might need to call allocate memory and maybe call code to make
+  // Since we might need to allocate memory and maybe call code to make
   // the caller, we need to be stopped.
   if (process_sp->GetState() != lldb::eStateStopped) {
-    error.SetErrorString("Can't make a function caller while the process is " 
-                         "running");
+    error.SetErrorStringWithFormatv(
+        "Can't make a function caller while the process is {0}: the process "
+        "must be stopped to allocate memory.",
+        StateAsCString(process_sp->GetState()));
     return nullptr;
   }
 
   Address impl_code_address;
   impl_code_address.SetOffset(StartAddress());
   std::string name(m_function_name);
   name.append("-caller");
 

lldb/test/Shell/Expr/TestExited.test

+# RUN: %clangxx_host %p/Inputs/call-function.cpp -g -o %t
+# RUN: %lldb %t -o 'r' -o 'expr strlen("")' 2>&1 | FileCheck %s
+# CHECK: error: unable to evaluate expression while the process is exited: the process must be stopped because the expression might require allocating memory.