This is an archive of the discontinued LLVM Phabricator instance.

[trace] Improve the TraceCursor iteration API
ClosedPublic

Authored by wallace on Jun 24 2022, 11:31 AM.

Details

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.

Diff Detail

Event Timeline

wallace created this revision.Jun 24 2022, 11:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2022, 11:31 AM
wallace requested review of this revision.Jun 24 2022, 11:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2022, 11:31 AM
jj10306 requested changes to this revision.Jun 24 2022, 3:19 PM

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
This revision now requires changes to proceed.Jun 24 2022, 3:19 PM
jj10306 accepted this revision.Jun 28 2022, 4:52 AM

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?

This revision is now accepted and ready to land.Jun 28 2022, 4:52 AM
wallace added inline comments.Jun 28 2022, 4:48 PM
lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
41

i think that's a very extreme case =P

44

i'll remove it