[XRay] Update FDR log reader to be aware of buffer sizes per thread.

[kpw]

Summary

It is problematic for this reader that it expects to read data from
several threads, but the header or message format does not define
framing. Since the buffers are reused, we can't rely on skipping
zeroed out data as a synchronization method either.

There is an argument that this is not version compatible with the format
the reader expected previously. I argue that since the writer wrote garbage
past the end of buffer record, there is no currently working reader to
compromise.

The corresponding writer change is posted to D31384.

[dberris]

LGTM with some nits.

lib/XRay/Trace.cpp

138 ↗

(On Diff #93104)

Just curious -- did you need to actually explicitly create an instance of Twine? Or would just returning the literal work just fine?

385–386 ↗

(On Diff #93104)

I don't think you need the else since you're already continuing in the body of the true branch.

423–426 ↗

(On Diff #93104)

I suspect it's more readable if you just used '+' here. Idiomatic Twine usage seems to prefer concatenation with '+' for readability reasons.

[kpw]

This now expects the buffer size information in the Header rather than in a separate
piece of data.

[dberris]

LGTM - Few more nits.

lib/XRay/Trace.cpp

376–377 ↗

(On Diff #93322)

Did clang-format do this?

388–390 ↗

(On Diff #93322)

I think you just need the first object in the '+' concatenation to be a Twine -- the rest can be as they are just fine:

Twine("Incomplete thread buffer. Expected ") + (State.CurrentBufferSize - State.CurrentBufferConsumed) + " remaining bytes but found " + S.size()

416–420 ↗

(On Diff #93322)

Same here.

[kpw]

lib/XRay/Trace.cpp

138 ↗

(On Diff #93104)

It was unnecessary thanks to implicit constructors.

423–426 ↗

(On Diff #93104)

Will a conversion operator be called for literal strings past the first.

Is it idiomatic to write Twine("Concatenating ") + "several" + " strings " + " that " + " become twines " and get a Twine as the result?

[dberris]

lib/XRay/Trace.cpp

423–426 ↗

(On Diff #93104)

Yes, that's idiomatic. :)

[kpw]

Few more cleanups for nits and change the freeform space handling to better use DataExtractor.

Twine needs to wrap integral types and clang format did ruin the indentation on FDRState.

[kpw]

lib/XRay/Trace.cpp

376–377 ↗

(On Diff #93322)

Unfortunately yes.

llvm/trunk/include/llvm/XRay/XRayRecord.h

struct XRayFileHeader {
 
   /// Whether the CPU that produced the timestamp counters (TSC) do not stop.
   bool NonstopTSC;
 
   /// The number of cycles per second for the CPU that produced the timestamp
   /// counter (TSC) values. Useful for estimating the amount of time that
   /// elapsed between two TSCs on some platforms.
   uint64_t CycleFrequency = 0;
+
+  // This is different depending on the type of xray record. The naive format
+  // stores a Wallclock timespec. FDR logging stores the size of a thread
+  // buffer.
+  char FreeFormData[16];
 };
 
 /// Determines the supported types of records that could be seen in XRay traces.
 /// This may or may not correspond to actual record types in the raw trace (as
 /// the loader implementation may synthesize this information in the process of
 /// of loading).
 enum class RecordTypes { ENTER, EXIT };
 

llvm/trunk/lib/XRay/Trace.cpp

Error readBinaryFormatHeader(StringRef Data, XRayFileHeader &FileHeader) {
   DataExtractor HeaderExtractor(Data, true, 8);
   uint32_t OffsetPtr = 0;
   FileHeader.Version = HeaderExtractor.getU16(&OffsetPtr);
   FileHeader.Type = HeaderExtractor.getU16(&OffsetPtr);
   uint32_t Bitfield = HeaderExtractor.getU32(&OffsetPtr);
   FileHeader.ConstantTSC = Bitfield & 1uL;
   FileHeader.NonstopTSC = Bitfield & 1uL << 1;
   FileHeader.CycleFrequency = HeaderExtractor.getU64(&OffsetPtr);
-
+  std::memcpy(&FileHeader.FreeFormData, Data.bytes_begin() + OffsetPtr, 16);
   if (FileHeader.Version != 1)
     return make_error<StringError>(
         Twine("Unsupported XRay file version: ") + Twine(FileHeader.Version),
         std::make_error_code(std::errc::invalid_argument));
   return Error::success();
 }
 
 Error loadNaiveFormatLog(StringRef Data, XRayFileHeader &FileHeader,
struct FDRState {
   uint16_t ThreadId;
   uint64_t BaseTSC;
   /// Encode some of the state transitions for the FDR log reader as explicit
   /// checks. These are expectations for the next Record in the stream.
   enum class Token {
     NEW_BUFFER_RECORD_OR_EOF,
     WALLCLOCK_RECORD,
     NEW_CPU_ID_RECORD,
-    FUNCTION_SEQUENCE
+    FUNCTION_SEQUENCE,
+    SCAN_TO_END_OF_THREAD_BUF,
   };
   Token Expects;
+  // Each threads buffer may have trailing garbage to scan over, so we track our
+  // progress.
+  uint64_t CurrentBufferSize;
+  uint64_t CurrentBufferConsumed;
 };
 
+Twine fdrStateToTwine(const FDRState::Token &state) {
+  switch (state) {
+  case FDRState::Token::NEW_BUFFER_RECORD_OR_EOF:
+    return "NEW_BUFFER_RECORD_OR_EOF";
+  case FDRState::Token::WALLCLOCK_RECORD:
+    return "WALLCLOCK_RECORD";
+  case FDRState::Token::NEW_CPU_ID_RECORD:
+    return "NEW_CPU_ID_RECORD";
+  case FDRState::Token::FUNCTION_SEQUENCE:
+    return "FUNCTION_SEQUENCE";
+  case FDRState::Token::SCAN_TO_END_OF_THREAD_BUF:
+    return "SCAN_TO_END_OF_THREAD_BUF";
+  }
+  return "UNKNOWN";
+}
+
 /// State transition when a NewBufferRecord is encountered.
 Error processFDRNewBufferRecord(FDRState &State, uint8_t RecordFirstByte,
                                 DataExtractor &RecordExtractor) {
 
-  if (State.Expects != FDRState::Token::NEW_BUFFER_RECORD_OR_EOF) {
+  if (State.Expects != FDRState::Token::NEW_BUFFER_RECORD_OR_EOF)
     return make_error<StringError>(
         "Malformed log. Read New Buffer record kind out of sequence",
         std::make_error_code(std::errc::executable_format_error));
-  }
   uint32_t OffsetPtr = 1; // 1 byte into record.
   State.ThreadId = RecordExtractor.getU16(&OffsetPtr);
   State.Expects = FDRState::Token::WALLCLOCK_RECORD;
   return Error::success();
 }
 
 /// State transition when an EndOfBufferRecord is encountered.
 Error processFDREndOfBufferRecord(FDRState &State, uint8_t RecordFirstByte,
                                   DataExtractor &RecordExtractor) {
-  if (State.Expects == FDRState::Token::NEW_BUFFER_RECORD_OR_EOF) {
+  if (State.Expects == FDRState::Token::NEW_BUFFER_RECORD_OR_EOF)
     return make_error<StringError>(
         "Malformed log. Received EOB message without current buffer.",
         std::make_error_code(std::errc::executable_format_error));
-  }
-  State.Expects = FDRState::Token::NEW_BUFFER_RECORD_OR_EOF;
+  State.Expects = FDRState::Token::SCAN_TO_END_OF_THREAD_BUF;
   return Error::success();
 }
 
 /// State transition when a NewCPUIdRecord is encountered.
 Error processFDRNewCPUIdRecord(FDRState &State, uint8_t RecordFirstByte,
                                DataExtractor &RecordExtractor) {
   if (State.Expects != FDRState::Token::FUNCTION_SEQUENCE &&
-      State.Expects != FDRState::Token::NEW_CPU_ID_RECORD) {
+      State.Expects != FDRState::Token::NEW_CPU_ID_RECORD)
     return make_error<StringError>(
         "Malformed log. Read NewCPUId record kind out of sequence",
         std::make_error_code(std::errc::executable_format_error));
-  }
   uint32_t OffsetPtr = 1; // Read starting after the first byte.
   State.CPUId = RecordExtractor.getU16(&OffsetPtr);
   State.BaseTSC = RecordExtractor.getU64(&OffsetPtr);
   State.Expects = FDRState::Token::FUNCTION_SEQUENCE;
   return Error::success();
 }
 
 /// State transition when a TSCWrapRecord (overflow detection) is encountered.
 Error processFDRTSCWrapRecord(FDRState &State, uint8_t RecordFirstByte,
                               DataExtractor &RecordExtractor) {
-  if (State.Expects != FDRState::Token::FUNCTION_SEQUENCE) {
+  if (State.Expects != FDRState::Token::FUNCTION_SEQUENCE)
     return make_error<StringError>(
         "Malformed log. Read TSCWrap record kind out of sequence",
         std::make_error_code(std::errc::executable_format_error));
-  }
   uint32_t OffsetPtr = 1; // Read starting after the first byte.
   State.BaseTSC = RecordExtractor.getU64(&OffsetPtr);
   return Error::success();
 }
 
 /// State transition when a WallTimeMarkerRecord is encountered.
 Error processFDRWallTimeRecord(FDRState &State, uint8_t RecordFirstByte,
                                DataExtractor &RecordExtractor) {
-  if (State.Expects != FDRState::Token::WALLCLOCK_RECORD) {
+  if (State.Expects != FDRState::Token::WALLCLOCK_RECORD)
     return make_error<StringError>(
         "Malformed log. Read Wallclock record kind out of sequence",
         std::make_error_code(std::errc::executable_format_error));
-  }
   // We don't encode the wall time into any of the records.
   // XRayRecords are concerned with the TSC instead.
   State.Expects = FDRState::Token::NEW_CPU_ID_RECORD;
   return Error::success();
 }
 
 /// Advances the state machine for reading the FDR record type by reading one
 /// Metadata Record and updating the State approriately based on the kind of

 /// the bit twiddling logic, which is consistent with the little-endian
 /// convention that BitFields within a struct will first be packed into the
 /// least significant bits the address they belong to.
 ///
 /// We expect a format complying with the grammar in the following pseudo-EBNF.
 ///
 /// FDRLog: XRayFileHeader ThreadBuffer*
 /// XRayFileHeader: 32 bits to identify the log as FDR with machine metadata.
-/// ThreadBuffer: NewBuffer WallClockTime NewCPUId FunctionSequence EOB
+/// ThreadBuffer: BufSize NewBuffer WallClockTime NewCPUId FunctionSequence EOB
+/// BufSize: 8 byte unsigned integer indicating how large the buffer is.
 /// NewBuffer: 16 byte metadata record with Thread Id.
 /// WallClockTime: 16 byte metadata record with human readable time.
 /// NewCPUId: 16 byte metadata record with CPUId and a 64 bit TSC reading.
-/// EOB: 16 byte metadata record marking the end of a thread's sequence.
+/// EOB: 16 byte record in a thread buffer plus mem garbage to fill BufSize.
 /// FunctionSequence: NewCPUId | TSCWrap | FunctionRecord
 /// TSCWrap: 16 byte metadata record with a full 64 bit TSC reading.
 /// FunctionRecord: 8 byte record with FunctionId, entry/exit, and TSC delta.
 Error loadFDRLog(StringRef Data, XRayFileHeader &FileHeader,
                  std::vector<XRayRecord> &Records) {
   if (Data.size() < 32)
     return make_error<StringError>(
         "Not enough bytes for an XRay log.",
         std::make_error_code(std::errc::invalid_argument));
 
   // For an FDR log, there are records sized 16 and 8 bytes.
-  if (Data.size() - 32 == 0 || Data.size() % 8 != 0)
+  // There actually may be no records if no non-trivial functions are
+  // instrumented.
+  if (Data.size() % 8 != 0)
     return make_error<StringError>(
         "Invalid-sized XRay data.",
         std::make_error_code(std::errc::invalid_argument));
 
   if (auto E = readBinaryFormatHeader(Data, FileHeader))
     return E;
 
-  FDRState State{0, 0, 0, FDRState::Token::NEW_BUFFER_RECORD_OR_EOF};
+  uint64_t BufferSize = 0;
+  {
+    StringRef ExtraDataRef(FileHeader.FreeFormData, 16);
+    DataExtractor ExtraDataExtractor(ExtraDataRef, true, 8);
+    uint32_t ExtraDataOffset = 0;
+    BufferSize = ExtraDataExtractor.getU64(&ExtraDataOffset);
+  }
+  FDRState State{0,          0, 0, FDRState::Token::NEW_BUFFER_RECORD_OR_EOF,
+                 BufferSize, 0};
   // RecordSize will tell the loop how far to seek ahead based on the record
   // type that we have just read.
   size_t RecordSize = 0;
   for (auto S = Data.drop_front(32); !S.empty(); S = S.drop_front(RecordSize)) {
     DataExtractor RecordExtractor(S, true, 8);
     uint32_t OffsetPtr = 0;
+    if (State.Expects == FDRState::Token::SCAN_TO_END_OF_THREAD_BUF) {
+      RecordSize = State.CurrentBufferSize - State.CurrentBufferConsumed;
+      if (S.size() < State.CurrentBufferSize - State.CurrentBufferConsumed) {
+        return make_error<StringError>(
+            Twine("Incomplete thread buffer. Expected ") +
+                Twine(State.CurrentBufferSize - State.CurrentBufferConsumed) +
+                " remaining bytes but found " + Twine(S.size()),
+            make_error_code(std::errc::invalid_argument));
+      }
+      State.CurrentBufferConsumed = 0;
+      State.Expects = FDRState::Token::NEW_BUFFER_RECORD_OR_EOF;
+      continue;
+    }
     uint8_t BitField = RecordExtractor.getU8(&OffsetPtr);
     bool isMetadataRecord = BitField & 0x01uL;
     if (isMetadataRecord) {
       RecordSize = 16;
       if (auto E = processFDRMetadataRecord(State, BitField, RecordExtractor))
         return E;
+      State.CurrentBufferConsumed += RecordSize;
     } else { // Process Function Record
       RecordSize = 8;
       if (auto E = processFDRFunctionRecord(State, BitField, RecordExtractor,
                                             Records))
         return E;
+      State.CurrentBufferConsumed += RecordSize;
     }
   }
-  if (State.Expects != FDRState::Token::NEW_BUFFER_RECORD_OR_EOF)
-    return make_error<StringError>(
-        "Encountered EOF without preceding End of Buffer record.",
+  // There are two conditions
+  if (State.Expects != FDRState::Token::NEW_BUFFER_RECORD_OR_EOF &&
+      !(State.Expects == FDRState::Token::SCAN_TO_END_OF_THREAD_BUF &&
+        State.CurrentBufferSize == State.CurrentBufferConsumed))
+    return make_error<StringError>(
+        Twine("Encountered EOF with unexpected state expectation ") +
+            fdrStateToTwine(State.Expects) +
+            ". Remaining expected bytes in thread buffer total " +
+            Twine(State.CurrentBufferSize - State.CurrentBufferConsumed),
         std::make_error_code(std::errc::executable_format_error));
 
   return Error::success();
 }
 
 Error loadYAMLLog(StringRef Data, XRayFileHeader &FileHeader,
                   std::vector<XRayRecord> &Records) {
   // Load the documents from the MappedFile.