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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Still wondering if we need to do caching in TracePluginCommandDelegate. How many times do we fetch the command object if we type "thread trace start"? I don't know how much performance is truly needed here at the expense of maintaining the caching code.



================
Comment at: lldb/include/lldb/Target/Process.h:1395
 
+  virtual bool IsLiveDebugSession() const = 0;
+
----------------
This should return true as a default implementation. Then only the ProcessTrace, and the ProcessMachCore, ProcessMinidump, and ProcessELFCore would need to override. We could make a ProcessCore base class that overrides this by returning false and then having all of the mentioned core file classes inherit from ProcessCore. We also need header documentation for this.

Having a default implementation will help make sure the buildbots stay clean for Process plug-ins that are not always built, like ProcessWindows, which we missed in this diff.




================
Comment at: lldb/include/lldb/Target/ProcessTrace.h:1
 //===-- ProcessTrace.h ------------------------------------------*- C++ -*-===//
 //
----------------
Should ProcessTrace and ThreadTrace be moved to the Process plug-in directory?


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2039-2040
+    ProcessWP process_wp;
+    CommandObjectSP command;
+    std::string error;
+  } m_delegate = {};
----------------
Should we store the the "command" and "error" as:
```
llvm::Expected<CommandObjectSP> expected_command;
```
If so we need a destructor to consume the error, and a method to get the error as a string.



================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h:128
 
+  bool IsLiveDebugSession() const override { return true; }
+
----------------
we can get rid this and make "true" the default implementation. This will help make sure the buildbots stay clean for Process plug-ins that are not always built, like ProcessWindows.


================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.h:78
 
+  bool IsLiveDebugSession() const override { return false; }
+
----------------
It might be nice to create a Process subclass for core files and have ProcessMachCore, ProcessMinidump, ProcessTrace, and ProcessElfCore all inherit. They might have other functions we can refactor.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90729/new/

https://reviews.llvm.org/D90729