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

[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/TraceCursor.h:64-65
 
+  TraceCursor(lldb::ThreadSP thread_sp) : m_thread_sp(thread_sp) {}
+
   /// Move the cursor to the next instruction more recent chronologically in the
----------------
move this above destructor above.


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:105-116
+  /// \param[in] s
+  ///     The stream object where the instructions are printed.
+  ///
+  /// \param[in] count
+  ///     The number of instructions to print.
+  ///
+  /// \param[in] skip
----------------
Some of these are out of order (skip and count), or missing (direction). 


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:117
+  ///     information.
+  void DumpInstructions(Stream &s, lldb::TraceDirection direction, size_t skip,
+                        size_t count, bool raw);
----------------
Should we just use a bool for direction? There can't be any direction other than forward or backward.


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:117-118
+  ///     information.
+  void DumpInstructions(Stream &s, lldb::TraceDirection direction, size_t skip,
+                        size_t count, bool raw);
 
----------------
clayborg wrote:
> Should we just use a bool for direction? There can't be any direction other than forward or backward.
Remove the "size_t skip" from this API. It isn't needed because the user can move the cursor around before they call this API to skip or position the cursor as needed, then call this function.


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:144-151
+  /// Get a number indicating the position of the current instruction in the
+  /// trace. This number can be negative if the trace is being traversed
+  /// backwards and the total number of instructions is not known.
+  ///
+  /// The objective of this number is to help distinguish consecutive
+  /// instructions for presentation purposes, and not for using it as an
+  /// accessor.
----------------
This description is pretty vague. It should probably be removed or fully documented to state exactly what is expected of trace clients. This also seems like something that might be returned when moving the cursor around instead of a separate method? From the description, I am unclear as to what I should expect from traversing backwards. What happens when I call SeekToEnd(), or SeekToBegin()? What happens if I call Next(...) or Prev(...) when the granularity is set in a specific way?




================
Comment at: lldb/include/lldb/Target/TraceCursor.h:160
+  /// The thread that owns this cursor.
+  lldb::ThreadSP m_thread_sp;
 };
----------------
Probably best to store this as a: ExecutionContextRef m_exe_ref. See description of ExecutionContextRef in ExecutionContext.h for reasons why. The main issue is we don't want a TraceCursor to keep a strong reference to a thread and keep the thread and possibly the process object alive. Keeping a weak reference is safer and allows you to try to fetch the thread when needed by calling ExecutionContextRef::GetThreadSP()


================
Comment at: lldb/include/lldb/lldb-enumerations.h:962-966
+/// Enum for the direction to use when traversing instructions.
+enum TraceDirection {
+  eTraceDirectionForwards = 0,
+  eTraceDirectionBackwards,
+};
----------------
Unless there is another direction, I would just switch this to a bool and remove this from the public API. Fine to use it internally if needed.


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2101-2105
+    cursor.DumpInstructions(result.GetOutputStream(),
+                            m_options.m_forwards ? eTraceDirectionForwards
+                                                 : eTraceDirectionBackwards,
+                            IsRepeatCommand() ? 0 : m_options.m_skip,
+                            m_options.m_count, m_options.m_raw);
----------------
take care of skip on the cursor _before_ calling this API, and remove skip argument.


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2102-2103
+    cursor.DumpInstructions(result.GetOutputStream(),
+                            m_options.m_forwards ? eTraceDirectionForwards
+                                                 : eTraceDirectionBackwards,
+                            IsRepeatCommand() ? 0 : m_options.m_skip,
----------------
just pass a bool? Enum seems a bit much for a simple bool argument. Unless there is going to be another direction in the future, but I can't think of one.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:121
 /// stopped at. See \a Trace::GetCursorPosition for more information.
-class DecodedThread {
+class DecodedThread : public std::enable_shared_from_this<DecodedThread> {
 public:
----------------
You only need to inherit from "std::enable_shared_from_this" if you ever need to take a "DecodedThread *" and get ahold of the original std::shared_ptr<DecodedThread>. Is that the case here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105531