[Lldb-commits] [PATCH] D90729: [trace][intel-pt] Scaffold the 'thread trace start | stop' commands

[walter erquinigo via Phabricator via lldb-commits]

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