[Lldb-commits] [PATCH] D103588: [trace] Create a top level ThreadTrace class

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I mostly commented on ThreadTrace.h and we should resolve the questions in there before reviewing the rest of this patch.



================
Comment at: lldb/include/lldb/Target/ThreadTrace.h:44-45
+  /// \param[in] start_position
+  ///     The instruction position to start iterating on, where 0 corresponds to
+  ///     the oldest instruction chronologically.
+  ///
----------------
Should zero mean oldest when using TraceDirection::Forwards and newest when using TraceDirection::Backwards? Otherwise how would a person easily go backwards from the end of the thread trace?


================
Comment at: lldb/include/lldb/Target/ThreadTrace.h:61
+  virtual void TraverseInstructions(
+      size_t position, TraceDirection direction,
+      std::function<bool(size_t index,
----------------
Maybe name this just "Traverse" and add an additional parameter like:

```
enum class TraceUnit {
  Instruction, ///< Call the callback with the address each every instruction in a program.
  Branch, ///< Call the callback with the address of each branch that causes program flow to change.
  Return, ///< Call the callback only for branches that return from a function and removes a function from the thread's stack.
  Call, ///< Call the callback only for a function calls that add a function to the thread's stack.
};
```

Then this would make this traverse function very versatile as it could be used for making backtraces or stepping through the trace (step in, out, over etc). The enums could alternatively define bits in a bitfield that could be combined.


================
Comment at: lldb/include/lldb/Target/ThreadTrace.h:68
+  ///       The number of available instructions in the trace.
+  virtual size_t GetInstructionCount() = 0;
+
----------------
vsk wrote:
> `GetInstructionCount` requires decoding the whole ThreadTrace (if not for PT, then for other tracing technologies). I don't think we can take the O(n) hit up front.
This could be very expensive to calculate. Do we want to omit this to avoid having to traverse all of the data and also disassemble everything to find out instruction counts? Since our trace might have 1000 trace branches, but millions of instructions that are traversed between each branch


================
Comment at: lldb/include/lldb/Target/ThreadTrace.h:77
+  ///      The instruction type.
+  virtual lldb::TraceInstructionType GetInstructionType(size_t index) = 0;
+
----------------
vsk wrote:
> How does a a ThreadTrace client query what the valid range for indices is?
Why not just put this information in the callback? It seems like a storage issue or performance issue if you can only find the address out at callback time.


================
Comment at: lldb/include/lldb/Target/ThreadTrace.h:83
+  ///      The index of the instruction in question. It must be valid.
+  virtual size_t GetInstructionSize(size_t index) = 0;
+};
----------------
Can we put this info in the callback? Ditto from above comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588