[Lldb-commits] [PATCH] D151497: [lldb] Improve function caller error message

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.
This revision is now accepted and ready to land.

In D151497#4374495 <https://reviews.llvm.org/D151497#4374495>, @JDevlieghere wrote:

> In D151497#4374080 <https://reviews.llvm.org/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.



================
Comment at: lldb/source/Expression/UserExpression.cpp:207-208
+
+  // Since we might need to call allocate memory, we need to be stopped to run
+  // an expression.
   if (process != nullptr && process->GetState() != lldb::eStateStopped) {
----------------



================
Comment at: lldb/source/Expression/UtilityFunction.cpp:68-69
   }
   // Since we might need to call allocate memory and maybe call code to make
   // the caller, we need to be stopped.
   if (process_sp->GetState() != lldb::eStateStopped) {
----------------



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151497/new/

https://reviews.llvm.org/D151497