This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Expression] Improve interpreter error message with a non-running target
ClosedPublic

Authored by mib on Jan 10 2020, 7:51 AM.

Details

Summary

[lldb/Expression] Improve interpreter error message with a non-running target

When trying to interpret an expression with a function call, if the
process hasn't been launched, the expression fails to be interpreted
and the user gets the following error message:

error: Can't run the expression locally

This message doesn't explain why the expression failed to be
interpreted, that's why this patch improves the error message that is
displayed when trying to run an expression while no process is running.

rdar://11991708

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Jan 10 2020, 7:51 AM
mib edited the summary of this revision. (Show Details)Jan 10 2020, 7:52 AM
mib edited the summary of this revision. (Show Details)Jan 10 2020, 7:58 AM
mib retitled this revision from [lldb/Expression] Improve interpreter error message with a non-running process to [lldb/Expression] Improve interpreter error message with a non-running target.Jan 10 2020, 8:03 AM
vsk added a subscriber: vsk.Jan 10 2020, 8:06 AM

Nice. So, does !support_function_calls always imply that the target isn’t running?

Also — s/interprete/interpret/, I think.

mib added a comment.Jan 10 2020, 8:42 AM

Actually, the target could be running but doesn't support interpreting expression with function calls.

May be I should add another error message for target that doesn't support interpreting function calls.

teemperor requested changes to this revision.Jan 13 2020, 12:25 AM
teemperor added a subscriber: teemperor.

To give this more context: The IRInterpreter can interpret function calls but only on Hexagon (see D9404). The reason why we essentially always see this error message when the target isn't running is because most people don't target Hexagon and function calls is the most common reason IRInterpreter::CanInterpret returns an error. However, it *could* also fail for any of the other error messages in this function.

So while the idea of the patch is great (the error message is very confusing), this is not the right place to mention a non-running target. We ask the IRInterpretet if it can interpret the expression and it should answer with why it can't interpret it. The reason is that it can't interpret function calls (in this regard the error message can be improved here). If we start talking about a running target here then we also need to add this same text to all other error messages (in fact, we would need to replace all other error message with this to be consistent). It also makes the error message an oxymoron:

error: Interpreting the expression locally failed:
          Interpreter requires to run the program to interprete the expression

I would go up to the ClangExpressionParser and fix the error message there:

if (!can_interpret && execution_policy == eExecutionPolicyNever) {
  err.SetErrorStringWithFormat("Can't run the expression locally: %s",
                               interpret_error.AsCString());
  return err;
}

This could be something like "Can't interpret the expression without a running target. Interpretation failed due to: %s" or something like that.

This revision now requires changes to proceed.Jan 13 2020, 12:25 AM
mib updated this revision to Diff 237911.Jan 14 2020, 3:27 AM
mib retitled this revision from [lldb/Expression] Improve interpreter error message with a non-running target to [lldb/Expression] Improve interpreter error message with a non-running target.
mib edited the summary of this revision. (Show Details)

Moved error to ClangExpressionParser instead of IRInterpreter.

mib updated this revision to Diff 237917.Jan 14 2020, 3:42 AM
mib edited the summary of this revision. (Show Details)

Update test.

teemperor accepted this revision.Jan 14 2020, 3:49 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jan 14 2020, 3:49 AM
This revision was automatically updated to reflect the committed changes.