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