This revision was automatically updated to reflect the committed changes.
Closed by commit rGf91d82816ff5: [trace] Improve the TraceCursor iteration API
(authored by Walter Erquinigo <wall...@fb.com>).
Changed prior to commit:
https://reviews.llvm.org/D128543?vs=439827&id=440810#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128543/new/
https://reviews.llvm.org/D128543
Files:
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
Index: lldb/source/Target/TraceInstructionDumper.cpp
===================================================================
--- lldb/source/Target/TraceInstructionDumper.cpp
+++ lldb/source/Target/TraceInstructionDumper.cpp
@@ -261,33 +261,20 @@
: 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 @@
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 @@
// makes sense.
PrintEvents();
}
-
- if (!m_cursor_up->Next())
- SetNoMoreData();
}
+ if (!m_cursor_up->HasValue())
+ m_writer_up->InfoMessage("no more data");
return last_id;
}
Index: lldb/source/Target/TraceCursor.cpp
===================================================================
--- lldb/source/Target/TraceCursor.cpp
+++ lldb/source/Target/TraceCursor.cpp
@@ -21,15 +21,6 @@
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; }
Index: lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
===================================================================
--- lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
+++ lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
@@ -154,7 +154,8 @@
// 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 @@
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()) {
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
===================================================================
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
@@ -19,9 +19,11 @@
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 @@
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;
};
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===================================================================
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -20,69 +20,49 @@
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() {
Index: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
===================================================================
--- lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
+++ lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
@@ -225,10 +225,6 @@
}
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());
}
Index: lldb/source/Commands/CommandObjectThread.cpp
===================================================================
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -2285,16 +2285,17 @@
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;
}
Index: lldb/include/lldb/Target/TraceInstructionDumper.h
===================================================================
--- lldb/include/lldb/Target/TraceInstructionDumper.h
+++ lldb/include/lldb/Target/TraceInstructionDumper.h
@@ -108,14 +108,6 @@
/// 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 @@
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;
};
Index: lldb/include/lldb/Target/TraceCursor.h
===================================================================
--- lldb/include/lldb/Target/TraceCursor.h
+++ lldb/include/lldb/Target/TraceCursor.h
@@ -25,51 +25,66 @@
/// 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 @@
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 @@
/// \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 @@
/// 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 @@
///
/// \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 @@
/// \}
/// 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 @@
protected:
ExecutionContextRef m_exe_ctx_ref;
-
- lldb::TraceInstructionControlFlowType m_granularity =
- lldb::eTraceInstructionControlFlowTypeInstruction;
- bool m_ignore_errors = false;
bool m_forwards = false;
};
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
