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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Check my current comments before I proceed to look at the rest. Now that we are using the API for something, it is showing some issues with the usability of the API in TraceCursor. Let me know what you think



================
Comment at: lldb/include/lldb/Target/TraceCursor.h:85-105
   virtual bool Next(lldb::TraceInstructionControlFlowType granularity =
                         lldb::eTraceInstructionControlFlowTypeInstruction,
                     bool ignore_errors = false) = 0;
 
   /// Similar to \a TraceCursor::Next(), but moves backwards chronologically.
   virtual bool Prev(lldb::TraceInstructionControlFlowType granularity =
                         lldb::eTraceInstructionControlFlowTypeInstruction,
----------------
Since we haven't made the trace cursor API public yet, I wonder if this would be a better interface:

```
virtual void SetGranularity(lldb::TraceInstructionControlFlowType granularity);
virtual void SetIgnoreErrors(bool ignore_errors);
virtual void SetDirection(bool forward);
virtual size_t Move(size_t count);
```

Then we don't need Next(...) and Prev(...) now since the direction is set via an accessor. The Move(...) function would advance "count" times and it would obey the the granularity that is currently active. It also simplifies all of the arguments that used to be needed to be passed to the Next(...) and Prev(...) functions and gets rid of the default parameters.




================
Comment at: lldb/include/lldb/Target/TraceCursor.h:97
+  /// \param[in] count
+  ///     How many positions to move.
+  ///
----------------
clarify that this would move "count" times the granularity right?


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:107-113
   /// Force the cursor to point to the end of the trace, i.e. the most recent
   /// item.
   virtual void SeekToEnd() = 0;
 
   /// Force the cursor to point to the beginning of the trace, i.e. the oldest
   /// item.
   virtual void SeekToBegin() = 0;
----------------
Wondering if we want to replace these with more of a file like seek with a offset and whence?:
```
enum class SeekType { Set, Current, End };
bool Seek(int64_t offset, SeekType whence);
```
Then SeekToEnd() would become:
```
Seek(0, SeekType::End);
```
And SeekToBegin():
```
Seek(0, SeekType::Set);
```
And to skip some instructions:
```
Seek(10, SeekType::Current); // Advance in the direction by 10 granularity units
Seek(-10, SeekType::Current); // Reverse in the direction by 10 granularity units
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105531