clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
So I am not sure I like the caching that we are doing in
TracePluginCommandDelegate in the CommandObjectTraceStart class. See inline
comments, but it might be better to cache the command object SP in
lldb_private::Process and add accessors.
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2013
+
+ if (process != m_delegate.process) {
+ m_delegate = {process, /*command*/ nullptr, /*error*/ ""};
----------------
See my comment below about TracePluginCommandDelegate.process being a pointer.
If we switch to using a weak pointer, then this should need to be:
```
if (m_delegate.process_wp.expired() || process !=
m_delegate.process_wp.lock().get())
```
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2014
+ if (process != m_delegate.process) {
+ m_delegate = {process, /*command*/ nullptr, /*error*/ ""};
+
----------------
if we use weak pointer:
```
m_delegate = {process->shared_from_this(), /*command*/ nullptr, /*error*/ ""};
```
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2034
+ struct TracePluginCommandDelegate {
+ Process *process;
+ CommandObjectSP command;
----------------
ProcessWP would be safer here. Theoretically a Process instance could be freed
and a new process could be allocated at the same address. So maybe:
```
ProcessWP process_wp;
```
The other option, which I think I prefer, would be to cache this in the
lldb_private::Process class instead of in a cache in CommandObjectTraceStart
================
Comment at: lldb/source/Commands/CommandObjectThreadUtil.cpp:1-2
+//===-- CommandObjectThreadUtil.cpp
+//-------------------------------------------===//
+//
----------------
Fix comment header to be on one line. This must be an auto lint thing?
================
Comment at: lldb/source/Commands/CommandObjectThreadUtil.h:1-2
+//===-- CommandObjectThreadUtil.h -----------------------------------*- C++
+//-*-===//
+//
----------------
fix comment header to be on one line
================
Comment at: lldb/test/API/commands/trace/TestTraceStartStop.py:24
+ self.expect("thread trace start", error=True,
+ substrs=["error: tracing is not supported for the current process.
Not implemented"])
+
----------------
Shouldn't we get a better error message here? Something like "error: can't
start tracing on a loaded trace file"? Might be nice to be able to somehow
detect that a process or thread comes from a core file. We would need to ask
the process something like "process->IsLiveDebugSession()" and return a correct
error if it isn't. If it is a live debug session, then the ""error: tracing is
not supported for the current process. Not implemented" is better.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90729/new/
https://reviews.llvm.org/D90729
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
