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