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

[Greg Clayton via Phabricator via lldb-commits]

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