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