[Lldb-commits] [lldb] f91d828 - [trace] Improve the TraceCursor iteration API
Walter Erquinigo via lldb-commits
lldb-commits at lists.llvm.org
Tue Jun 28 16:50:19 PDT 2022
Author: Walter Erquinigo
Date: 2022-06-28T16:50:12-07:00
New Revision: f91d82816ff5a88a59e86b54a4d63547776d4854
URL: https://github.com/llvm/llvm-project/commit/f91d82816ff5a88a59e86b54a4d63547776d4854
DIFF: https://github.com/llvm/llvm-project/commit/f91d82816ff5a88a59e86b54a4d63547776d4854.diff
LOG: [trace] Improve the TraceCursor iteration API
The current way ot traversing the cursor is a bit uncommon and it can't handle empty traces, in fact, its invariant is that it shold always point to a valid item. This diff simplifies the cursor API and allows it to point to invalid items, thus being able to handle empty traces or to know it ran out of data.
- Removed all the granularity functionalities, because we are not actually making use of that. We can bring them back when they are actually needed.
- change the looping logic to the following:
```
for (; cursor->HasValue(); cursor->Next()) {
if (cursor->IsError()) {
.. do something for error
continue;
}
.. do something for instruction
}
```
- added a HasValue method that can be used to identify if the cursor ran out of data, the trace is empty, or the user tried to move to an invalid position via SetId() or Seek()
- made several simplifications to severals parts of the code.
Differential Revision: https://reviews.llvm.org/D128543
Added:
Modified:
lldb/include/lldb/Target/TraceCursor.h
lldb/include/lldb/Target/TraceInstructionDumper.h
lldb/source/Commands/CommandObjectThread.cpp
lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
lldb/source/Target/TraceCursor.cpp
lldb/source/Target/TraceInstructionDumper.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/TraceCursor.h b/lldb/include/lldb/Target/TraceCursor.h
index 79c4cd371e4f4..7640fed93b99f 100644
--- a/lldb/include/lldb/Target/TraceCursor.h
+++ b/lldb/include/lldb/Target/TraceCursor.h
@@ -25,51 +25,66 @@ namespace lldb_private {
/// Live processes:
/// In the case of a live process trace, an instance of a \a TraceCursor should
/// point to the trace at the moment it was collected. If the process is later
-/// resumed and new trace data is collected, that should leave that old cursor
-/// unaffected.
+/// resumed and new trace data is collected, then it's up to each trace plug-in
+/// to decide whether to leave the old cursor unaffected or not.
///
/// Errors in the trace:
/// As there could be errors when reconstructing the instructions of a trace,
/// these errors are represented as failed instructions, and the cursor can
-/// point at them. The consumer should invoke \a TraceCursor::GetError() to
-/// check if the cursor is pointing to either a valid instruction or an error.
+/// point at them. The consumer should invoke \a TraceCursor::IsError() to
+/// check if the cursor is pointing to either a valid instruction or an error,
+/// and then \a TraceCursor::GetError() can return the actual error message.
///
/// Instructions:
/// A \a TraceCursor always points to a specific instruction or error in the
/// trace.
///
/// Defaults:
-/// By default, the cursor points at the end item of the trace, moves
-/// backwards, has a move granularity of \a
-/// eTraceInstructionControlFlowTypeInstruction (i.e. visit every instruction)
-/// and stops at every error (the "ignore errors" flag is \b false). See the
-/// \a TraceCursor::Next() method for more documentation.
+/// By default, the cursor points at the most recent item in the trace and is
+/// set up to iterate backwards. See the \a TraceCursor::Next() method for
+/// more documentation.
///
/// Sample usage:
///
/// TraceCursorUP cursor = trace.GetTrace(thread);
///
-/// cursor->SetGranularity(eTraceInstructionControlFlowTypeCall |
-/// eTraceInstructionControlFlowTypeReturn);
+/// for (; cursor->HasValue(); cursor->Next()) {
+/// if (cursor->IsError()) {
+/// cout << "error found at: " << cursor->GetError() << endl;
+/// continue;
+/// }
///
-/// do {
-/// if (llvm::Error error = cursor->GetError())
-/// cout << "error found at: " << llvm::toString(error) << endl;
-/// else if (cursor->GetInstructionControlFlowType() &
-/// eTraceInstructionControlFlowTypeCall)
-/// std::cout << "call found at " << cursor->GetLoadAddress() <<
-/// std::endl;
-/// else if (cursor->GetInstructionControlFlowType() &
-/// eTraceInstructionControlFlowTypeReturn)
-/// std::cout << "return found at " << cursor->GetLoadAddress() <<
-/// std::endl;
-/// } while(cursor->Next());
+/// switch (cursor->GetInstructionControlFlowType()) {
+/// eTraceInstructionControlFlowTypeCall:
+/// std::cout << "CALL found at " << cursor->GetLoadAddress() <<
+/// std::endl; break;
+/// eTraceInstructionControlFlowTypeReturn:
+/// std::cout << "RETURN found at " << cursor->GetLoadAddress() <<
+/// std::endl; break;
+/// }
+/// }
///
-/// Low level traversal:
-/// Unlike the \a TraceCursor::Next() API, which uses a given granularity and
-/// direction to advance the cursor, the \a TraceCursor::Seek() method can be
-/// used to reposition the cursor to an offset of the end, beginning, or
-/// current position of the trace.
+/// As the trace might be empty or the cursor might have reached the end of the
+/// trace, you should always invoke \a HasValue() to make sure you don't access
+/// invalid memory.
+///
+/// Random accesses:
+///
+/// The Trace Cursor offer random acesses in the trace via two APIs:
+///
+/// TraceCursor::Seek():
+/// Unlike the \a TraceCursor::Next() API, which moves instruction by
+/// instruction, the \a TraceCursor::Seek() method can be used to
+/// reposition the cursor to an offset of the end, beginning, or current
+/// position of the trace.
+///
+/// TraceCursor::GetId() / TraceCursor::SetId(id):
+/// Each item (error or instruction) in the trace has a numeric identifier
+/// which is defined by the trace plug-in. It's possible to access the id
+/// of the current item using GetId(), and to reposition the cursor to a
+/// given id using SetId(id).
+///
+/// You can read more in the documentation of these methods.
class TraceCursor {
public:
/// Helper enum to indicate the reference point when invoking
@@ -90,12 +105,6 @@ class TraceCursor {
virtual ~TraceCursor() = default;
- /// Set the granularity to use in the \a TraceCursor::Next() method.
- void SetGranularity(lldb::TraceInstructionControlFlowType granularity);
-
- /// Set the "ignore errors" flag to use in the \a TraceCursor::Next() method.
- void SetIgnoreErrors(bool ignore_errors);
-
/// Set the direction to use in the \a TraceCursor::Next() method.
///
/// \param[in] forwards
@@ -109,8 +118,7 @@ class TraceCursor {
/// \b true if the current direction is forwards, \b false if backwards.
bool IsForwards() const;
- /// Move the cursor to the next instruction that matches the current
- /// granularity.
+ /// Move the cursor to the next item (instruction or error).
///
/// Direction:
/// The traversal is done following the current direction of the trace. If
@@ -118,21 +126,12 @@ class TraceCursor {
/// chronologically. Otherwise, the traversal is done in
/// the opposite direction. By default, a cursor moves backwards unless
/// changed with \a TraceCursor::SetForwards().
- ///
- /// Granularity:
- /// The cursor will traverse the trace looking for the first instruction
- /// that matches the current granularity. If there aren't any matching
- /// instructions, the cursor won't move, to give the opportunity of
- /// changing granularities.
- ///
- /// Ignore errors:
- /// If the "ignore errors" flags is \b false, the traversal will stop as
- /// soon as it finds an error in the trace and the cursor will point at
- /// it.
- ///
+ virtual void Next() = 0;
+
/// \return
- /// \b true if the cursor effectively moved, \b false otherwise.
- virtual bool Next() = 0;
+ /// \b true if the cursor is pointing to a valid item. \b false if the
+ /// cursor has reached the end of the trace.
+ virtual bool HasValue() const = 0;
/// Instruction identifiers:
///
@@ -169,8 +168,8 @@ class TraceCursor {
///
/// \return
/// \b true if the given identifier exists and the cursor effectively
- /// moved. Otherwise, \b false is returned and the cursor doesn't change
- /// its position.
+ /// moved to it. Otherwise, \b false is returned and the cursor now points
+ /// to an invalid item, i.e. calling \a HasValue() will return \b false.
virtual bool GoToId(lldb::user_id_t id) = 0;
/// \return
@@ -185,25 +184,26 @@ class TraceCursor {
/// \}
/// Make the cursor point to an item in the trace based on an origin point and
- /// an offset. This API doesn't distinguishes instruction types nor errors in
- /// the trace, unlike the \a TraceCursor::Next() method.
+ /// an offset.
///
/// The resulting position of the trace is
/// origin + offset
///
- /// If this resulting position would be out of bounds, it will be adjusted to
- /// the last or first item in the trace correspondingly.
+ /// If this resulting position would be out of bounds, the trace then points
+ /// to an invalid item, i.e. calling \a HasValue() returns \b false.
///
/// \param[in] offset
/// How many items to move forwards (if positive) or backwards (if
- /// negative) from the given origin point.
+ /// negative) from the given origin point. For example, if origin is \b
+ /// End, then a negative offset would move backward in the trace, but a
+ /// positive offset would move past the trace to an invalid item.
///
/// \param[in] origin
/// The reference point to use when moving the cursor.
///
/// \return
- /// The number of trace items moved from the origin.
- virtual uint64_t Seek(int64_t offset, SeekType origin) = 0;
+ /// \b true if and only if the cursor ends up pointing to a valid item.
+ virtual bool Seek(int64_t offset, SeekType origin) = 0;
/// \return
/// The \a ExecutionContextRef of the backing thread from the creation time
@@ -260,10 +260,6 @@ class TraceCursor {
protected:
ExecutionContextRef m_exe_ctx_ref;
-
- lldb::TraceInstructionControlFlowType m_granularity =
- lldb::eTraceInstructionControlFlowTypeInstruction;
- bool m_ignore_errors = false;
bool m_forwards = false;
};
diff --git a/lldb/include/lldb/Target/TraceInstructionDumper.h b/lldb/include/lldb/Target/TraceInstructionDumper.h
index 5afff20317198..03aa8d6a6de84 100644
--- a/lldb/include/lldb/Target/TraceInstructionDumper.h
+++ b/lldb/include/lldb/Target/TraceInstructionDumper.h
@@ -108,14 +108,6 @@ class TraceInstructionDumper {
/// if no instructions were visited.
llvm::Optional<lldb::user_id_t> DumpInstructions(size_t count);
- /// \return
- /// \b true if there's still more data to traverse in the trace.
- bool HasMoreData();
-
- /// Indicate to the dumper that no more data is available in the trace.
- /// This will prevent further iterations.
- void SetNoMoreData();
-
private:
/// Create an instruction entry for the current position without symbol
/// information.
@@ -125,8 +117,6 @@ class TraceInstructionDumper {
lldb::TraceCursorUP m_cursor_up;
TraceInstructionDumperOptions m_options;
- /// If \b true, all the instructions have been traversed.
- bool m_no_more_data = false;
std::unique_ptr<OutputWriter> m_writer_up;
};
diff --git a/lldb/source/Commands/CommandObjectThread.cpp b/lldb/source/Commands/CommandObjectThread.cpp
index 037bbafdf8940..65512a051ab47 100644
--- a/lldb/source/Commands/CommandObjectThread.cpp
+++ b/lldb/source/Commands/CommandObjectThread.cpp
@@ -2285,16 +2285,17 @@ class CommandObjectTraceDumpInstructions : public CommandObjectParsed {
lldb::eFilePermissionsFileDefault);
}
+ if (m_options.m_continue && !m_last_id) {
+ // We need to stop processing data when we already ran out of instructions
+ // in a previous command. We can fake this by setting the cursor past the
+ // end of the trace.
+ cursor_up->Seek(1, TraceCursor::SeekType::End);
+ }
+
TraceInstructionDumper dumper(
std::move(cursor_up), out_file ? *out_file : result.GetOutputStream(),
m_options.m_dumper_options);
- if (m_options.m_continue && !m_last_id) {
- // We need to tell the dumper to stop processing data when
- // we already ran out of instructions in a previous command
- dumper.SetNoMoreData();
- }
-
m_last_id = dumper.DumpInstructions(m_options.m_count);
return true;
}
diff --git a/lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp b/lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
index 73d4f351fec41..e5d8e2d88490f 100644
--- a/lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
+++ b/lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
@@ -225,10 +225,6 @@ DecodedThread::DecodedThread(ThreadSP thread_sp, Error &&error)
}
lldb::TraceCursorUP DecodedThread::GetCursor() {
- // We insert a fake error signaling an empty trace if needed becasue the
- // TraceCursor requires non-empty traces.
- if (m_instruction_ips.empty())
- AppendError(createStringError(inconvertibleErrorCode(), "empty trace"));
return std::make_unique<TraceCursorIntelPT>(m_thread_sp, shared_from_this());
}
diff --git a/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp b/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
index 0e06905f027de..ebce5d950b15a 100644
--- a/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ b/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -20,69 +20,49 @@ using namespace llvm;
TraceCursorIntelPT::TraceCursorIntelPT(ThreadSP thread_sp,
DecodedThreadSP decoded_thread_sp)
: TraceCursor(thread_sp), m_decoded_thread_sp(decoded_thread_sp) {
- assert(m_decoded_thread_sp->GetInstructionsCount() > 0 &&
- "a trace should have at least one instruction or error");
- m_pos = m_decoded_thread_sp->GetInstructionsCount() - 1;
- m_tsc_range =
- m_decoded_thread_sp->CalculateTscRange(m_pos, /*hint_range*/ None);
+ Seek(0, SeekType::End);
}
-size_t TraceCursorIntelPT::GetInternalInstructionSize() {
+int64_t TraceCursorIntelPT::GetItemsCount() const {
return m_decoded_thread_sp->GetInstructionsCount();
}
-bool TraceCursorIntelPT::Next() {
- auto canMoveOne = [&]() {
- if (IsForwards())
- return m_pos + 1 < GetInternalInstructionSize();
- return m_pos > 0;
- };
+void TraceCursorIntelPT::CalculateTscRange() {
+ // If we failed, then we look for the exact range
+ if (!m_tsc_range || !m_tsc_range->InRange(m_pos))
+ m_tsc_range = m_decoded_thread_sp->CalculateTscRange(
+ m_pos, /*hit_range=*/m_tsc_range);
+}
- size_t initial_pos = m_pos;
+void TraceCursorIntelPT::Next() {
+ m_pos += IsForwards() ? 1 : -1;
- while (canMoveOne()) {
- m_pos += IsForwards() ? 1 : -1;
+ // We try to go to a neighbor tsc range that might contain the current pos
+ if (m_tsc_range && !m_tsc_range->InRange(m_pos))
+ m_tsc_range = IsForwards() ? m_tsc_range->Next() : m_tsc_range->Prev();
- if (m_tsc_range && !m_tsc_range->InRange(m_pos))
- m_tsc_range = IsForwards() ? m_tsc_range->Next() : m_tsc_range->Prev();
+ // If we failed, this call will fix it
+ CalculateTscRange();
+}
- if (!m_ignore_errors && IsError())
- return true;
- if (GetInstructionControlFlowType() & m_granularity)
- return true;
+bool TraceCursorIntelPT::Seek(int64_t offset, SeekType origin) {
+ switch (origin) {
+ case TraceCursor::SeekType::Beginning:
+ m_pos = offset;
+ break;
+ case TraceCursor::SeekType::End:
+ m_pos = GetItemsCount() - 1 + offset;
+ break;
+ case TraceCursor::SeekType::Current:
+ m_pos += offset;
}
+ CalculateTscRange();
- // Didn't find any matching instructions
- m_pos = initial_pos;
- return false;
+ return HasValue();
}
-uint64_t TraceCursorIntelPT::Seek(int64_t offset, SeekType origin) {
- int64_t last_index = GetInternalInstructionSize() - 1;
-
- auto fitPosToBounds = [&](int64_t raw_pos) -> int64_t {
- return std::min(std::max((int64_t)0, raw_pos), last_index);
- };
-
- auto FindDistanceAndSetPos = [&]() -> int64_t {
- switch (origin) {
- case TraceCursor::SeekType::Beginning:
- m_pos = fitPosToBounds(offset);
- return m_pos;
- case TraceCursor::SeekType::End:
- m_pos = fitPosToBounds(offset + last_index);
- return last_index - m_pos;
- case TraceCursor::SeekType::Current:
- int64_t new_pos = fitPosToBounds(offset + m_pos);
- int64_t dist = m_pos - new_pos;
- m_pos = new_pos;
- return std::abs(dist);
- }
- };
-
- int64_t dist = FindDistanceAndSetPos();
- m_tsc_range = m_decoded_thread_sp->CalculateTscRange(m_pos, m_tsc_range);
- return dist;
+bool TraceCursorIntelPT::HasValue() const {
+ return m_pos >= 0 && m_pos < GetItemsCount();
}
bool TraceCursorIntelPT::IsError() {
diff --git a/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h b/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
index c8a2db26b6eba..37fdb1f5fe8de 100644
--- a/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
+++ b/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
@@ -19,9 +19,11 @@ class TraceCursorIntelPT : public TraceCursor {
TraceCursorIntelPT(lldb::ThreadSP thread_sp,
DecodedThreadSP decoded_thread_sp);
- uint64_t Seek(int64_t offset, SeekType origin) override;
+ bool Seek(int64_t offset, SeekType origin) override;
- virtual bool Next() override;
+ void Next() override;
+
+ bool HasValue() const override;
const char *GetError() override;
@@ -43,12 +45,17 @@ class TraceCursorIntelPT : public TraceCursor {
bool HasId(lldb::user_id_t id) const override;
private:
- size_t GetInternalInstructionSize();
+ /// \return
+ /// The number of instructions and errors in the trace.
+ int64_t GetItemsCount() const;
+
+ /// Calculate the tsc range for the current position if needed.
+ void CalculateTscRange();
/// Storage of the actual instructions
DecodedThreadSP m_decoded_thread_sp;
/// Internal instruction index currently pointing at.
- size_t m_pos;
+ int64_t m_pos;
/// Tsc range covering the current instruction.
llvm::Optional<DecodedThread::TscRange> m_tsc_range;
};
diff --git a/lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp b/lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
index a927905464314..68c21462be4f4 100644
--- a/lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
+++ b/lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
@@ -154,7 +154,8 @@ TraceHTR::TraceHTR(Thread &thread, TraceCursor &cursor)
// TODO: Make distinction between errors by storing the error messages.
// Currently, all errors are treated the same.
m_instruction_layer_up->AppendInstruction(0);
- more_data_in_trace = cursor.Next();
+ cursor.Next();
+ more_data_in_trace = cursor.HasValue();
} else {
lldb::addr_t current_instruction_load_address = cursor.GetLoadAddress();
lldb::TraceInstructionControlFlowType current_instruction_type =
@@ -162,7 +163,8 @@ TraceHTR::TraceHTR(Thread &thread, TraceCursor &cursor)
m_instruction_layer_up->AppendInstruction(
current_instruction_load_address);
- more_data_in_trace = cursor.Next();
+ cursor.Next();
+ more_data_in_trace = cursor.HasValue();
if (current_instruction_type &
lldb::eTraceInstructionControlFlowTypeCall) {
if (more_data_in_trace && !cursor.IsError()) {
diff --git a/lldb/source/Target/TraceCursor.cpp b/lldb/source/Target/TraceCursor.cpp
index 23925748dd20b..485cb58cbb441 100644
--- a/lldb/source/Target/TraceCursor.cpp
+++ b/lldb/source/Target/TraceCursor.cpp
@@ -21,15 +21,6 @@ ExecutionContextRef &TraceCursor::GetExecutionContextRef() {
return m_exe_ctx_ref;
}
-void TraceCursor::SetGranularity(
- lldb::TraceInstructionControlFlowType granularity) {
- m_granularity = granularity;
-}
-
-void TraceCursor::SetIgnoreErrors(bool ignore_errors) {
- m_ignore_errors = ignore_errors;
-}
-
void TraceCursor::SetForwards(bool forwards) { m_forwards = forwards; }
bool TraceCursor::IsForwards() const { return m_forwards; }
diff --git a/lldb/source/Target/TraceInstructionDumper.cpp b/lldb/source/Target/TraceInstructionDumper.cpp
index 2da11eae7ca01..9e71686036c9e 100644
--- a/lldb/source/Target/TraceInstructionDumper.cpp
+++ b/lldb/source/Target/TraceInstructionDumper.cpp
@@ -261,33 +261,20 @@ TraceInstructionDumper::TraceInstructionDumper(
: m_cursor_up(std::move(cursor_up)), m_options(options),
m_writer_up(CreateWriter(s, m_options)) {
- if (m_options.id) {
- if (!m_cursor_up->GoToId(*m_options.id)) {
- m_writer_up->InfoMessage("invalid instruction id");
- SetNoMoreData();
- }
- } else if (m_options.forwards) {
+ if (m_options.id)
+ m_cursor_up->GoToId(*m_options.id);
+ else if (m_options.forwards)
m_cursor_up->Seek(0, TraceCursor::SeekType::Beginning);
- } else {
+ else
m_cursor_up->Seek(0, TraceCursor::SeekType::End);
- }
m_cursor_up->SetForwards(m_options.forwards);
if (m_options.skip) {
- uint64_t to_skip = *m_options.skip;
- if (m_cursor_up->Seek((m_options.forwards ? 1 : -1) * to_skip,
- TraceCursor::SeekType::Current) < to_skip) {
- // This happens when the skip value was more than the number of
- // available instructions.
- SetNoMoreData();
- }
+ m_cursor_up->Seek((m_options.forwards ? 1 : -1) * *m_options.skip,
+ TraceCursor::SeekType::Current);
}
}
-void TraceInstructionDumper::SetNoMoreData() { m_no_more_data = true; }
-
-bool TraceInstructionDumper::HasMoreData() { return !m_no_more_data; }
-
TraceInstructionDumper::InstructionEntry
TraceInstructionDumper::CreatRawInstructionEntry() {
InstructionEntry insn;
@@ -375,11 +362,8 @@ TraceInstructionDumper::DumpInstructions(size_t count) {
ExecutionContext exe_ctx;
thread_sp->GetProcess()->GetTarget().CalculateExecutionContext(exe_ctx);
- for (size_t i = 0; i < count; i++) {
- if (!HasMoreData()) {
- m_writer_up->InfoMessage("no more data");
- break;
- }
+ for (size_t i = 0; i < count && m_cursor_up->HasValue();
+ m_cursor_up->Next(), i++) {
last_id = m_cursor_up->GetId();
if (m_options.forwards) {
@@ -418,9 +402,8 @@ TraceInstructionDumper::DumpInstructions(size_t count) {
// makes sense.
PrintEvents();
}
-
- if (!m_cursor_up->Next())
- SetNoMoreData();
}
+ if (!m_cursor_up->HasValue())
+ m_writer_up->InfoMessage("no more data");
return last_id;
}
More information about the lldb-commits
mailing list