[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 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