wallace added inline comments.
================
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>();
----------------
clayborg wrote:
> 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.
l'll add it later when I implement the actual StartTracingThread logic. I want
to make sure I can cleanly allow each Trace plug-in to have its own set of
tracing options for the StartTracingThread, and I'd rather do that in another
diff.
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2126
+/// can support the current process.
+class CommandObjectTraceStart : public CommandObjectParsed {
+public:
----------------
clayborg wrote:
> 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.
Great idea!
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2170
+
+ bool Execute(const char *args_string, CommandReturnObject &result) override {
+ TracePluginCommandImplementation &trace_plugin_command =
----------------
clayborg wrote:
> Why are you taking over execute instead of DoExecute?
because Execute in the delegate configures the ExecutionContext correctly, so I
need to invoke Execute in the Trace plug-in command. With the Delegate idea
that you mention, this will look cleaner
================
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 "
----------------
clayborg wrote:
> If we require that only one tracing mechanism is available in a process, then
> why do we need this check?
that's right!
================
Comment at: lldb/source/Interpreter/CommandObject.cpp:263-266
+ if (!m_exe_ctx.GetProcessPtr()) {
+ result.SetError("Process must exist.");
+ return false;
+ }
----------------
clayborg wrote:
> The command should specify it needs a process with eCommandRequiresProcess.
> Remove these lines.
sure
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
