clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
I think we should probably make a TraceDumpOptions class to contain all of the
dumping options and use that in the Target.h, Trace.h and all plug-ins. We will
no doubt expand the number of options in the future and add more. We current
have "tid" and "verbose". See inlined comments for details!
================
Comment at: lldb/include/lldb/Target/Target.h:1126-1127
+ /// \param[in] tid
+ /// If tid is not \a LLDB_INVALID_THREAD_ID, then only the trace
information
+ /// of the provided thread is printed.
+ ///
----------------
So we are going to want probably more options for this in the future. We
currently have "tid" and "verbose". We probably also need to specify the
process. If we add more options in the future, we would need to add more
arguments here. One way around this is to create a "TraceDumpOptions" structure
and pass it in:
```
struct TraceDumpOptions {
Stream *s;
Process *process; // If NULL, dump all processes
std::vector<lldb::tid_t> tids; // Thread IDs, if empty dump all threads
bool verbose;
};
void DumpTrace(const TraceDumpOptions &trace_options);
```
================
Comment at: lldb/include/lldb/Target/Target.h:1325-1330
+ /// When the private state thread calls SB API's - usually because it is
/// running OS plugin or Python ThreadPlan code - it should not block on the
/// API mutex that is held by the code that kicked off the sequence of events
- /// that led us to run the code. We hand out this mutex instead when we
+ /// that led us to run the code. We hand out this mutex instead when we
/// detect that code is running on the private state thread.
+ std::recursive_mutex m_private_mutex;
----------------
Remove whitespace only changes.
================
Comment at: lldb/include/lldb/Target/Trace.h:60-61
+ /// Flag that indicates to print verbose information.
+ virtual void Dump(lldb_private::Stream &s, lldb_private::Process *process,
+ lldb_private::Thread *thread, bool verbose) const = 0;
----------------
Maybe use "struct TraceDumpOptions" as mentioned before?
================
Comment at: lldb/source/Commands/CommandObjectTrace.cpp:155-160
+ case 't': {
+ if (option_arg.empty() || option_arg.getAsInteger(0, m_tid))
+ error.SetErrorStringWithFormat("invalid thread id string '%s'",
+ option_arg.str().c_str());
+ break;
+ }
----------------
Should we be able to specify this more than once?
```
(lldb) trace dump --tid 123 --tid 234
```
================
Comment at: lldb/source/Commands/CommandObjectTrace.cpp:176-177
bool m_verbose; // Enable verbose logging for debugging purposes.
+ tid_t m_tid;
};
----------------
replace with TraceDumpOptions struct:
```
TraceDumpOptions m_dump_options;
```
Then fill in this structure instead of m_verbose and m_tid.
================
Comment at: lldb/source/Commands/CommandObjectTrace.cpp:195-196
+ Target &target = GetSelectedOrDummyTarget();
+ target.DumpTrace(result.GetOutputStream(), m_options.m_tid,
+ m_options.m_verbose);
+
----------------
```
target.DumpTrace(m_options.m_dump_options);
```
Again this will allow us to add new dumping options easily in the future.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:21-22
public:
- void Dump(lldb_private::Stream *s) const override;
+ void Dump(lldb_private::Stream &s, lldb_private::Process *process,
+ lldb_private::Thread *thread, bool verbose) const override;
----------------
Use TraceDumpOptions
================
Comment at: lldb/source/Target/Target.cpp:2972
+
+void Target::DumpTrace(Stream &s, lldb::tid_t tid, bool verbose) {
+ if (!m_trace_sp) {
----------------
Use TraceDumpOptions. You will want to populate the "Process *process;" setting
from the target's process and anything else in the dump settings that isn't
filled in already that is target specific.
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
