[trace] Improve the TraceCursor iteration API

[wallace]

Summary

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.

[jj10306]

will take a complete look over the weekend, but wanted to point out the conflict with @persona0220's diff asap

lldb/include/lldb/Target/TraceCursor.h

93–98

@persona0220 did this in https://reviews.llvm.org/D128477 ?

[jj10306]

lgtm, thanks for making the cursor traversal much cleaner

lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp

228–231

nice, the new approach is much cleaner

lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp

41

should only do this increment or decrement if HasValue() is true? otherwise (in theory) this value could wrap around if it's incremented/decremented too many times?

44

why is this new scope introduced here?

[wallace]

lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp

41

i think that's a very extreme case =P

44

i'll remove it

lldb/include/lldb/Target/TraceCursor.h

 /// This class attempts to be a generic interface for accessing the instructions
 /// of the trace so that each Trace plug-in can reconstruct, represent and store
 /// the instruction data in an flexible way that is efficient for the given
 /// technology.
 ///
 /// 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;
+///     }
+///
+///     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;
+///     }
+///  }
+///
+///  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).
 ///
-///  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());
-///
-/// 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.
+///   You can read more in the documentation of these methods.
 class TraceCursor {
 public:
   /// Helper enum to indicate the reference point when invoking
   /// \a TraceCursor::Seek().
   /// The following values are inspired by \a std::istream::seekg.
   enum class SeekType {
     /// The beginning of the trace, i.e the oldest item.
     Beginning = 0,
     /// The current position in the trace.
     Current,
     /// The end of the trace, i.e the most recent item.
     End
   };
 
   /// Create a cursor that initially points to the end of the trace, i.e. the
   /// most recent item.
   TraceCursor(lldb::ThreadSP thread_sp);
 
   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
   ///     If \b true, then the traversal will be forwards, otherwise backwards.
   void SetForwards(bool forwards);
 
   /// Check if the direction to use in the \a TraceCursor::Next() method is
   /// forwards.
   ///
   /// \return
   ///     \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
   ///     it is forwards, the instructions are visited forwards
   ///     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:
   ///
   /// When building complex higher level tools, fast random accesses in the
   /// trace might be needed, for which each instruction requires a unique
   /// identifier within its thread trace. For example, a tool might want to
   /// repeatedly inspect random consecutive portions of a trace. This means that
   /// it will need to first move quickly to the beginning of each section and
public:
   ///   way to traverse sequential instructions is to use the \a
   ///   TraceCursor::Next() method and only use \a TraceCursor::GoToId(id)
   ///   sparingly.
 
   /// Make the cursor point to the item whose identifier is \p id.
   ///
   /// \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
   ///     \b true if and only if there's an instruction item with the given \p
   ///     id.
   virtual bool HasId(lldb::user_id_t id) const = 0;
 
   /// \return
   ///     A unique identifier for the instruction or error this cursor is
   ///     pointing to.
   virtual lldb::user_id_t GetId() const = 0;
   /// \}
 
   /// 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
   ///   of this cursor.
   ExecutionContextRef &GetExecutionContextRef();
 
   /// Instruction or error information
   /// \{
public:
   ///     instruction the cursor is pointing at falls into. If the cursor points
   ///     to an error in the trace, return \b 0.
   virtual lldb::TraceInstructionControlFlowType
   GetInstructionControlFlowType() = 0;
   /// \}
 
 protected:
   ExecutionContextRef m_exe_ctx_ref;
-
-  lldb::TraceInstructionControlFlowType m_granularity =
-      lldb::eTraceInstructionControlFlowTypeInstruction;
-  bool m_ignore_errors = false;
   bool m_forwards = false;
 };
 
 namespace trace_event_utils {
 /// Convert an individual event to a display string.
 ///
 /// \param[in] event
 ///     An individual event.

lldb/include/lldb/Target/TraceInstructionDumper.h

public:
   /// \param[in] count
   ///     The number of instructions to print.
   ///
   /// \return
   ///     The instruction id of the last traversed instruction, or \b llvm::None
   ///     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.
   InstructionEntry CreatRawInstructionEntry();
 
   void PrintEvents();
 
   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;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_TRACE_INSTRUCTION_DUMPER_H

lldb/source/Commands/CommandObjectThread.cpp

bool DoExecute(Args &args, CommandReturnObject &result) override {
 
     llvm::Optional<StreamFile> out_file;
     if (m_options.m_output_file) {
       out_file.emplace(m_options.m_output_file->GetPath().c_str(),
                        File::eOpenOptionWriteOnly | File::eOpenOptionCanCreate,
                        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;
   }
 
   CommandOptions m_options;
   // Last traversed id used to continue a repeat command. None means
   // that all the trace has been consumed.
   llvm::Optional<lldb::user_id_t> m_last_id;

lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp

 DecodedThread::DecodedThread(ThreadSP thread_sp) : m_thread_sp(thread_sp) {}
 
 DecodedThread::DecodedThread(ThreadSP thread_sp, Error &&error)
     : m_thread_sp(thread_sp) {
   AppendError(std::move(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());
 }
 
 size_t DecodedThread::CalculateApproximateMemoryUsage() const {
   return sizeof(pt_insn::ip) * m_instruction_ips.size() +
          sizeof(pt_insn::size) * m_instruction_sizes.size() +
          sizeof(pt_insn::iclass) * m_instruction_classes.size() +
          (sizeof(size_t) + sizeof(uint64_t)) * m_instruction_timestamps.size() +

lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h

 namespace lldb_private {
 namespace trace_intel_pt {
 
 class TraceCursorIntelPT : public TraceCursor {
 public:
   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;
 
   lldb::addr_t GetLoadAddress() override;
 
   llvm::Optional<uint64_t> GetCounter(lldb::TraceCounter counter_type) override;
 
   lldb::TraceEvents GetEvents() override;
 
   lldb::TraceInstructionControlFlowType
   GetInstructionControlFlowType() override;
 
   bool IsError() override;
 
   bool GoToId(lldb::user_id_t id) override;
 
   lldb::user_id_t GetId() const override;
 
   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;
 };
 
 } // namespace trace_intel_pt
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_TRACECURSORINTELPT_H

lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp

 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::trace_intel_pt;
 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;
-  };
-
-  size_t initial_pos = m_pos;
+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);
+}
 
-  while (canMoveOne()) {
-    m_pos += IsForwards() ? 1 : -1;
+void TraceCursorIntelPT::Next() {
 
-    if (m_tsc_range && !m_tsc_range->InRange(m_pos))
-      m_tsc_range = IsForwards() ? m_tsc_range->Next() : m_tsc_range->Prev();
+  // We try to go to a neighbor tsc range that might contain the current pos
 
-    if (!m_ignore_errors && IsError())
-      return true;
-    if (GetInstructionControlFlowType() & m_granularity)
-      return true;
-  }
-
-  // Didn't find any matching instructions
-  m_pos = initial_pos;
-  return false;
+  // If we failed, this call will fix it
+  CalculateTscRange();
 }
 
-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);
-    }
-  };
+bool TraceCursorIntelPT::Seek(int64_t offset, SeekType origin) {
+    m_pos = offset;
+    break;
+    m_pos = GetItemsCount() - 1 + offset;
+    break;
+    m_pos += offset;
+  CalculateTscRange();
 
-  int64_t dist = FindDistanceAndSetPos();
-  m_tsc_range = m_decoded_thread_sp->CalculateTscRange(m_pos, m_tsc_range);
-  return dist;
+  return HasValue();
+}
+
+bool TraceCursorIntelPT::HasValue() const {
+  return m_pos >= 0 && m_pos < GetItemsCount();
 }
 
 bool TraceCursorIntelPT::IsError() {
   return m_decoded_thread_sp->IsInstructionAnError(m_pos);
 }
 
 const char *TraceCursorIntelPT::GetError() {
   return m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);

lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp

TraceHTR::TraceHTR(Thread &thread, TraceCursor &cursor)
   bool more_data_in_trace = true;
   while (more_data_in_trace) {
     if (cursor.IsError()) {
       // Append a load address of 0 for all instructions that an error occured
       // while decoding.
       // 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 =
           cursor.GetInstructionControlFlowType();
 
       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()) {
           m_instruction_layer_up->AddCallInstructionMetadata(
               current_instruction_load_address,
               function_name_from_load_address(cursor.GetLoadAddress()));
         } else {
           // Next instruction is not known - pass None to indicate the name

lldb/source/Target/TraceCursor.cpp

 
 TraceCursor::TraceCursor(lldb::ThreadSP thread_sp)
     : m_exe_ctx_ref(ExecutionContext(thread_sp)) {}
 
 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; }
 
 const char *trace_event_utils::EventToDisplayString(lldb::TraceEvents event) {
   switch (event) {
   case lldb::eTraceEventPaused:
     return "paused";

lldb/source/Target/TraceInstructionDumper.cpp

 }
 
 TraceInstructionDumper::TraceInstructionDumper(
     lldb::TraceCursorUP &&cursor_up, Stream &s,
     const TraceInstructionDumperOptions &options)
     : 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;
   insn.id = m_cursor_up->GetId();
 
   if (m_options.show_tsc)
     insn.tsc = m_cursor_up->GetCounter(lldb::eTraceCounterTSC);
   return insn;
m_writer_up->InfoMessage(formatv("thread #{0}: tid = {1}",
                                .str());
 
   SymbolInfo prev_symbol_info;
   Optional<lldb::user_id_t> last_id;
 
   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) {
       // When moving forwards, we first print the event before printing
       // the actual instruction.
       PrintEvents();
     }
 
for (size_t i = 0; i < count && m_cursor_up->HasValue();
     }
 
     if (!m_options.forwards) {
       // If we move backwards, we print the events after printing
       // the actual instruction so that reading chronologically
       // makes sense.
       PrintEvents();
     }
-
-    if (!m_cursor_up->Next())
-      SetNoMoreData();
   }
+  if (!m_cursor_up->HasValue())
+    m_writer_up->InfoMessage("no more data");
   return last_id;
 }