[lldb] do not show misleading error when there is no frame #119103
Conversation
|
@llvm/pr-subscribers-lldb Author: oltolm (oltolm) ChangesI am using VSCode with the official vscode-lldb extension. When I try to list the breakpoints in the debug console get the message: I know that this is wrong and you need to use but the error message is misleading. I cleaned up the code and now the error message is which is still not perfect, but at least it's not misleading. Full diff: https://github.com/llvm/llvm-project/pull/119103.diff 1 Files Affected:
diff --git a/lldb/source/API/SBFrame.cpp b/lldb/source/API/SBFrame.cpp
index 2300bec4d685d2..a5e106c97b1f24 100644
--- a/lldb/source/API/SBFrame.cpp
+++ b/lldb/source/API/SBFrame.cpp
@@ -6,7 +6,6 @@
//
//===----------------------------------------------------------------------===//
-#include <algorithm>
#include <set>
#include <string>
@@ -18,11 +17,8 @@
#include "lldb/Core/Address.h"
#include "lldb/Core/Debugger.h"
#include "lldb/Expression/ExpressionVariable.h"
-#include "lldb/Expression/UserExpression.h"
-#include "lldb/Host/Host.h"
#include "lldb/Symbol/Block.h"
#include "lldb/Symbol/Function.h"
-#include "lldb/Symbol/Symbol.h"
#include "lldb/Symbol/SymbolContext.h"
#include "lldb/Symbol/Variable.h"
#include "lldb/Symbol/VariableList.h"
@@ -33,7 +29,6 @@
#include "lldb/Target/StackFrameRecognizer.h"
#include "lldb/Target/StackID.h"
#include "lldb/Target/Target.h"
-#include "lldb/Target/Thread.h"
#include "lldb/Utility/ConstString.h"
#include "lldb/Utility/Instrumentation.h"
#include "lldb/Utility/LLDBLog.h"
@@ -43,7 +38,6 @@
#include "lldb/ValueObject/ValueObjectVariable.h"
#include "lldb/API/SBAddress.h"
-#include "lldb/API/SBDebugger.h"
#include "lldb/API/SBExpressionOptions.h"
#include "lldb/API/SBFormat.h"
#include "lldb/API/SBStream.h"
@@ -603,11 +597,11 @@ SBValue SBFrame::FindValue(const char *name, ValueType value_type,
stop_if_block_is_inlined_function,
[frame](Variable *v) { return v->IsInScope(frame); },
&variable_list);
- if (value_type == eValueTypeVariableGlobal
- || value_type == eValueTypeVariableStatic) {
+ if (value_type == eValueTypeVariableGlobal ||
+ value_type == eValueTypeVariableStatic) {
const bool get_file_globals = true;
- VariableList *frame_vars = frame->GetVariableList(get_file_globals,
- nullptr);
+ VariableList *frame_vars =
+ frame->GetVariableList(get_file_globals, nullptr);
if (frame_vars)
frame_vars->AppendVariablesIfUnique(variable_list);
}
@@ -790,14 +784,13 @@ SBValueList SBFrame::GetVariables(const lldb::SBVariablesOptions &options) {
const bool statics = options.GetIncludeStatics();
const bool arguments = options.GetIncludeArguments();
const bool recognized_arguments =
- options.GetIncludeRecognizedArguments(SBTarget(exe_ctx.GetTargetSP()));
+ options.GetIncludeRecognizedArguments(SBTarget(exe_ctx.GetTargetSP()));
const bool locals = options.GetIncludeLocals();
const bool in_scope_only = options.GetInScopeOnly();
const bool include_runtime_support_values =
options.GetIncludeRuntimeSupportValues();
const lldb::DynamicValueType use_dynamic = options.GetUseDynamic();
-
std::set<VariableSP> variable_set;
Process *process = exe_ctx.GetProcessPtr();
if (target && process) {
@@ -816,9 +809,11 @@ SBValueList SBFrame::GetVariables(const lldb::SBVariablesOptions &options) {
if (num_variables) {
size_t num_produced = 0;
for (const VariableSP &variable_sp : *variable_list) {
- if (INTERRUPT_REQUESTED(dbg,
- "Interrupted getting frame variables with {0} of {1} "
- "produced.", num_produced, num_variables))
+ if (INTERRUPT_REQUESTED(
+ dbg,
+ "Interrupted getting frame variables with {0} of {1} "
+ "produced.",
+ num_produced, num_variables))
return {};
if (variable_sp) {
@@ -1012,33 +1007,26 @@ bool SBFrame::GetDescription(SBStream &description) {
SBValue SBFrame::EvaluateExpression(const char *expr) {
LLDB_INSTRUMENT_VA(this, expr);
- SBValue result;
std::unique_lock<std::recursive_mutex> lock;
ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
StackFrame *frame = exe_ctx.GetFramePtr();
Target *target = exe_ctx.GetTargetPtr();
+ SBExpressionOptions options;
if (frame && target) {
- SBExpressionOptions options;
lldb::DynamicValueType fetch_dynamic_value =
frame->CalculateTarget()->GetPreferDynamicValue();
options.SetFetchDynamicValue(fetch_dynamic_value);
- options.SetUnwindOnError(true);
- options.SetIgnoreBreakpoints(true);
- SourceLanguage language = target->GetLanguage();
- if (!language)
- language = frame->GetLanguage();
- options.SetLanguage((SBSourceLanguageName)language.name, language.version);
- return EvaluateExpression(expr, options);
- } else {
- Status error;
- error = Status::FromErrorString("can't evaluate expressions when the "
- "process is running.");
- ValueObjectSP error_val_sp =
- ValueObjectConstResult::Create(nullptr, std::move(error));
- result.SetSP(error_val_sp, false);
}
- return result;
+ options.SetUnwindOnError(true);
+ options.SetIgnoreBreakpoints(true);
+ SourceLanguage language;
+ if (target)
+ language = target->GetLanguage();
+ if (!language && frame)
+ language = frame->GetLanguage();
+ options.SetLanguage((SBSourceLanguageName)language.name, language.version);
+ return EvaluateExpression(expr, options);
}
SBValue
@@ -1135,10 +1123,10 @@ lldb::SBValue SBFrame::EvaluateExpression(const char *expr,
expr_result.SetSP(expr_value_sp, false);
}
} else {
- Status error;
- error = Status::FromErrorString("sbframe object is not valid.");
- expr_value_sp = ValueObjectConstResult::Create(nullptr, std::move(error));
- expr_result.SetSP(expr_value_sp, false);
+ Status error;
+ error = Status::FromErrorString("sbframe object is not valid.");
+ expr_value_sp = ValueObjectConstResult::Create(nullptr, std::move(error));
+ expr_result.SetSP(expr_value_sp, false);
}
if (expr_result.GetError().Success())
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
Thanks for not ignoring the formatter - but - please remove the formatting changes in this case because it's obscuring the actual change. When changing existing code like this, we often ignore the formatter for this reason. Also, I'm not sure this is any better. I guess that So if the target were running, The lldb-dap folks (the thing inside the vscode extension) will know more about this, they'll be able to judge better than me. |
|
I imagine that your change does make sense, but it's really not strictly tied to lldb-dap. It's more generic and it's about what happens when the expression evaluator is invoked from SBFrame when the process is not stopped, right? |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Done.
Yes, this message appears even when stopped. I know that "sbframe object is not valid." is not helpful, but at least it is not misleading. I could improve it in this PR if you have any suggestions or leave it for another PR. |
Yes the message "can't evaluate expressions when the process is running." appeared when process was not stopped.
Done.
Sorry, I don't get this sentence. I just push my commits to this branch / PR. |
| std::unique_lock<std::recursive_mutex> lock; | ||
| ExecutionContext exe_ctx(m_opaque_sp.get(), lock); | ||
|
|
||
| StackFrame *frame = exe_ctx.GetFramePtr(); | ||
| Target *target = exe_ctx.GetTargetPtr(); | ||
| SBExpressionOptions options; | ||
| if (frame && target) { |
There was a problem hiding this comment.
Should this still check the target? Maybe something like:
if(frame) {
if (Target* target = frame->CalculateTarget())
lldb::DynamicValueType fetch_dynamic_value = target->GetPreferDynamicValue();
options.SetFetchDynamicValue(fetch_dynamic_value);
}
}
There was a problem hiding this comment.
Is target not the same as frame->CalculateTarget()? Because then this would work
if (target) {
lldb::DynamicValueType fetch_dynamic_value =
target->GetPreferDynamicValue();
options.SetFetchDynamicValue(fetch_dynamic_value);
}There was a problem hiding this comment.
Maybe? I'm not a 100% sure which is why I wasn't suggesting it. Even the StackFrame implementation itself seems pretty inconsistent in whether it uses the execution context or CalculateTarget. I'd keep the existing behavior to avoid regressing anything.
There was a problem hiding this comment.
Then I'd leave it as is. Everywhere else it is done in the same way. E.g.
if (frame && target) {
lldb::DynamicValueType use_dynamic =
frame->CalculateTarget()->GetPreferDynamicValue();
sb_value = GetValueForVariablePath(var_path, use_dynamic);
}
or
if (frame && target) {
lldb::DynamicValueType use_dynamic =
frame->CalculateTarget()->GetPreferDynamicValue();
value = FindVariable(name, use_dynamic);
}
and so on.
lldb/source/API/SBFrame.cpp
Outdated
| @@ -18,11 +17,8 @@ | |||
| #include "lldb/Core/Address.h" | |||
| #include "lldb/Core/Debugger.h" | |||
| #include "lldb/Expression/ExpressionVariable.h" | |||
| #include "lldb/Expression/UserExpression.h" | |||
There was a problem hiding this comment.
Please upstage the header changes too.
I am using VSCode with the official vscode-lldb extension. When I try to list the breakpoints in the debug console get the message:
I know that this is wrong and you need to use
but the error message is misleading. I cleaned up the code and now the error message is
which is still not perfect, but at least it's not misleading.