[Lldb-commits] [PATCH] D104422: [trace] Add a TraceCursor class

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/Target/Trace.h:224
+  ///     failed to load, an \a llvm::Error is returned.
+  virtual llvm::Expected<lldb::TraceCursorSP> GetThreadEndCursor(
+      Thread &thread,
----------------
If the trace cursor contains an error, do we need llvm::Expected here? Maybe return a std::unique_ptr<TraceCursor> since the cursor can indicate any errors. I am assuming you did a shared pointer to make it easier to eventually push this into the lldb::SB API? a unique pointer works great for the SBAPI and no one will ever need a shared reference to one of these.


================
Comment at: lldb/include/lldb/Target/Trace.h:226-227
+      Thread &thread,
+      lldb::TraceInstructionType filter = lldb::eTraceInstructionTypeAny,
+      bool ignore_errors = false) = 0;
+
----------------
We don' t need these arguments right? These are only needed for the TraceCursor::Next(...) and TraceCursor::Prev(...) right?


================
Comment at: lldb/include/lldb/Target/Trace.h:231
+  /// instruction instead.
+  virtual llvm::Expected<lldb::TraceCursorSP> GetThreadBeginCursor(
+      Thread &thread,
----------------
Ditto, return std::unique_ptr<TraceCursor>?


================
Comment at: lldb/include/lldb/Target/Trace.h:233-234
+      Thread &thread,
+      lldb::TraceInstructionType filter = lldb::eTraceInstructionTypeAny,
+      bool ignore_errors = false) = 0;
+
----------------
We don' t need these arguments right? These are only needed for the TraceCursor::Next(...) and TraceCursor::Prev(...) right?


================
Comment at: lldb/include/lldb/Target/Trace.h:235
+      bool ignore_errors = false) = 0;
+
   /// Get the number of available instructions in the trace of the given thread.
----------------
Should there just be a single Trace::GetCursor() and then we can just add some more TraceInstructionType bits? My thinking is the cursor would always return an end cursor and we could do something like:
```
std::unique_ptr<TraceCursor> end_cursor_up = trace-GetCursor(thread);
std::unique_ptr<TraceCursor> begin_cursor_up = trace-GetCursor(thread);
begin_cursor_up->Prev(eTraceInstructionTypeBegin);
```




================
Comment at: lldb/include/lldb/Target/TraceCursor.h:39
+///
+class TraceCursor {
+public:
----------------
I am assuming all function calls need to be pure virtual here so that each plug-in can subclass this?


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:61
+  /// Similar to \a TraceCursor::Next(), but moves backwards chronologically.
+  bool Prev();
+
----------------
Do we not need the filter and ignore errors arguments for this as well?


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:72-75
+  /// \return
+  ///     \b true if the cursor is pointing to an error in the trace. The actual
+  ///     error message can be gotten by calling \a TraceCursor::GetError().
+  bool IsError();
----------------
Do we need this if we have GetError?


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:79
+  ///     \b nullptr if the cursor is not pointing to an error in the trace.
+  ///     Otherwise return an error message describing the issue.
+  const char *GetError();
----------------
What is the intended ownership of this string? If this object would own it, maybe add a "std::string m_error;" member variable to this class to store any error strings.


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:88
+  /// \return
+  ///     The size in bytes of the instruction. If the cursor points to an error
+  ///     in the trace, return \b 0.
----------------
describe what the size means a bit better? Is the size of instructions going to be larger if we go to the next Jump or call?


================
Comment at: lldb/include/lldb/lldb-enumerations.h:963
+/// analysis on traces.
+FLAGS_ENUM(TraceInstructionType){
+    /// Any instruction not listed below.
----------------
Maybe "TraceCursorGranularity"? the "Type" suffix seems to indicate individual types


================
Comment at: lldb/include/lldb/lldb-enumerations.h:964-965
+FLAGS_ENUM(TraceInstructionType){
+    /// Any instruction not listed below.
+    eTraceInstructionTypeNormal = (1u << 1),
+    /// This includes both conditional and unconditional jumps.
----------------



================
Comment at: lldb/include/lldb/lldb-enumerations.h:966-967
+    eTraceInstructionTypeNormal = (1u << 1),
+    /// This includes both conditional and unconditional jumps.
+    eTraceInstructionTypeJump = (1u << 2),
+    /// This represents a call to a function.
----------------
Maybe have two things for branches? Something like:
```
/// Any branch instruction even if the branch is conditional and not taken.
eTraceInstructionTypeBranch,
/// Only branches that are taken and cause the control flow to change.
eTraceInstructionTypeBranchTaken,
```
This might help avoid multiple calls to the cursor if the user doesn't need to worry about branches that aren't taken (like when single stepping maybe?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104422