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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
