clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.
================
Comment at: lldb/include/lldb/Target/Trace.h:183
+ /// llvm::Error::success() otherwise.
+ virtual llvm::Error StopTracingThread(const Thread &thread) {
+ return llvm::make_error<UnimplementedError>();
----------------
We need to have StartTracingThread() in this interface as well. I know right
now we can just do the work in the command plug-in, but eventually we will need
to expose the trace stuff through the SB API, so we need the StartTracingThread
in the API here.
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2126
+/// can support the current process.
+class CommandObjectTraceStart : public CommandObjectParsed {
+public:
----------------
We should probably create a CommandObjectDelegate in CommandObjectDelegate.cpp
and CommandObjectDelegate.h. This class will have one pure virtual function
named GetDelegateCommand() which we would override in this class and return
GetTracePluginCommand(). Then this class will become much simpler and the next
people that will create a similar kind of command that just forwards to another
command can use it.
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2139
+
+ Options *GetOptions() override {
+ if (CommandObject *trace_plugin_command =
----------------
Move to CommandObjectDelegate.cpp
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2140-2141
+ Options *GetOptions() override {
+ if (CommandObject *trace_plugin_command =
+ GetTracePluginCommand().command.get())
+ return trace_plugin_command->GetOptions();
----------------
Store as a shared pointer here please and use it:
```
CommandObjectSP delegate_cmd_sp = GetDelegateCommand();
if (delegate_cmd_sp)
delegate_cmd_sp->GetOptions();
```
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2146
+
+ llvm::StringRef GetSyntax() override {
+ if (CommandObject *trace_plugin_command =
----------------
Move to CommandObjectDelegate.cpp
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2147-2148
+ llvm::StringRef GetSyntax() override {
+ if (CommandObject *trace_plugin_command =
+ GetTracePluginCommand().command.get())
+ return trace_plugin_command->GetSyntax();
----------------
Store as a shared pointer like above comment.
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2153
+
+ llvm::StringRef GetHelp() override {
+ if (CommandObject *trace_plugin_command =
----------------
Move to CommandObjectDelegate.cpp
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2154-2155
+ llvm::StringRef GetHelp() override {
+ if (CommandObject *trace_plugin_command =
+ GetTracePluginCommand().command.get())
+ return trace_plugin_command->GetHelp();
----------------
Store as a shared pointer like above comment.
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2160
+
+ llvm::StringRef GetHelpLong() override {
+ if (CommandObject *trace_plugin_command =
----------------
Move to CommandObjectDelegate.cpp
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2161-2162
+ llvm::StringRef GetHelpLong() override {
+ if (CommandObject *trace_plugin_command =
+ GetTracePluginCommand().command.get())
+ return trace_plugin_command->GetHelpLong();
----------------
Store as a shared pointer like above comment.
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2170
+
+ bool Execute(const char *args_string, CommandReturnObject &result) override {
+ TracePluginCommandImplementation &trace_plugin_command =
----------------
Why are you taking over execute instead of DoExecute?
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2176-2187
+ if (const TraceSP &trace_sp =
+ trace_plugin_command.process->GetTarget().GetTrace()) {
+ ConstString plugin_name = trace_sp->GetPluginName();
+ if (plugin_name.GetStringRef() != trace_plugin_command.plugin_name) {
+ result.AppendErrorWithFormat(
+ "error: attempted to trace with the '%s' plug-in, but this process
"
+ "is already being traced with the '%s' plug-in. Stop tracing "
----------------
If we require that only one tracing mechanism is available in a process, then
why do we need this check?
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2202
+ /// the trace plug-in that supports the current process.
+ TracePluginCommandImplementation &GetTracePluginCommand() {
+ ExecutionContext exe_ctx = m_interpreter.GetExecutionContext();
----------------
make virtual override of CommandObjectDelegate::GetDelegateCommand()
================
Comment at: lldb/source/Commands/CommandObjectThread.h:16
+class CommandObjectIterateOverThreads : public CommandObjectParsed {
+
----------------
This should go in a file on its own near the CommandObject.cpp file. Maybe in
CommandObjectThreadUtil.h/.cpp?
================
Comment at: lldb/source/Interpreter/CommandObject.cpp:263-266
+ if (!m_exe_ctx.GetProcessPtr()) {
+ result.SetError("Process must exist.");
+ return false;
+ }
----------------
The command should specify it needs a process with eCommandRequiresProcess.
Remove these lines.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90729/new/
https://reviews.llvm.org/D90729
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
