[LLDB][NFC] Remove redundant target/process checks in SBFrame #153258
Conversation
|
@llvm/pr-subscribers-lldb Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesThis is a follow up to #152020, continuing the removal of now-redundant This has exposed some odd behaviors:
To stick to the strictly NFC nature of this patch, it does not address any of these issues. Patch is 22.53 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/153258.diff 2 Files Affected:
diff --git a/lldb/include/lldb/API/SBFrame.h b/lldb/include/lldb/API/SBFrame.h
index 08de0605b3240..2666862a8c405 100644
--- a/lldb/include/lldb/API/SBFrame.h
+++ b/lldb/include/lldb/API/SBFrame.h
@@ -13,6 +13,8 @@
#include "lldb/API/SBValueList.h"
namespace lldb_private {
+class StackFrame;
+class Debugger;
namespace python {
class SWIGBridge;
}
@@ -237,6 +239,23 @@ class LLDB_API SBFrame {
/// not currently stopped.
static SBValue CreateProcessIsRunningExprEvalError();
+ enum WasInterrupted { Yes, No };
+
+ /// Populates `value_list` with the variables from `frame` according to
+ /// `options`. This method checks whether the Debugger received an interrupt
+ /// before processing every variable, returning `WasInterrupted::yes` in that
+ /// case.
+ static WasInterrupted FetchVariablesUnlessInterrupted(
+ const lldb::SBVariablesOptions &options, lldb_private::StackFrame &frame,
+ SBValueList &value_list, lldb_private::Debugger &dbg);
+
+ /// Populates `value_list` with recognized arguments of `frame` according to
+ /// `options`.
+ static void FetchRecognizedArguments(const SBVariablesOptions &options,
+ lldb_private::StackFrame &frame,
+ SBTarget target,
+ SBValueList &value_list);
+
lldb::ExecutionContextRefSP m_opaque_sp;
};
diff --git a/lldb/source/API/SBFrame.cpp b/lldb/source/API/SBFrame.cpp
index b12cfceacd75d..3c0087dc719e2 100644
--- a/lldb/source/API/SBFrame.cpp
+++ b/lldb/source/API/SBFrame.cpp
@@ -490,99 +490,85 @@ SBValue SBFrame::FindValue(const char *name, ValueType value_type,
if (!exe_ctx) {
LLDB_LOG_ERROR(GetLog(LLDBLog::API), exe_ctx.takeError(), "{0}");
return value_sp;
- } else {
- Target *target = exe_ctx->GetTargetPtr();
- Process *process = exe_ctx->GetProcessPtr();
- if (target && process) { // FIXME: this check is redundant.
- if (StackFrame *frame = exe_ctx->GetFramePtr()) {
- VariableList variable_list;
-
- switch (value_type) {
- case eValueTypeVariableGlobal: // global variable
- case eValueTypeVariableStatic: // static variable
- case eValueTypeVariableArgument: // function argument variables
- case eValueTypeVariableLocal: // function local variables
- case eValueTypeVariableThreadLocal: // thread local variables
- {
- SymbolContext sc(frame->GetSymbolContext(eSymbolContextBlock));
-
- const bool can_create = true;
- const bool get_parent_variables = true;
- const bool stop_if_block_is_inlined_function = true;
-
- if (sc.block)
- sc.block->AppendVariables(
- can_create, get_parent_variables,
- stop_if_block_is_inlined_function,
- [frame](Variable *v) { return v->IsInScope(frame); },
- &variable_list);
- if (value_type == eValueTypeVariableGlobal
- || value_type == eValueTypeVariableStatic) {
- const bool get_file_globals = true;
- VariableList *frame_vars = frame->GetVariableList(get_file_globals,
- nullptr);
- if (frame_vars)
- frame_vars->AppendVariablesIfUnique(variable_list);
- }
- ConstString const_name(name);
- VariableSP variable_sp(
- variable_list.FindVariable(const_name, value_type));
- if (variable_sp) {
- value_sp = frame->GetValueObjectForFrameVariable(variable_sp,
- eNoDynamicValues);
- sb_value.SetSP(value_sp, use_dynamic);
- }
- } break;
-
- case eValueTypeRegister: // stack frame register value
- {
- RegisterContextSP reg_ctx(frame->GetRegisterContext());
- if (reg_ctx) {
- if (const RegisterInfo *reg_info =
- reg_ctx->GetRegisterInfoByName(name)) {
- value_sp = ValueObjectRegister::Create(frame, reg_ctx, reg_info);
- sb_value.SetSP(value_sp);
- }
- }
- } break;
-
- case eValueTypeRegisterSet: // A collection of stack frame register
- // values
- {
- RegisterContextSP reg_ctx(frame->GetRegisterContext());
- if (reg_ctx) {
- const uint32_t num_sets = reg_ctx->GetRegisterSetCount();
- for (uint32_t set_idx = 0; set_idx < num_sets; ++set_idx) {
- const RegisterSet *reg_set = reg_ctx->GetRegisterSet(set_idx);
- if (reg_set &&
- (llvm::StringRef(reg_set->name).equals_insensitive(name) ||
- llvm::StringRef(reg_set->short_name)
- .equals_insensitive(name))) {
- value_sp =
- ValueObjectRegisterSet::Create(frame, reg_ctx, set_idx);
- sb_value.SetSP(value_sp);
- break;
- }
- }
- }
- } break;
-
- case eValueTypeConstResult: // constant result variables
- {
- ConstString const_name(name);
- ExpressionVariableSP expr_var_sp(
- target->GetPersistentVariable(const_name));
- if (expr_var_sp) {
- value_sp = expr_var_sp->GetValueObject();
- sb_value.SetSP(value_sp, use_dynamic);
- }
- } break;
-
- default:
+ }
+
+ StackFrame *frame = exe_ctx->GetFramePtr();
+ if (!frame)
+ return value_sp;
+
+ VariableList variable_list;
+
+ switch (value_type) {
+ case eValueTypeVariableGlobal: // global variable
+ case eValueTypeVariableStatic: // static variable
+ case eValueTypeVariableArgument: // function argument variables
+ case eValueTypeVariableLocal: // function local variables
+ case eValueTypeVariableThreadLocal: { // thread local variables
+ SymbolContext sc(frame->GetSymbolContext(eSymbolContextBlock));
+
+ const bool can_create = true;
+ const bool get_parent_variables = true;
+ const bool stop_if_block_is_inlined_function = true;
+
+ if (sc.block)
+ sc.block->AppendVariables(
+ can_create, get_parent_variables, stop_if_block_is_inlined_function,
+ [frame](Variable *v) { return v->IsInScope(frame); }, &variable_list);
+ if (value_type == eValueTypeVariableGlobal ||
+ value_type == eValueTypeVariableStatic) {
+ const bool get_file_globals = true;
+ VariableList *frame_vars =
+ frame->GetVariableList(get_file_globals, nullptr);
+ if (frame_vars)
+ frame_vars->AppendVariablesIfUnique(variable_list);
+ }
+ ConstString const_name(name);
+ VariableSP variable_sp(variable_list.FindVariable(const_name, value_type));
+ if (variable_sp) {
+ value_sp =
+ frame->GetValueObjectForFrameVariable(variable_sp, eNoDynamicValues);
+ sb_value.SetSP(value_sp, use_dynamic);
+ }
+ } break;
+
+ case eValueTypeRegister: { // stack frame register value
+ if (RegisterContextSP reg_ctx = frame->GetRegisterContext()) {
+ if (const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoByName(name)) {
+ value_sp = ValueObjectRegister::Create(frame, reg_ctx, reg_info);
+ sb_value.SetSP(value_sp);
+ }
+ }
+ } break;
+
+ case eValueTypeRegisterSet: { // A collection of stack frame register
+ // values
+ if (RegisterContextSP reg_ctx = frame->GetRegisterContext()) {
+ const uint32_t num_sets = reg_ctx->GetRegisterSetCount();
+ for (uint32_t set_idx = 0; set_idx < num_sets; ++set_idx) {
+ const RegisterSet *reg_set = reg_ctx->GetRegisterSet(set_idx);
+ if (reg_set &&
+ (llvm::StringRef(reg_set->name).equals_insensitive(name) ||
+ llvm::StringRef(reg_set->short_name).equals_insensitive(name))) {
+ value_sp = ValueObjectRegisterSet::Create(frame, reg_ctx, set_idx);
+ sb_value.SetSP(value_sp);
break;
}
}
}
+ } break;
+
+ case eValueTypeConstResult: { // constant result variables
+ ConstString const_name(name);
+ Target *target = exe_ctx->GetTargetPtr();
+ ExpressionVariableSP expr_var_sp(target->GetPersistentVariable(const_name));
+ if (expr_var_sp) {
+ value_sp = expr_var_sp->GetValueObject();
+ sb_value.SetSP(value_sp, use_dynamic);
+ }
+ } break;
+
+ default:
+ break;
}
return sb_value;
@@ -698,169 +684,184 @@ lldb::SBValueList SBFrame::GetVariables(bool arguments, bool locals,
return GetVariables(options);
}
+/// Returns true if the variable is in any of the requested scopes.
+static bool IsInRequestedScope(bool statics, bool arguments, bool locals,
+ Variable &var) {
+ switch (var.GetScope()) {
+ case eValueTypeVariableGlobal:
+ case eValueTypeVariableStatic:
+ case eValueTypeVariableThreadLocal:
+ return statics;
+
+ case eValueTypeVariableArgument:
+ return arguments;
+
+ case eValueTypeVariableLocal:
+ return locals;
+
+ default:
+ break;
+ }
+ return false;
+}
+
+SBFrame::WasInterrupted SBFrame::FetchVariablesUnlessInterrupted(
+ const lldb::SBVariablesOptions &options, StackFrame &frame,
+ SBValueList &value_list, Debugger &dbg) {
+ const bool statics = options.GetIncludeStatics();
+ const bool arguments = options.GetIncludeArguments();
+ 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();
+
+ Status var_error;
+ VariableList *variable_list = frame.GetVariableList(true, &var_error);
+
+ std::set<VariableSP> variable_set;
+
+ if (var_error.Fail())
+ value_list.SetError(std::move(var_error));
+
+ if (!variable_list)
+ return WasInterrupted::No;
+ const size_t num_variables = variable_list->GetSize();
+ size_t num_produced = 0;
+ for (const VariableSP &variable_sp : *variable_list) {
+ if (!variable_sp ||
+ !IsInRequestedScope(statics, arguments, locals, *variable_sp))
+ continue;
+
+ if (INTERRUPT_REQUESTED(
+ dbg,
+ "Interrupted getting frame variables with {0} of {1} "
+ "produced.",
+ num_produced, num_variables))
+ return WasInterrupted::Yes;
+
+ // Only add variables once so we don't end up with duplicates
+ if (variable_set.insert(variable_sp).second == false)
+ continue;
+ if (in_scope_only && !variable_sp->IsInScope(&frame))
+ continue;
+
+ ValueObjectSP valobj_sp(
+ frame.GetValueObjectForFrameVariable(variable_sp, eNoDynamicValues));
+
+ if (!include_runtime_support_values && valobj_sp != nullptr &&
+ valobj_sp->IsRuntimeSupportValue())
+ continue;
+
+ SBValue value_sb;
+ value_sb.SetSP(valobj_sp, use_dynamic);
+ value_list.Append(value_sb);
+ }
+ num_produced++;
+
+ return WasInterrupted::No;
+}
+
+void SBFrame::FetchRecognizedArguments(const SBVariablesOptions &options,
+ StackFrame &frame, SBTarget target,
+ SBValueList &value_list) {
+ if (!options.GetIncludeRecognizedArguments(target))
+ return;
+ RecognizedStackFrameSP recognized_frame = frame.GetRecognizedFrame();
+ if (!recognized_frame)
+ return;
+
+ ValueObjectListSP recognized_arg_list =
+ recognized_frame->GetRecognizedArguments();
+ if (!recognized_arg_list)
+ return;
+
+ const lldb::DynamicValueType use_dynamic = options.GetUseDynamic();
+ for (auto &rec_value_sp : recognized_arg_list->GetObjects()) {
+ SBValue value_sb;
+ value_sb.SetSP(rec_value_sp, use_dynamic);
+ value_list.Append(value_sb);
+ }
+}
+
SBValueList SBFrame::GetVariables(const lldb::SBVariablesOptions &options) {
LLDB_INSTRUMENT_VA(this, options);
- SBValueList value_list;
llvm::Expected<StoppedExecutionContext> exe_ctx =
GetStoppedExecutionContext(m_opaque_sp);
if (!exe_ctx) {
LLDB_LOG_ERROR(GetLog(LLDBLog::API), exe_ctx.takeError(), "{0}");
return SBValueList();
- } else {
- const bool statics = options.GetIncludeStatics();
- const bool arguments = options.GetIncludeArguments();
- const bool recognized_arguments =
- 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 (process) { // FIXME: this check is redundant.
- if (StackFrame *frame = exe_ctx->GetFramePtr()) {
- Debugger &dbg = process->GetTarget().GetDebugger();
- VariableList *variable_list = nullptr;
- Status var_error;
- variable_list = frame->GetVariableList(true, &var_error);
- if (var_error.Fail())
- value_list.SetError(std::move(var_error));
- if (variable_list) {
- const size_t num_variables = variable_list->GetSize();
- 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))
- return {};
-
- if (variable_sp) {
- bool add_variable = false;
- switch (variable_sp->GetScope()) {
- case eValueTypeVariableGlobal:
- case eValueTypeVariableStatic:
- case eValueTypeVariableThreadLocal:
- add_variable = statics;
- break;
-
- case eValueTypeVariableArgument:
- add_variable = arguments;
- break;
-
- case eValueTypeVariableLocal:
- add_variable = locals;
- break;
-
- default:
- break;
- }
- if (add_variable) {
- // Only add variables once so we don't end up with duplicates
- if (variable_set.find(variable_sp) == variable_set.end())
- variable_set.insert(variable_sp);
- else
- continue;
-
- if (in_scope_only && !variable_sp->IsInScope(frame))
- continue;
-
- ValueObjectSP valobj_sp(frame->GetValueObjectForFrameVariable(
- variable_sp, eNoDynamicValues));
-
- if (!include_runtime_support_values && valobj_sp != nullptr &&
- valobj_sp->IsRuntimeSupportValue())
- continue;
-
- SBValue value_sb;
- value_sb.SetSP(valobj_sp, use_dynamic);
- value_list.Append(value_sb);
- }
- }
- }
- num_produced++;
- }
- }
- if (recognized_arguments) {
- auto recognized_frame = frame->GetRecognizedFrame();
- if (recognized_frame) {
- ValueObjectListSP recognized_arg_list =
- recognized_frame->GetRecognizedArguments();
- if (recognized_arg_list) {
- for (auto &rec_value_sp : recognized_arg_list->GetObjects()) {
- SBValue value_sb;
- value_sb.SetSP(rec_value_sp, use_dynamic);
- value_list.Append(value_sb);
- }
- }
- }
- }
- }
- }
}
+ StackFrame *frame = exe_ctx->GetFramePtr();
+ if (!frame)
+ return SBValueList();
+
+ SBValueList value_list;
+ if (WasInterrupted::Yes ==
+ FetchVariablesUnlessInterrupted(options, *frame, value_list,
+ exe_ctx->GetTargetPtr()->GetDebugger()))
+ return value_list;
+
+ FetchRecognizedArguments(options, *frame, SBTarget(exe_ctx->GetTargetSP()),
+ value_list);
return value_list;
}
SBValueList SBFrame::GetRegisters() {
LLDB_INSTRUMENT_VA(this);
- SBValueList value_list;
llvm::Expected<StoppedExecutionContext> exe_ctx =
GetStoppedExecutionContext(m_opaque_sp);
if (!exe_ctx) {
LLDB_LOG_ERROR(GetLog(LLDBLog::API), exe_ctx.takeError(), "{0}");
return SBValueList();
- } else {
- Target *target = exe_ctx->GetTargetPtr();
- Process *process = exe_ctx->GetProcessPtr();
- if (target && process) { // FIXME: this check is redundant.
- if (StackFrame *frame = exe_ctx->GetFramePtr()) {
- RegisterContextSP reg_ctx(frame->GetRegisterContext());
- if (reg_ctx) {
- const uint32_t num_sets = reg_ctx->GetRegisterSetCount();
- for (uint32_t set_idx = 0; set_idx < num_sets; ++set_idx) {
- value_list.Append(
- ValueObjectRegisterSet::Create(frame, reg_ctx, set_idx));
- }
- }
- }
- }
}
+ StackFrame *frame = exe_ctx->GetFramePtr();
+ if (!frame)
+ return SBValueList();
+
+ RegisterContextSP reg_ctx(frame->GetRegisterContext());
+ if (!reg_ctx)
+ return SBValueList();
+
+ SBValueList value_list;
+ const uint32_t num_sets = reg_ctx->GetRegisterSetCount();
+ for (uint32_t set_idx = 0; set_idx < num_sets; ++set_idx)
+ value_list.Append(ValueObjectRegisterSet::Create(frame, reg_ctx, set_idx));
+
return value_list;
}
SBValue SBFrame::FindRegister(const char *name) {
LLDB_INSTRUMENT_VA(this, name);
- SBValue result;
ValueObjectSP value_sp;
llvm::Expected<StoppedExecutionContext> exe_ctx =
GetStoppedExecutionContext(m_opaque_sp);
if (!exe_ctx) {
LLDB_LOG_ERROR(GetLog(LLDBLog::API), exe_ctx.takeError(), "{0}");
return SBValue();
- } else {
- Target *target = exe_ctx->GetTargetPtr();
- Process *process = exe_ctx->GetProcessPtr();
- if (target && process) { // FIXME: this check is redundant.
- if (StackFrame *frame = exe_ctx->GetFramePtr()) {
- RegisterContextSP reg_ctx(frame->GetRegisterContext());
- if (reg_ctx) {
- if (const RegisterInfo *reg_info =
- reg_ctx->GetRegisterInfoByName(name)) {
- value_sp = ValueObjectRegister::Create(frame, reg_ctx, reg_info);
- result.SetSP(value_sp);
- }
- }
- }
- }
}
+ StackFrame *frame = exe_ctx->GetFramePtr();
+ if (!frame)
+ return SBValue();
+
+ RegisterContextSP reg_ctx(frame->GetRegisterContext());
+ if (!reg_ctx)
+ return SBValue();
+
+ const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoByName(name);
+ if (!reg_info)
+ return SBValue();
+
+ SBValue result;
+ value_sp = ValueObjectRegister::Create(frame, reg_ctx, reg_info);
+ result.SetSP(value_sp);
+
return result;
}
@@ -1000,58 +1001,55 @@ lldb::SBValue SBFrame::EvaluateExpression(const char *expr,
const SBExpressionOptions &options) {
LLDB_INSTRUMENT_VA(this, expr, options);
- Log *expr_log = GetLog(LLDBLog::Expressions);
-
- SBValue expr_result;
+ auto LogResult = [](SBValue expr_result) {
+ Log *expr_log = GetLog(LLDBLog::Expressions);
+ if (expr_result.GetError().Success())
+ LLDB_LOGF(expr_log,
+ "** [SBFrame::EvaluateExpression] Expression result is "
+ "%s, summary %s **",
+ expr_result.GetValue(), expr_result.GetSummary());
+ else
+ LLDB_LOGF(
+ expr_log,
+ "** [SBFrame::EvaluateExpression] Expression evaluation failed: "
+ "%s **",
+ expr_result.GetError().GetCString());
+ };
if (expr == nullptr || expr[0] == '\0') {
- return expr_result;
+ return SBValue();
}
- ValueObjectSP expr_value_sp;
-
llvm::Expected<StoppedExecutionContext> exe_ctx =
GetStoppedExecutionContext(m_opaque_sp);
if (!exe_ctx) {
LLDB_LOG_ERROR(GetLog(LLDBLog::API), exe_ctx.takeError(), "{0}");
- expr_result = CreateProcessIsRunni...
[truncated]
|
lldb/include/lldb/API/SBFrame.h
Outdated
There was a problem hiding this comment.
Is there a way we can do this without adding private classes to the SBFrame header? Maybe these methods could be put into StackFrame directly? I'd prefer to not add private details to the public header unless absolutely necessary.
There was a problem hiding this comment.
I didn't want to do this either, but right now I don't see a way around it. This is the current challenge with SB classes: they are all friends of each other so that they can expose helper functions to other SB classes using private visibility (and therefore not show up in the python API). This design makes it impossible to have internal free functions, as they wouldn't have access to those private methods (and you can't declare a friend method that will have internal linkage).
In this particular case, we have a bunch of:
SBValue value_sb;
value_sb.SetSP(valobj_sp, use_dynamic);
value_list.Append(value_sb);
inside the would-be internal-linkage helper function. The SetSP method is private.
I'll try to change the signature slightly to work around the issue.
There was a problem hiding this comment.
This doesn't seem possible... I tried moving the parts that require the friend access back to the SBFrame method, but the call to value_sb.SetSP(valobj_sp, use_dynamic); needs to happen inside the loop guarded by the interrupt checks...
There was a problem hiding this comment.
Could we move some of this logic into the private layer? Perhaps some of this PR could be to move things into StackFrame, for example.
There was a problem hiding this comment.
Probably, but I don't think it would be an NFC change, because of the constraints imposed by the interrupt mechanism. Anything that is hoisting things out of the SB layer will invariably change the interrupt behavior, since we have a loop whose code crosses the private/SB layers and this loop can be interrupted.
If we want to go down that path, I can inline this function back into the callsite and then in the future we try to come up with a solution for this.
There was a problem hiding this comment.
I've removed FetchRecognizedArguments from the public header by having it return a vector of ValueObject, and having the call site deal with the private members of the SB APIs
This is a follow up to llvm#152020, continuing the removal of now-redundant `if(process && target)` checks. Since this causes a diff in every line of the affected functions, this commit also uses the opportunity to create some helper functions and reduce nesting of the affected methods by rewriting all pre-condition checks as early returns, while remaining strictly NFC. This has exposed some odd behaviors: 1. `SBFrame::GetVariables` has a variable `num_produced` which is clearly meant to be incremented on every iteration of the loop but it is only incremented once, after the loop. So its value is always 0 or 1. The variable now lives in `FetchVariablesUnlessInterrupted`. 2. `SBFrame::GetVariables` has an interruption mechanism for local variables, but not for "recognized arguments". It's unclear if this is by design or not, but it is now evident that there is a discrepancy there. 3. In `SBFrame::EvaluateExpression` we only log some error paths, but not all of them. To stick to the strictly NFC nature of this patch, it does not address any of these issues.
felipepiovezan
force-pushed
the
felipe/execution_ctx_followup2
branch
from
8f9be2f to
8cbecc7
Compare
|
This is not your fault, but that much code in an SBAnything.cpp always smells wrong. But I don't see why Interruption causes a problem with moving this into StackFrame. You could have a StackFrame method that makes up a ValueObjectList of everything that's going to be fetched and return that to the SBFrame method. Then all the SBFrame method does is convert the ValueObjects in the list to SBValues and inserts them into the returned SBValueList. If the StackFrame method got interrupted, it would just stop adding ValueObjects to the list and return what it had. The SBFrame method shouldn't have to care about interruption then, it would just convert whatever it got. That's such a quick operation, interrupting it isn't worth the effort. |
|
Interrupt detection works the same whether you are detecting it at the SB Layer or the lldb_private layer. So that is not a functional change. |
|
I think we need to talk concretely to prove (or disprove!) my point. Let's consider the main loop we're thinking about hoisting to SBFrame: The argument Jim and Alex are making is that most of this loop can be put into a StackFrame method, which would return a list of So this would have be a separate loop inside the SB class, converting the ValueObjects into an SBValues; that's not a problem. What happens if an interrupt arrives? It would be processed inside the StackFrame loop, which would then early return with a partial list of (*)This is where my argument might be faulty: if that's a cheap operation, we can do that even in the presence of the interrupt. |
|
My opinion was: "The SBFrame method shouldn't have to care about interruption then, it would just convert whatever it got. That's such a quick operation, interrupting it isn't worth the effort." The current lldb Interrupt handling is voluntary. You are only required to check for a requested interrupt at safe/useful interrupt points. So the SBFrame code wouldn't need to check for interrupts itself because the operation it would be interrupting is not useful to interrupt. |
|
I don't have a strong opinion on returning a partial list of If you want to return a partial list, you call the private method (which may be interrupted) and convert the partial list of |
|
And to the (*) point, SBValue::SetSP takes the incoming ValueObjectSP, makes a ValueImpl which just copies over the It always uses this branch: If you have a dynamic value you have to have already computed the static value, so GetStaticValue doesn't require computation. If it weren't, we would have had to change that. It makes little sense to front-load the expensive computations when you're just trying to get an SBValue representing the ValueObject, and haven't done anything with it yet. |
|
I'm going to propose a middle ground here: if we feel strongly about the changes in the I'm going to motivate this with a wall of text below, but you're also welcome to skip it and take my word for it :) The following might need to become a filter into an SBFrame method: This feels like it should not exist at all: I removed it (locally, not in this PR) and no tests failed. This makes me think we're calling the wrong SBFrame method: The above already exists in SBFrame: it's called Regarding the interruption, we may need a new method entirely in StackFrame, as opposed to reusing the two above, or change the API to have some sort of "should interrupt" function ptr that can be given a "never interrupt" for other call sites. We also need to find a place for: Maybe it would require a new method in StackFrame. And finally there's this: This might also be part of the filter function I described earlier? Here's how I want to approach this:
And then look at what we're left with and try to design a new function for StackFrame / update the existing ones. |
|
I've updated the PR with some creative solutions to get rid of the SBFrame.h changes (you can see the diff on the last fixup commit). I personally, prefer the old approach -- as it would be temporary -- but I'll take any compromises to get this pr going |
|
Provided this is only temporary, I'm fine with either version. In my mind they are no uglier than having all this code in an SB implementation file and thus effectively lost to the rest of lldb, which was the situation you started with. |
|
@bulbazord what do you think? |
This is a follow up to #152020, continuing the removal of now-redundant
if(process && target)checks. Since this causes a diff in every line of the affected functions, this commit also uses the opportunity to create some helper functions and reduce nesting of the affected methods by rewriting all pre-condition checks as early returns, while remaining strictly NFC.This has exposed some odd behaviors:
SBFrame::GetVariableshas a variablenum_producedwhich is clearly meant to be incremented on every iteration of the loop but it is only incremented once, after the loop. So its value is always 0 orFetchVariablesUnlessInterrupted.SBFrame::GetVariableshas an interruption mechanism for local variables, but not for "recognized arguments". It's unclear if this is by design or not, but it is now evident that there is a discrepancy there.SBFrame::EvaluateExpressionwe only log some error paths, but not all of them.To stick to the strictly NFC nature of this patch, it does not address any of these issues.