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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
