[lldb] Unify the way we get the Target in CommandObject #101208
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, CommandObjects are obtaining a target in a variety of ways. Often the command incorrectly operates on the selected target. As an example, when a breakpoint command is running, the current target is passed into the command but the target that hit the breakpoint is not the selected target. In other places we use the CommandObject's execution context, which is frozen during the execution of the command, which comes with its own limitations. And of course there's all the places where we need to use, or to fallback on the dummy target Instead of having to guess how to get the target, this patch introduces one helper function in CommandObject to get the most relevant target. In order of priority, that's the target from the command object's execution context, from the interpreter's execution context, the selected target or the dummy target. This function always return the dummy target if explicitly requested. rdar://110846511
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesCurrently, CommandObjects are obtaining a target in a variety of ways. Often the command incorrectly operates on the selected target. As an example, when a breakpoint command is running, the current target is passed into the command but the target that hit the breakpoint is not the selected target. In other places we use the CommandObject's execution context, which is frozen during the execution of the command, which comes with its own limitations. And of course there's all the places where we need to use, or to fallback on the dummy target Instead of having to guess how to get the target, this patch introduces one helper function in CommandObject to get the most relevant target. In order of priority, that's the target from the command object's execution context, from the interpreter's execution context, the selected target or the dummy target. This function always return the dummy target if explicitly requested. rdar://110846511 Patch is 36.00 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/101208.diff 14 Files Affected:
diff --git a/lldb/include/lldb/Interpreter/CommandObject.h b/lldb/include/lldb/Interpreter/CommandObject.h
index d48dbcdd5a5da..16d89f1e5db87 100644
--- a/lldb/include/lldb/Interpreter/CommandObject.h
+++ b/lldb/include/lldb/Interpreter/CommandObject.h
@@ -369,12 +369,12 @@ class CommandObject : public std::enable_shared_from_this<CommandObject> {
"currently stopped.";
}
- // This is for use in the command interpreter, when you either want the
- // selected target, or if no target is present you want to prime the dummy
- // target with entities that will be copied over to new targets.
- Target &GetSelectedOrDummyTarget(bool prefer_dummy = false);
- Target &GetSelectedTarget();
- Target &GetDummyTarget();
+ // This is for use in the command interpreter and returns the most relevant
+ // target. In order of priority, that's the target from the command object's
+ // execution context, from the interpreter's execution context, the selected
+ // target or the dummy target. This function always return the dummy target if
+ // explicitly requested.
+ Target &GetTarget(bool dummy = false);
// If a command needs to use the "current" thread, use this call. Command
// objects will have an ExecutionContext to use, and that may or may not have
diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index 773f8ed2fa8af..40be0cf02cd35 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -539,7 +539,7 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target &target = GetSelectedOrDummyTarget(m_dummy_options.m_use_dummy);
+ Target &target = GetTarget(m_dummy_options.m_use_dummy);
// The following are the various types of breakpoints that could be set:
// 1). -f -l -p [-s -g] (setting breakpoint by source location)
@@ -747,7 +747,7 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
const bool show_locations = false;
bp_sp->GetDescription(&output_stream, lldb::eDescriptionLevelInitial,
show_locations);
- if (&target == &GetDummyTarget())
+ if (&target == &GetTarget(/*dummy=*/true))
output_stream.Printf("Breakpoint set in dummy target, will get copied "
"into future targets.\n");
else {
@@ -839,7 +839,7 @@ class CommandObjectBreakpointModify : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target &target = GetSelectedOrDummyTarget(m_dummy_opts.m_use_dummy);
+ Target &target = GetTarget(m_dummy_opts.m_use_dummy);
std::unique_lock<std::recursive_mutex> lock;
target.GetBreakpointList().GetListMutex(lock);
@@ -903,7 +903,7 @@ class CommandObjectBreakpointEnable : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target &target = GetSelectedOrDummyTarget();
+ Target &target = GetTarget();
std::unique_lock<std::recursive_mutex> lock;
target.GetBreakpointList().GetListMutex(lock);
@@ -1010,7 +1010,7 @@ the second re-enables the first location.");
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target &target = GetSelectedOrDummyTarget();
+ Target &target = GetTarget();
std::unique_lock<std::recursive_mutex> lock;
target.GetBreakpointList().GetListMutex(lock);
@@ -1148,7 +1148,7 @@ class CommandObjectBreakpointList : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target &target = GetSelectedOrDummyTarget(m_options.m_use_dummy);
+ Target &target = GetTarget(m_options.m_use_dummy);
const BreakpointList &breakpoints =
target.GetBreakpointList(m_options.m_internal);
@@ -1267,7 +1267,7 @@ class CommandObjectBreakpointClear : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target &target = GetSelectedOrDummyTarget();
+ Target &target = GetTarget();
// The following are the various types of breakpoints that could be
// cleared:
@@ -1416,7 +1416,7 @@ class CommandObjectBreakpointDelete : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target &target = GetSelectedOrDummyTarget(m_options.m_use_dummy);
+ Target &target = GetTarget(m_options.m_use_dummy);
result.Clear();
std::unique_lock<std::recursive_mutex> lock;
@@ -1676,7 +1676,7 @@ class CommandObjectBreakpointNameConfigure : public CommandObjectParsed {
return;
}
- Target &target = GetSelectedOrDummyTarget(false);
+ Target &target = GetTarget(false);
std::unique_lock<std::recursive_mutex> lock;
target.GetBreakpointList().GetListMutex(lock);
@@ -1763,8 +1763,7 @@ class CommandObjectBreakpointNameAdd : public CommandObjectParsed {
return;
}
- Target &target =
- GetSelectedOrDummyTarget(m_name_options.m_use_dummy.GetCurrentValue());
+ Target &target = GetTarget(m_name_options.m_use_dummy.GetCurrentValue());
std::unique_lock<std::recursive_mutex> lock;
target.GetBreakpointList().GetListMutex(lock);
@@ -1837,8 +1836,7 @@ class CommandObjectBreakpointNameDelete : public CommandObjectParsed {
return;
}
- Target &target =
- GetSelectedOrDummyTarget(m_name_options.m_use_dummy.GetCurrentValue());
+ Target &target = GetTarget(m_name_options.m_use_dummy.GetCurrentValue());
std::unique_lock<std::recursive_mutex> lock;
target.GetBreakpointList().GetListMutex(lock);
@@ -1896,8 +1894,7 @@ class CommandObjectBreakpointNameList : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target &target =
- GetSelectedOrDummyTarget(m_name_options.m_use_dummy.GetCurrentValue());
+ Target &target = GetTarget(m_name_options.m_use_dummy.GetCurrentValue());
std::vector<std::string> name_list;
if (command.empty()) {
@@ -2209,7 +2206,7 @@ class CommandObjectBreakpointRead : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target &target = GetSelectedOrDummyTarget();
+ Target &target = GetTarget();
std::unique_lock<std::recursive_mutex> lock;
target.GetBreakpointList().GetListMutex(lock);
@@ -2319,7 +2316,7 @@ class CommandObjectBreakpointWrite : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target &target = GetSelectedOrDummyTarget();
+ Target &target = GetTarget();
std::unique_lock<std::recursive_mutex> lock;
target.GetBreakpointList().GetListMutex(lock);
diff --git a/lldb/source/Commands/CommandObjectBreakpointCommand.cpp b/lldb/source/Commands/CommandObjectBreakpointCommand.cpp
index 6ebe6e8a35570..fb352e70c2355 100644
--- a/lldb/source/Commands/CommandObjectBreakpointCommand.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpointCommand.cpp
@@ -323,7 +323,7 @@ are no syntax errors may indicate that a function was declared but never called.
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target &target = GetSelectedOrDummyTarget(m_options.m_use_dummy);
+ Target &target = GetTarget(m_options.m_use_dummy);
const BreakpointList &breakpoints = target.GetBreakpointList();
size_t num_breakpoints = breakpoints.GetSize();
@@ -481,7 +481,7 @@ class CommandObjectBreakpointCommandDelete : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target &target = GetSelectedOrDummyTarget(m_options.m_use_dummy);
+ Target &target = GetTarget(m_options.m_use_dummy);
const BreakpointList &breakpoints = target.GetBreakpointList();
size_t num_breakpoints = breakpoints.GetSize();
@@ -548,7 +548,7 @@ class CommandObjectBreakpointCommandList : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target *target = &GetSelectedTarget();
+ Target *target = &GetTarget();
const BreakpointList &breakpoints = target->GetBreakpointList();
size_t num_breakpoints = breakpoints.GetSize();
diff --git a/lldb/source/Commands/CommandObjectDWIMPrint.cpp b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
index b7cd955e00203..35db422121a20 100644
--- a/lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -77,7 +77,7 @@ void CommandObjectDWIMPrint::DoExecute(StringRef command,
Target *target_ptr = m_exe_ctx.GetTargetPtr();
// Fallback to the dummy target, which can allow for expression evaluation.
- Target &target = target_ptr ? *target_ptr : GetDummyTarget();
+ Target &target = target_ptr ? *target_ptr : GetTarget(/*dummy=*/true);
EvaluateExpressionOptions eval_options =
m_expr_options.GetEvaluateExpressionOptions(target, m_varobj_options);
diff --git a/lldb/source/Commands/CommandObjectDisassemble.cpp b/lldb/source/Commands/CommandObjectDisassemble.cpp
index d975e39801317..8ec55cc120725 100644
--- a/lldb/source/Commands/CommandObjectDisassemble.cpp
+++ b/lldb/source/Commands/CommandObjectDisassemble.cpp
@@ -227,7 +227,7 @@ llvm::Error CommandObjectDisassemble::CheckRangeSize(const AddressRange &range,
return llvm::Error::success();
StreamString msg;
msg << "Not disassembling " << what << " because it is very large ";
- range.Dump(&msg, &GetSelectedTarget(), Address::DumpStyleLoadAddress,
+ range.Dump(&msg, &GetTarget(), Address::DumpStyleLoadAddress,
Address::DumpStyleFileAddress);
msg << ". To disassemble specify an instruction count limit, start/stop "
"addresses or use the --force option.";
@@ -252,7 +252,7 @@ CommandObjectDisassemble::GetContainingAddressRanges() {
}
};
- Target &target = GetSelectedTarget();
+ Target &target = GetTarget();
if (!target.GetSectionLoadList().IsEmpty()) {
Address symbol_containing_address;
if (target.GetSectionLoadList().ResolveLoadAddress(
@@ -351,8 +351,8 @@ CommandObjectDisassemble::GetNameRanges(CommandReturnObject &result) {
// Find functions matching the given name.
SymbolContextList sc_list;
- GetSelectedTarget().GetImages().FindFunctions(name, eFunctionNameTypeAuto,
- function_options, sc_list);
+ GetTarget().GetImages().FindFunctions(name, eFunctionNameTypeAuto,
+ function_options, sc_list);
std::vector<AddressRange> ranges;
llvm::Error range_errs = llvm::Error::success();
@@ -439,7 +439,7 @@ CommandObjectDisassemble::GetRangesForSelectedMode(
void CommandObjectDisassemble::DoExecute(Args &command,
CommandReturnObject &result) {
- Target *target = &GetSelectedTarget();
+ Target *target = &GetTarget();
if (!m_options.arch.IsValid())
m_options.arch = target->GetArchitecture();
diff --git a/lldb/source/Commands/CommandObjectExpression.cpp b/lldb/source/Commands/CommandObjectExpression.cpp
index eb76753d98efc..a91e6a08d9933 100644
--- a/lldb/source/Commands/CommandObjectExpression.cpp
+++ b/lldb/source/Commands/CommandObjectExpression.cpp
@@ -344,7 +344,7 @@ void CommandObjectExpression::HandleCompletion(CompletionRequest &request) {
return;
Target *exe_target = exe_ctx.GetTargetPtr();
- Target &target = exe_target ? *exe_target : GetDummyTarget();
+ Target &target = exe_target ? *exe_target : GetTarget(/*dummy=*/true);
unsigned cursor_pos = request.GetRawCursorPos();
// Get the full user input including the suffix. The suffix is necessary
@@ -407,7 +407,7 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
// that use an input reader...
ExecutionContext exe_ctx(m_interpreter.GetExecutionContext());
Target *exe_target = exe_ctx.GetTargetPtr();
- Target &target = exe_target ? *exe_target : GetDummyTarget();
+ Target &target = exe_target ? *exe_target : GetTarget(/*dummy=*/true);
lldb::ValueObjectSP result_valobj_sp;
StackFrame *frame = exe_ctx.GetFramePtr();
@@ -605,7 +605,7 @@ void CommandObjectExpression::DoExecute(llvm::StringRef command,
return;
if (m_repl_option.GetOptionValue().GetCurrentValue()) {
- Target &target = GetSelectedOrDummyTarget();
+ Target &target = GetTarget();
// Drop into REPL
m_expr_lines.clear();
m_expr_line_count = 0;
@@ -665,7 +665,7 @@ void CommandObjectExpression::DoExecute(llvm::StringRef command,
}
}
- Target &target = GetSelectedOrDummyTarget();
+ Target &target = GetTarget();
if (EvaluateExpression(expr, result.GetOutputStream(),
result.GetErrorStream(), result)) {
diff --git a/lldb/source/Commands/CommandObjectFrame.cpp b/lldb/source/Commands/CommandObjectFrame.cpp
index 3f4178c1a9595..29e460fe3885f 100644
--- a/lldb/source/Commands/CommandObjectFrame.cpp
+++ b/lldb/source/Commands/CommandObjectFrame.cpp
@@ -687,7 +687,7 @@ may even involve JITing and running code in the target program.)");
m_cmd_name);
// Increment statistics.
- TargetStats &target_stats = GetSelectedOrDummyTarget().GetStatistics();
+ TargetStats &target_stats = GetTarget().GetStatistics();
if (result.Succeeded())
target_stats.GetFrameVariableStats().NotifySuccess();
else
@@ -874,13 +874,13 @@ void CommandObjectFrameRecognizerAdd::DoExecute(Args &command,
RegularExpressionSP(new RegularExpression(m_options.m_module));
auto func =
RegularExpressionSP(new RegularExpression(m_options.m_symbols.front()));
- GetSelectedOrDummyTarget().GetFrameRecognizerManager().AddRecognizer(
+ GetTarget().GetFrameRecognizerManager().AddRecognizer(
recognizer_sp, module, func, m_options.m_first_instruction_only);
} else {
auto module = ConstString(m_options.m_module);
std::vector<ConstString> symbols(m_options.m_symbols.begin(),
m_options.m_symbols.end());
- GetSelectedOrDummyTarget().GetFrameRecognizerManager().AddRecognizer(
+ GetTarget().GetFrameRecognizerManager().AddRecognizer(
recognizer_sp, module, symbols, m_options.m_first_instruction_only);
}
#endif
@@ -898,9 +898,7 @@ class CommandObjectFrameRecognizerClear : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- GetSelectedOrDummyTarget()
- .GetFrameRecognizerManager()
- .RemoveAllRecognizers();
+ GetTarget().GetFrameRecognizerManager().RemoveAllRecognizers();
result.SetStatus(eReturnStatusSuccessFinishResult);
}
};
@@ -922,7 +920,7 @@ class CommandObjectFrameRecognizerDelete : public CommandObjectParsed {
if (request.GetCursorIndex() != 0)
return;
- GetSelectedOrDummyTarget().GetFrameRecognizerManager().ForEach(
+ GetTarget().GetFrameRecognizerManager().ForEach(
[&request](uint32_t rid, std::string rname, std::string module,
llvm::ArrayRef<lldb_private::ConstString> symbols,
bool regexp) {
@@ -953,9 +951,7 @@ class CommandObjectFrameRecognizerDelete : public CommandObjectParsed {
return;
}
- GetSelectedOrDummyTarget()
- .GetFrameRecognizerManager()
- .RemoveAllRecognizers();
+ GetTarget().GetFrameRecognizerManager().RemoveAllRecognizers();
result.SetStatus(eReturnStatusSuccessFinishResult);
return;
}
@@ -973,9 +969,8 @@ class CommandObjectFrameRecognizerDelete : public CommandObjectParsed {
return;
}
- if (!GetSelectedOrDummyTarget()
- .GetFrameRecognizerManager()
- .RemoveRecognizerWithID(recognizer_id)) {
+ if (!GetTarget().GetFrameRecognizerManager().RemoveRecognizerWithID(
+ recognizer_id)) {
result.AppendErrorWithFormat("'%s' is not a valid recognizer id.\n",
command.GetArgumentAtIndex(0));
return;
@@ -996,7 +991,7 @@ class CommandObjectFrameRecognizerList : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
bool any_printed = false;
- GetSelectedOrDummyTarget().GetFrameRecognizerManager().ForEach(
+ GetTarget().GetFrameRecognizerManager().ForEach(
[&result, &any_printed](
uint32_t recognizer_id, std::string name, std::string module,
llvm::ArrayRef<ConstString> symbols, bool regexp) {
@@ -1073,9 +1068,8 @@ class CommandObjectFrameRecognizerInfo : public CommandObjectParsed {
return;
}
- auto recognizer = GetSelectedOrDummyTarget()
- .GetFrameRecognizerManager()
- .GetRecognizerForFrame(frame_sp);
+ auto recognizer =
+ GetTarget().GetFrameRecognizerManager().GetRecognizerForFrame(frame_sp);
Stream &output_stream = result.GetOutputStream();
output_stream.Printf("frame %d ", frame_index);
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index e605abdb3c771..fe5c5041af2d1 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -1584,7 +1584,7 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
protected:
void DoExecute(Args &signal_args, CommandReturnObject &result) override {
- Target &target = GetSelectedOrDummyTarget();
+ Target &target = GetTarget();
// Any signals that are being set should be added to the Target's
// DummySignals so they will get applied on rerun, etc.
@@ -1662,7 +1662,7 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
if (m_options.do_clear) {
target.ClearDummySignals(signal_args);
if (m_options.dummy)
- GetDummyTarget().ClearDummySignals(signal_args);
+ GetTarget(/*dummy=*/true).ClearDummySignals(signal_args);
result.SetStatus(eReturnStatusSuccessFinishNoResult);
return;
}
diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index cc381a2ecee1d..3e077f00e59d2 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -1027,7 +1027,7 @@ class CommandObjectTargetModulesSearchPathsAdd : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target *target = &GetSelectedTarget();
+ Target *target = &GetTarget();
const size_t argc = command.GetArgumentCount();
if (argc & 1) {
result.AppendError("add requires an even number of arguments\n");
@@ -1074,7 +1074,7 @@ class CommandObjectTargetModulesSearchPathsClear : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target *target = &GetSelectedTarget();
+ Target *target = &GetTarget();
bool notify = true;
target->GetImageSearchPathList().Clear(notify);
result.SetStatus(eReturnStatusSuccessFinishNoResult);
@@ -1148,7 +1148,7 @@ class CommandObjectTargetModulesSearchPathsInsert : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target *target = &GetSelectedTarget();
+ Target *target = &GetTarget();
size_t argc = command.GetArgumentCount();
// check for at least 3 arguments and an odd number of parameters
if (argc >= 3 && argc & 1) {
@@ -1203,7 +1203,7 @@ class CommandObjectTargetModulesSearchPathsList : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
- Target *target = &GetSelectedTarget();
+ Target *target = &GetTarget();
target->GetImageSearchPathList().Dump(&result.GetOutputStream());
result.SetStatus(eReturnStatusSuccessFinishResult);
@@ -1226,7 +1226,7 @@ class CommandObjectTargetModulesSearchPathsQu...
[truncated]
|
bulbazord
reviewed
Jul 30, 2024
| // This is for use in the command interpreter and returns the most relevant | ||
| // target. In order of priority, that's the target from the command object's | ||
| // execution context, from the interpreter's execution context, the selected | ||
| // target or the dummy target. This function always return the dummy target if |
| } | ||
|
|
||
| Target &CommandObject::GetSelectedTarget() { | ||
| assert(m_flags.AnySet(eCommandRequiresTarget | eCommandProcessMustBePaused | |
There was a problem hiding this comment.
Do you think this assertion would still be useful to have?
There was a problem hiding this comment.
Not really, since we now fall back to the dummy target. The only place to put it would be in the last if-clause but at that point we know the target already exists so there's nothing to assert anymore.
jimingham
reviewed
Jul 30, 2024
| // This is for use in the command interpreter and returns the most relevant | ||
| // target. In order of priority, that's the target from the command object's | ||
| // execution context, from the interpreter's execution context, the selected | ||
| // target or the dummy target. This function always return the dummy target if |
There was a problem hiding this comment.
always returns
not
always return
jimingham
reviewed
Jul 30, 2024
| if (TargetSP target_sp = m_interpreter.GetDebugger().GetSelectedTarget()) | ||
| return *target_sp; | ||
|
|
||
| // We only have the dummy target. |
There was a problem hiding this comment.
I don't think it's worth using an optional here, but passing out a reference here means that if you called this with dummy == false, and there was only the dummy target, we would still return the Dummy target.
That seems unexpected - but maybe just worth a comment?
There was a problem hiding this comment.
The dummy argument is meant to indicate that we always want the dummy target. Would it be clearer if I called it force_dummy or dummy_only or something?
There was a problem hiding this comment.
Or prefer_dummy like the original signature? I shortened it to facilitate the inline comments, but I can change it back.
There was a problem hiding this comment.
Is it worth having that flag, since if you wanted ONLY the dummy target, you could still call GetDummyTarget.
There was a problem hiding this comment.
I kind of like that better, you shouldn't be dialing up the dummy target directly unless you really mean it, so making that stand out a bit more in the code might be good.
There was a problem hiding this comment.
I don't like prefer_dummy, "prefer" makes it sound like not returning the dummy is an option here, which it isn't...
There was a problem hiding this comment.
Alright, I removed the argument from GetTarget and added GetDummyTarget again.
There was a problem hiding this comment.
Yeah, that makes the case where people are dialing it up from an option slightly more verbose, but much clearer what's going on.
jimingham
approved these changes
Jul 30, 2024
bulbazord
approved these changes
Jul 30, 2024
labath
approved these changes
Jul 31, 2024
adrian-prantl
pushed a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Sep 19, 2024
Currently, CommandObjects are obtaining a target in a variety of ways. Often the command incorrectly operates on the selected target. As an example, when a breakpoint command is running, the current target is passed into the command but the target that hit the breakpoint is not the selected target. In other places we use the CommandObject's execution context, which is frozen during the execution of the command, and comes with its own limitations. Finally, we often want to fall back to the dummy target if no real target is available. Instead of having to guess how to get the target, this patch introduces one helper function in CommandObject to get the most relevant target. In order of priority, that's the target from the command object's execution context, from the interpreter's execution context, the selected target or the dummy target. rdar://110846511 (cherry picked from commit 8398ad9)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, CommandObjects are obtaining a target in a variety of ways. Often
the command incorrectly operates on the selected target. As an example, when a
breakpoint command is running, the current target is passed into the command
but the target that hit the breakpoint is not the selected target. In other
places we use the CommandObject's execution context, which is frozen during the
execution of the command, and comes with its own limitations. Finally, we often
want to fall back to the dummy target if no real target is available.
Instead of having to guess how to get the target, this patch introduces one
helper function in CommandObject to get the most relevant target. In order of
priority, that's the target from the command object's execution context, from
the interpreter's execution context, the selected target or the dummy target.
rdar://110846511