[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

[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/Target.h:1105
 
+  void SetTrace(const lldb::TraceSP &trace_sp);
+
----------------
wallace wrote:
> wallace wrote:
> > JDevlieghere wrote:
> > > Who owns the trace? If there's a 1:1 relationship between a trace and a target, can we make the target its owner? I'm trying to avoid adding shared pointers if possible. 
> > Well, there's a 1 to many relationship. Many targets can own the same trace object, as a single trace can have data of several processes.  On the other hand, there should no other object that could own a trace. I haven't found a better solution :(
> What do you think about changing GetTrace() to returns a Trace & instead of TraceSP &, so that no other object can share the ownership?
So this is an instance of a Trace plug-in for a given set of data described by the JSON. Since trace files can contain multiple processes, this will need to be shared between multiple Target instances if the "trace load" decides to load all processes, not just one. So I think this should remain a TraceSP so that an instance of the trace plug-in can be shared.


================
Comment at: lldb/include/lldb/Target/Target.h:1341
   unsigned m_next_persistent_variable_index = 0;
+  lldb::TraceSP m_trace;
   /// Stores the frame recognizers of this target.
----------------
wallace wrote:
> JDevlieghere wrote:
> > Doxygen comment?
> good catch
rename to "m_trace_sp" to indicate shared pointer here. See my previous comment for why we need a share pointer.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:97
 
-void TraceIntelPT::Dump(lldb_private::Stream *s) const {}
+void TraceIntelPT::Dump(lldb_private::Stream *s) const {
+  s->PutCString("Settings\n");
----------------
Since one Trace plug-in instance can contain trace data for one or more processes, would it make sense to have more arguments here? "bool verbose" which would enable dumping of settings? "lldb::pid_t pid" to dump the data for just one process? "lldb::tid_t tid" to dump the data for just one thread? 

Or we might want to add functions to lldb_private::Target that can dump the trace data? Like maybe:

```
void Target::DumpTrace(Stream &strm, lldb::tid_t tid = LLDB_INVALID_THREAD_ID) {
  if (!m_trace_sp)
    strm.PutCString("error: no trace data in target");
  ThreadSP thread_sp;
  if (tid != LLDB_INVALID_THREAD_ID)
    thread_sp = m_process_sp->FindThreadByID(tid);
  m_trace_sp->Dump(strm, m_process_sp.get(), thread_sp.get());
}
```

Then the Trace::Dump would look like:

```
void Trace::Dump(Stream &strm, Process *process, Thread *thread, bool verbose);
```
If there is no process, dump it all trace data for all processes and all threads in each process.
If there is a process and no thread, dump all threads from that process.
If there is a process and thread, dump just the data for that thread

The "trace dump" command can then compose calls using this API for any arguments might receive?

Dump all processes all threads:
```
(lldb) trace dump
```

Dump all threads for the specified processes:
```
(lldb) trace dump --pid 123  --pid 234
```

Dump all one thread from one process:
```
(lldb) trace dump --pid 123 --tid 345
```
If the --tid is specified, then we can have only one "--pid" argument.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:98-106
+  s->PutCString("Settings\n");
+  s->Indent();
+  m_settings.Dump(*s, /*pretty_print*/ true);
+  s->IndentLess();
+
+  s->PutCString("\nSettings directory\n");
+  s->Indent();
----------------
Settings should probably be only dumped if the "bool verbose" argument is true?


================
Comment at: lldb/source/Target/Target.cpp:2970
+
+lldb::TraceSP &Target::GetTrace() { return m_trace; }
+
----------------
Should we return a "Trace *" from this? If nullptr, then there is no trace data, else we have valid trace data? No one other than Target instances should be adding to the ref count. But I guess this is safer as if someone wants to use the returned Trace pointer, some other thread could free it and cause a crash. So maybe leaving as a TraceSP is a good idea...


================
Comment at: lldb/test/API/commands/trace/TestTraceLoad.py:70
+
+        self.expect("trace dump", substrs=["the current target does not have any traces"], error=True)
----------------
We should probably test --pid and --thread here if we decide the APIs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86670