[Lldb-commits] [PATCH] D105531: [trace][intel pt] Implement the Intel PT cursor

[walter erquinigo via Phabricator via lldb-commits]

wallace marked 9 inline comments as done.
wallace added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2111
   bool m_create_repeat_command_just_invoked;
-  size_t m_consecutive_repetitions = 0;
+  std::map<const Thread *, std::unique_ptr<TraceInstructionDumper>> m_dumpers;
 };
----------------
clayborg wrote:
> "Thread *" isn't a stable thing to use, I would suggest using the "tid" of the thread. Any reason you are making a map of thread to unique pointers? Can't this just be:
> ```
> std::map<lldb::tid_t, TraceInstructionDumper> m_dumpers;
> ```
I would suggest using the "tid" of the thread -> makes sense

The cursor is a Unique pointer, which renders the TraceInstructionDumper not copyable.


================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:22
+TraceInstructionDumper::TraceInstructionDumper(lldb::TraceCursorUP &&cursor_up,
+                                               bool forwards, size_t skip,
+                                               bool raw)
----------------
clayborg wrote:
> Seems like the "forwards" and "skip" parameter shouldn't be in this class at all? Just setup the cursor before giving it to this class?
the direction is useful to knowing whether to call Next or Prev in the iterations

the skip is also useful to have inside, because that way it's easier to know if all the data has been consumed by the skip. That's stored in the m_no_more_data flag. When a trace has been fully consumed, it's still pointing to the last instruction, so another variable is needed to prevent more iterations


================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:35
+  if (m_forwards) {
+    if (!m_cursor_up->Next()) {
+      m_no_more_data = true;
----------------
clayborg wrote:
> Should we modify the TraceCursor API for Next(...) to be able to take a count? Seems like that might be much more efficient than recursively calling this function?
that's a pretty good idea


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105531