[XRay] Improve error reporting when loading traces

[dberris]

Summary

This change uses a single offset pointer used throughout the
implementation of the individual record parsers. This allows us to
report where in a trace file parsing failed.

We're still in an intermediate step here as we prepare to refactor this
further into a set of types and use object-oriented design principles
for a cleaner implementation. The next steps will be to allow us to
parse/dump files in a streaming fashion and incrementally build up the
structures in memory instead of the current all-or-nothing approach.

[kpw]

llvm/lib/XRay/Trace.cpp

71 ↗

(On Diff #158682)

Missing error check here.

113 ↗

(On Diff #158682)

It would be simpler to check IsValidOffset(OffsetPtr + 31) at the start of the loop. If there are incomplete records, we can identify, diagnose, and handle them here.

Since the condition for the offset not being updated is "the offset is out of bounds or there are not enough bytes to extract this value" and we know the size of the record, we can check once per record (and check before reading custom event payloads).

Granted, this relies on logic errors not causing reads past the expected 32 bytes, but interspersing all of these error checks on every read obscures the logic imho.

244 ↗

(On Diff #158682)

If you move the preoffset ptr check to the beginning of the loop, we could add an assert here that OffsetPtr ends up 32 bytes ahead to keep the implementation honest.

325 ↗

(On Diff #158682)

asserts would be good at the end of these blocks to check that the OffsetPtr has advanced by the body size as expected.

478 ↗

(On Diff #158682)

If the source of error is that the event size exceeds the actual buffer, it would be more informative to check that here rather than get an error message trying to read the next record.

549 ↗

(On Diff #158682)

In my opinion this deserves an explanation, but you've deleted the comment.

635–637 ↗

(On Diff #158682)

Do you really believe this combination of auto and decltype is more clear?

646–649 ↗

(On Diff #158682)

While I think this is an improvement, it runs counter to the NFC assertion, no?

793–801 ↗

(On Diff #158682)

Consider that (apart from custom events, we know to expect at least either kFDRMetadataBodySize more valid bytes or (sizeof(functionrecord) - 1) == 7) more valid bytes. We could check that here perfectly well and avoid relying on so many different places to correctly update OffsetPtr.

[dberris]

• fixup: address comments by @kpw

[dberris]

llvm/lib/XRay/Trace.cpp

244 ↗

(On Diff #158682)

I've added a check explicitly to see whether we can load enough bytes for a full record at the beginning of the loop instead.

635–637 ↗

(On Diff #158682)

Yes, because this avoids having to guess what the types are (and allowing the compiler decide these). In particular, this doesn't force narrowing/widening conversions of the values resulting from the arithmetic operations.

646–649 ↗

(On Diff #158682)

Right, I'll update the title.

793–801 ↗

(On Diff #158682)

I thought about that, but it makes refactoring this code much harder (which I'm actually doing after this change, in my local repo). Having this one point where the information on the sizing of the records actually makes it harder to reason about the state of the parsing. This single change pushes down the logic for knowing which parts of the record is padding, which is important, etc.

[kpw]

Consider creating wrapper functions around DataExtractor's getU64, getU32, getU16, and getU8 that take a DataExtractor&, an Offset&, a descriptive twine, and return an Expected<T>. I'm trying to come up with ideas to keep all the granular checks as you desire, but pull some of the ceremonial noise out of the flow of the main logic.

the use would be something like:

auto result = expectU16(RecordExtractor, OffsetPtr, "cpu id");
if (auto err = result.takeError()) return err;
State.CPUId = *result;

The implementation (you'd have to duplicate functions for a few different types) would be something like

Expected<uint16_t> expectU16(DataExtractor &RecordExtractor, uint32_t &OffsetPtr, Twine FieldDescriptor) {
  auto PreReadOffset = OffsetPtr;
  auto result = RecordExtractor.getU16(&OffsetPtr);
  if (OffsetPtr == PreReadOffset)
    return createStringError(
        std::make_error_code(std::errc::executable_format_error),
        "Failed reading %s at offset %d.", FieldDescriptor, OffsetPtr);
  return result;
}

I'm not sure this would amount to much less code, but it would move boilerplate out of "the way" and I claim that readers (or future you or I) would be able to follow the core state transitions more easily.

Edit: You could even get away with defining it once as a template function, where you dispatch to specialized wrappers on RecordExtractor:

template<typename T>
auto extract(DataExtractor& extractor, uint32_t& OffsetPtr) -> T; 
// Can you declare with the auto syntax like this without defining?
// Does it even need to match the return type?
 
template<>
uint8_t extract<uint8_t>(DataExtractor& extractor, uint32_t& OffsetPtr) {
   return extractor.getU8(OffsetPtr);
}

...

llvm/lib/XRay/Trace.cpp

121–124 ↗

(On Diff #159247)

This checks makes the other PreReadOffset error checks in this loop defensive coding at best, but for the moment dead code. They aren't hurting anything by being there, and are a defense against future changes, so that's OK if you really want.

[dberris]

Thanks, I considered doing something like that, but it turns out a more principled refactoring makes it easier to reduce the requirement to wrap the data extractor API. It also turns out that we can make this simpler by extracting the "parsing" and validation logic out from the loading/reading logic, and operating at a higher level. I'm close to getting these parts re-written, where I may be able to do something like you're suggesting, but at a more fundamental level.

llvm/trunk/lib/XRay/Trace.cpp

 using namespace llvm;
 using namespace llvm::xray;
 using llvm::yaml::Input;
 
 namespace {
 using XRayRecordStorage =
     std::aligned_storage<sizeof(XRayRecord), alignof(XRayRecord)>::type;
 
+// This is the number of bytes in the "body" of a MetadataRecord in FDR Mode.
+// This already excludes the first byte, which indicates the type of metadata
+// record it is.
+constexpr auto kFDRMetadataBodySize = 15;
+
 // Populates the FileHeader reference by reading the first 32 bytes of the file.
-Error readBinaryFormatHeader(StringRef Data, XRayFileHeader &FileHeader) {
+Error readBinaryFormatHeader(DataExtractor &HeaderExtractor,
+                             uint32_t &OffsetPtr, XRayFileHeader &FileHeader) {
   // FIXME: Maybe deduce whether the data is little or big-endian using some
   // magic bytes in the beginning of the file?
 
   // First 32 bytes of the file will always be the header. We assume a certain
   // format here:
   //
   //   (2)   uint16 : version
   //   (2)   uint16 : type
   //   (4)   uint32 : bitfield
   //   (8)   uint64 : cycle frequency
   //   (16)  -      : padding
 
-  DataExtractor HeaderExtractor(Data, true, 8);
-  uint32_t OffsetPtr = 0;
+  auto PreReadOffset = OffsetPtr;
   FileHeader.Version = HeaderExtractor.getU16(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::invalid_argument),
+        "Failed reading version from file header at offset %d.", OffsetPtr);
+
+  PreReadOffset = OffsetPtr;
   FileHeader.Type = HeaderExtractor.getU16(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::invalid_argument),
+        "Failed reading file type from file header at offset %d.", OffsetPtr);
+
+  PreReadOffset = OffsetPtr;
   uint32_t Bitfield = HeaderExtractor.getU32(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::invalid_argument),
+        "Failed reading flag bits from file header at offset %d.", OffsetPtr);
+
   FileHeader.ConstantTSC = Bitfield & 1uL;
   FileHeader.NonstopTSC = Bitfield & 1uL << 1;
+  PreReadOffset = OffsetPtr;
   FileHeader.CycleFrequency = HeaderExtractor.getU64(&OffsetPtr);
-  std::memcpy(&FileHeader.FreeFormData, Data.bytes_begin() + OffsetPtr, 16);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::invalid_argument),
+        "Failed reading cycle frequency from file header at offset %d.",
+        OffsetPtr);
+
+  std::memcpy(&FileHeader.FreeFormData,
+              HeaderExtractor.getData().bytes_begin() + OffsetPtr, 16);
+
+  // Manually advance the offset pointer 16 bytes, after getting a raw memcpy
+  // from the underlying data.
+  OffsetPtr += 16;
   if (FileHeader.Version != 1 && FileHeader.Version != 2 &&
       FileHeader.Version != 3)
-    return make_error<StringError>(
-        Twine("Unsupported XRay file version: ") + Twine(FileHeader.Version),
-        std::make_error_code(std::errc::invalid_argument));
+    return createStringError(std::make_error_code(std::errc::invalid_argument),
+                             "Unsupported XRay file version: %d at offset %d",
+                             FileHeader.Version, OffsetPtr);
   return Error::success();
 }
 
 Error loadNaiveFormatLog(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));
 
   if (Data.size() - 32 == 0 || Data.size() % 32 != 0)
     return make_error<StringError>(
         "Invalid-sized XRay data.",
         std::make_error_code(std::errc::invalid_argument));
 
-  if (auto E = readBinaryFormatHeader(Data, FileHeader))
+  DataExtractor Reader(Data, true, 8);
+  uint32_t OffsetPtr = 0;
+  if (auto E = readBinaryFormatHeader(Reader, OffsetPtr, FileHeader))
     return E;
 
   // Each record after the header will be 32 bytes, in the following format:
   //
   //   (2)   uint16 : record type
   //   (1)   uint8  : cpu id
   //   (1)   uint8  : type
   //   (4)   sint32 : function id
   //   (8)   uint64 : tsc
   //   (4)   uint32 : thread id
   //   (4)   uint32 : process id
   //   (8)   -      : padding
-  for (auto S = Data.drop_front(32); !S.empty(); S = S.drop_front(32)) {
-    DataExtractor RecordExtractor(S, true, 8);
-    uint32_t OffsetPtr = 0;
-    switch (auto RecordType = RecordExtractor.getU16(&OffsetPtr)) {
+  while (Reader.isValidOffset(OffsetPtr)) {
+    if (!Reader.isValidOffsetForDataOfSize(OffsetPtr, 32))
+      return createStringError(
+          std::make_error_code(std::errc::executable_format_error),
+          "Not enough bytes to read a full record at offset %d.", OffsetPtr);
+    auto PreReadOffset = OffsetPtr;
+    auto RecordType = Reader.getU16(&OffsetPtr);
+    if (OffsetPtr == PreReadOffset)
+      return createStringError(
+          std::make_error_code(std::errc::executable_format_error),
+          "Failed reading record type at offset %d.", OffsetPtr);
+
+    switch (RecordType) {
     case 0: { // Normal records.
       Records.emplace_back();
       auto &Record = Records.back();
       Record.RecordType = RecordType;
-      Record.CPU = RecordExtractor.getU8(&OffsetPtr);
-      auto Type = RecordExtractor.getU8(&OffsetPtr);
+
+      PreReadOffset = OffsetPtr;
+      Record.CPU = Reader.getU8(&OffsetPtr);
+      if (OffsetPtr == PreReadOffset)
+        return createStringError(
+            std::make_error_code(std::errc::executable_format_error),
+            "Failed reading CPU field at offset %d.", OffsetPtr);
+
+      PreReadOffset = OffsetPtr;
+      auto Type = Reader.getU8(&OffsetPtr);
+      if (OffsetPtr == PreReadOffset)
+        return createStringError(
+            std::make_error_code(std::errc::executable_format_error),
+            "Failed reading record type field at offset %d.", OffsetPtr);
+
       switch (Type) {
       case 0:
         Record.Type = RecordTypes::ENTER;
         break;
       case 1:
         Record.Type = RecordTypes::EXIT;
         break;
       case 2:
         Record.Type = RecordTypes::TAIL_EXIT;
         break;
       case 3:
         Record.Type = RecordTypes::ENTER_ARG;
         break;
       default:
-        return make_error<StringError>(
-            Twine("Unknown record type '") + Twine(int{Type}) + "'",
-            std::make_error_code(std::errc::executable_format_error));
-      }
-      Record.FuncId = RecordExtractor.getSigned(&OffsetPtr, sizeof(int32_t));
-      Record.TSC = RecordExtractor.getU64(&OffsetPtr);
-      Record.TId = RecordExtractor.getU32(&OffsetPtr);
-      Record.PId = RecordExtractor.getU32(&OffsetPtr);
+        return createStringError(
+            std::make_error_code(std::errc::executable_format_error),
+            "Unknown record type '%d' at offset %d.", Type, OffsetPtr);
+      }
+
+      PreReadOffset = OffsetPtr;
+      Record.FuncId = Reader.getSigned(&OffsetPtr, sizeof(int32_t));
+      if (OffsetPtr == PreReadOffset)
+        return createStringError(
+            std::make_error_code(std::errc::executable_format_error),
+            "Failed reading function id field at offset %d.", OffsetPtr);
+
+      PreReadOffset = OffsetPtr;
+      Record.TSC = Reader.getU64(&OffsetPtr);
+      if (OffsetPtr == PreReadOffset)
+        return createStringError(
+            std::make_error_code(std::errc::executable_format_error),
+            "Failed reading TSC field at offset %d.", OffsetPtr);
+
+      PreReadOffset = OffsetPtr;
+      Record.TId = Reader.getU32(&OffsetPtr);
+      if (OffsetPtr == PreReadOffset)
+        return createStringError(
+            std::make_error_code(std::errc::executable_format_error),
+            "Failed reading thread id field at offset %d.", OffsetPtr);
+
+      PreReadOffset = OffsetPtr;
+      Record.PId = Reader.getU32(&OffsetPtr);
+      if (OffsetPtr == PreReadOffset)
+        return createStringError(
+            std::make_error_code(std::errc::executable_format_error),
+            "Failed reading process id at offset %d.", OffsetPtr);
+
       break;
     }
     case 1: { // Arg payload record.
       auto &Record = Records.back();
-      // Advance two bytes to avoid padding.
+
+      // We skip the next two bytes of the record, because we don't need the
+      // type and the CPU record for arg payloads.
       OffsetPtr += 2;
-      int32_t FuncId = RecordExtractor.getSigned(&OffsetPtr, sizeof(int32_t));
-      auto TId = RecordExtractor.getU32(&OffsetPtr);
-      auto PId = RecordExtractor.getU32(&OffsetPtr);
+      PreReadOffset = OffsetPtr;
+      int32_t FuncId = Reader.getSigned(&OffsetPtr, sizeof(int32_t));
+      if (OffsetPtr == PreReadOffset)
+        return createStringError(
+            std::make_error_code(std::errc::executable_format_error),
+            "Failed reading function id field at offset %d.", OffsetPtr);
+
+      PreReadOffset = OffsetPtr;
+      auto TId = Reader.getU32(&OffsetPtr);
+      if (OffsetPtr == PreReadOffset)
+        return createStringError(
+            std::make_error_code(std::errc::executable_format_error),
+            "Failed reading thread id field at offset %d.", OffsetPtr);
+
+      PreReadOffset = OffsetPtr;
+      auto PId = Reader.getU32(&OffsetPtr);
+      if (OffsetPtr == PreReadOffset)
+        return createStringError(
+            std::make_error_code(std::errc::executable_format_error),
+            "Failed reading process id field at offset %d.", OffsetPtr);
 
       // Make a check for versions above 3 for the Pid field
       if (Record.FuncId != FuncId || Record.TId != TId ||
           (FileHeader.Version >= 3 ? Record.PId != PId : false))
-        return make_error<StringError>(
-            Twine("Corrupted log, found arg payload following non-matching "
-                  "function + thread record. Record for function ") +
-                Twine(Record.FuncId) + " != " + Twine(FuncId) + "; offset: " +
-                Twine(S.data() - Data.data()),
-            std::make_error_code(std::errc::executable_format_error));
+        return createStringError(
+            std::make_error_code(std::errc::executable_format_error),
+            "Corrupted log, found arg payload following non-matching "
+            "function+thread record. Record for function %d != %d at offset "
+            "%d",
+            Record.FuncId, FuncId, OffsetPtr);
+
+      PreReadOffset = OffsetPtr;
+      auto Arg = Reader.getU64(&OffsetPtr);
+      if (OffsetPtr == PreReadOffset)
+        return createStringError(
+            std::make_error_code(std::errc::executable_format_error),
+            "Failed reading argument payload at offset %d.", OffsetPtr);
 
-      auto Arg = RecordExtractor.getU64(&OffsetPtr);
       Record.CallArgs.push_back(Arg);
       break;
     }
     default:
-      return make_error<StringError>(
-          Twine("Unknown record type == ") + Twine(RecordType),
-          std::make_error_code(std::errc::executable_format_error));
-    }
+      return createStringError(
+          std::make_error_code(std::errc::executable_format_error),
+          "Unknown record type '%d' at offset %d.", RecordType, OffsetPtr);
+    }
+    // Advance the offset pointer enough bytes to align to 32-byte records for
+    // basic mode logs.
+    OffsetPtr += 8;
   }
   return Error::success();
 }
 
 /// When reading from a Flight Data Recorder mode log, metadata records are
 /// sparse compared to packed function records, so we must maintain state as we
 /// read through the sequence of entries. This allows the reader to denormalize
 /// the CPUId and Thread Id onto each Function Record and transform delta
case FDRState::Token::BUFFER_EXTENTS:
     return "BUFFER_EXTENTS";
   case FDRState::Token::PID_RECORD:
     return "PID_RECORD";
   }
   return "UNKNOWN";
 }
 
 /// State transition when a NewBufferRecord is encountered.
-Error processFDRNewBufferRecord(FDRState &State, uint8_t RecordFirstByte,
-                                DataExtractor &RecordExtractor) {
-
+Error processFDRNewBufferRecord(FDRState &State, DataExtractor &RecordExtractor,
+                                uint32_t &OffsetPtr) {
   if (State.Expects != FDRState::Token::NEW_BUFFER_RECORD_OR_EOF)
-    return make_error<StringError>(
-        Twine("Malformed log. Read New Buffer record kind out of sequence; "
-              "expected: ") +
-            fdrStateToTwine(State.Expects),
-        std::make_error_code(std::errc::executable_format_error));
-  uint32_t OffsetPtr = 1; // 1 byte into record.
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Malformed log: Read New Buffer record kind out of sequence; expected: "
+        "%s at offset %d.",
+        fdrStateToTwine(State.Expects), OffsetPtr);
+
+  auto PreReadOffset = OffsetPtr;
   State.ThreadId = RecordExtractor.getU16(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Failed reading the thread id at offset %d.", OffsetPtr);
   State.Expects = FDRState::Token::WALLCLOCK_RECORD;
+
+  // Advance the offset pointer by enough bytes representing the remaining
+  // padding in a metadata record.
+  OffsetPtr += kFDRMetadataBodySize - 2;
+  assert(OffsetPtr - PreReadOffset == kFDRMetadataBodySize);
   return Error::success();
 }
 
 /// State transition when an EndOfBufferRecord is encountered.
-Error processFDREndOfBufferRecord(FDRState &State, uint8_t RecordFirstByte,
-                                  DataExtractor &RecordExtractor) {
+Error processFDREndOfBufferRecord(FDRState &State, uint32_t &OffsetPtr) {
   if (State.Expects == FDRState::Token::NEW_BUFFER_RECORD_OR_EOF)
-    return make_error<StringError>(
-        Twine("Malformed log. Received EOB message without current buffer; "
-              "expected: ") +
-            fdrStateToTwine(State.Expects),
-        std::make_error_code(std::errc::executable_format_error));
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Malformed log: Received EOB message without current buffer; expected: "
+        "%s at offset %d.",
+        fdrStateToTwine(State.Expects), OffsetPtr);
+
   State.Expects = FDRState::Token::SCAN_TO_END_OF_THREAD_BUF;
+
+  // Advance the offset pointer by enough bytes representing the remaining
+  // padding in a metadata record.
+  OffsetPtr += kFDRMetadataBodySize;
   return Error::success();
 }
 
 /// State transition when a NewCPUIdRecord is encountered.
-Error processFDRNewCPUIdRecord(FDRState &State, uint8_t RecordFirstByte,
-                               DataExtractor &RecordExtractor) {
+Error processFDRNewCPUIdRecord(FDRState &State, DataExtractor &RecordExtractor,
+                               uint32_t &OffsetPtr) {
   if (State.Expects != FDRState::Token::FUNCTION_SEQUENCE &&
       State.Expects != FDRState::Token::NEW_CPU_ID_RECORD)
     return make_error<StringError>(
         Twine("Malformed log. Read NewCPUId record kind out of sequence; "
               "expected: ") +
             fdrStateToTwine(State.Expects),
         std::make_error_code(std::errc::executable_format_error));
-  uint32_t OffsetPtr = 1; // Read starting after the first byte.
+  auto BeginOffset = OffsetPtr;
+  auto PreReadOffset = OffsetPtr;
   State.CPUId = RecordExtractor.getU16(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Failed reading the CPU field at offset %d.", OffsetPtr);
+
+  PreReadOffset = OffsetPtr;
   State.BaseTSC = RecordExtractor.getU64(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Failed reading the base TSC field at offset %d.", OffsetPtr);
+
   State.Expects = FDRState::Token::FUNCTION_SEQUENCE;
+
+  // Advance the offset pointer by a few bytes, to account for the padding in
+  // CPU ID metadata records that we've already advanced through.
+  OffsetPtr += kFDRMetadataBodySize - (OffsetPtr - BeginOffset);
+  assert(OffsetPtr - BeginOffset == kFDRMetadataBodySize);
   return Error::success();
 }
 
 /// State transition when a TSCWrapRecord (overflow detection) is encountered.
-Error processFDRTSCWrapRecord(FDRState &State, uint8_t RecordFirstByte,
-                              DataExtractor &RecordExtractor) {
+Error processFDRTSCWrapRecord(FDRState &State, DataExtractor &RecordExtractor,
+                              uint32_t &OffsetPtr) {
   if (State.Expects != FDRState::Token::FUNCTION_SEQUENCE)
     return make_error<StringError>(
         Twine("Malformed log. Read TSCWrap record kind out of sequence; "
               "expecting: ") +
             fdrStateToTwine(State.Expects),
         std::make_error_code(std::errc::executable_format_error));
-  uint32_t OffsetPtr = 1; // Read starting after the first byte.
+  auto PreReadOffset = OffsetPtr;
   State.BaseTSC = RecordExtractor.getU64(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Failed reading the base TSC field at offset %d.", OffsetPtr);
+
+  // Advance the offset pointer by a few more bytes, accounting for the padding
+  // in the metadata record after reading the base TSC.
+  OffsetPtr += kFDRMetadataBodySize - 8;
+  assert(OffsetPtr - PreReadOffset == kFDRMetadataBodySize);
   return Error::success();
 }
 
 /// State transition when a WallTimeMarkerRecord is encountered.
-Error processFDRWallTimeRecord(FDRState &State, uint8_t RecordFirstByte,
-                               DataExtractor &RecordExtractor) {
+Error processFDRWallTimeRecord(FDRState &State, DataExtractor &RecordExtractor,
+                               uint32_t &OffsetPtr) {
   if (State.Expects != FDRState::Token::WALLCLOCK_RECORD)
     return make_error<StringError>(
         Twine("Malformed log. Read Wallclock record kind out of sequence; "
               "expecting: ") +
             fdrStateToTwine(State.Expects),
         std::make_error_code(std::errc::executable_format_error));
 
+  // Read in the data from the walltime record.
+  auto PreReadOffset = OffsetPtr;
+  auto WallTime = RecordExtractor.getU64(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Failed reading the walltime record at offset %d.", OffsetPtr);
+
   // TODO: Someday, reconcile the TSC ticks to wall clock time for presentation
   // purposes. For now, we're ignoring these records.
+  (void)WallTime;
   State.Expects = FDRState::Token::NEW_CPU_ID_RECORD;
+
+  // Advance the offset pointer by a few more bytes, accounting for the padding
+  // in the metadata record after reading in the walltime data.
+  OffsetPtr += kFDRMetadataBodySize - 8;
+  assert(OffsetPtr - PreReadOffset == kFDRMetadataBodySize);
   return Error::success();
 }
 
 /// State transition when a PidRecord is encountered.
-Error processFDRPidRecord(FDRState &State, uint8_t RecordFirstByte,
-                          DataExtractor &RecordExtractor) {
-
+Error processFDRPidRecord(FDRState &State, DataExtractor &RecordExtractor,
+                          uint32_t &OffsetPtr) {
   if (State.Expects != FDRState::Token::PID_RECORD)
     return make_error<StringError>(
         Twine("Malformed log. Read Pid record kind out of sequence; "
               "expected: ") +
             fdrStateToTwine(State.Expects),
         std::make_error_code(std::errc::executable_format_error));
-
-  uint32_t OffsetPtr = 1; // Read starting after the first byte.
+  auto PreReadOffset = OffsetPtr;
   State.ProcessId = RecordExtractor.getU32(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Failed reading the process ID at offset %d.", OffsetPtr);
   State.Expects = FDRState::Token::NEW_CPU_ID_RECORD;
+
+  // Advance the offset pointer by a few more bytes, accounting for the padding
+  // in the metadata record after reading in the PID.
+  OffsetPtr += kFDRMetadataBodySize - 4;
+  assert(OffsetPtr - PreReadOffset == kFDRMetadataBodySize);
   return Error::success();
 }
 
 /// State transition when a CustomEventMarker is encountered.
-Error processCustomEventMarker(FDRState &State, uint8_t RecordFirstByte,
-                               DataExtractor &RecordExtractor,
-                               size_t &RecordSize) {
+Error processCustomEventMarker(FDRState &State, DataExtractor &RecordExtractor,
+                               uint32_t &OffsetPtr) {
   // We can encounter a CustomEventMarker anywhere in the log, so we can handle
   // it regardless of the expectation. However, we do set the expectation to
   // read a set number of fixed bytes, as described in the metadata.
-  uint32_t OffsetPtr = 1; // Read after the first byte.
+  auto BeginOffset = OffsetPtr;
+  auto PreReadOffset = OffsetPtr;
   uint32_t DataSize = RecordExtractor.getU32(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Failed reading a custom event marker at offset %d.", OffsetPtr);
+
+  PreReadOffset = OffsetPtr;
   uint64_t TSC = RecordExtractor.getU64(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Failed reading the TSC at offset %d.", OffsetPtr);
 
   // FIXME: Actually represent the record through the API. For now we only
   // skip through the data.
   (void)TSC;
-  RecordSize = 16 + DataSize;
+  // Advance the offset ptr by the size of the data associated with the custom
+  // event, as well as the padding associated with the remainder of the metadata
+  // record.
+  OffsetPtr += (kFDRMetadataBodySize - (OffsetPtr - BeginOffset)) + DataSize;
+  if (!RecordExtractor.isValidOffset(OffsetPtr))
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Reading custom event data moves past addressable trace data (starting "
+        "at offset %d, advancing to offset %d).",
+        BeginOffset, OffsetPtr);
   return Error::success();
 }
 
 /// State transition when an BufferExtents record is encountered.
-Error processBufferExtents(FDRState &State, uint8_t RecordFirstByte,
-                           DataExtractor &RecordExtractor) {
+Error processBufferExtents(FDRState &State, DataExtractor &RecordExtractor,
+                           uint32_t &OffsetPtr) {
   if (State.Expects != FDRState::Token::BUFFER_EXTENTS)
     return make_error<StringError>(
         Twine("Malformed log. Buffer Extents unexpected; expected: ") +
             fdrStateToTwine(State.Expects),
         std::make_error_code(std::errc::executable_format_error));
-  uint32_t OffsetPtr = 1; // Read after the first byte.
+
+  auto PreReadOffset = OffsetPtr;
   State.CurrentBufferSize = RecordExtractor.getU64(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Failed to read current buffer size at offset %d.", OffsetPtr);
+
   State.Expects = FDRState::Token::NEW_BUFFER_RECORD_OR_EOF;
+
+  // Advance the offset pointer by enough bytes accounting for the padding in a
+  // metadata record, after we read in the buffer extents.
+  OffsetPtr += kFDRMetadataBodySize - 8;
   return Error::success();
 }
 
 /// State transition when a CallArgumentRecord is encountered.
-Error processFDRCallArgumentRecord(FDRState &State, uint8_t RecordFirstByte,
+Error processFDRCallArgumentRecord(FDRState &State,
                                    DataExtractor &RecordExtractor,
-                                   std::vector<XRayRecord> &Records) {
-  uint32_t OffsetPtr = 1; // Read starting after the first byte.
+                                   std::vector<XRayRecord> &Records,
+                                   uint32_t &OffsetPtr) {
   auto &Enter = Records.back();
-
-  if (Enter.Type != RecordTypes::ENTER)
+  if (Enter.Type != RecordTypes::ENTER && Enter.Type != RecordTypes::ENTER_ARG)
     return make_error<StringError>(
         "CallArgument needs to be right after a function entry",
         std::make_error_code(std::errc::executable_format_error));
+
+  auto PreReadOffset = OffsetPtr;
+  auto Arg = RecordExtractor.getU64(&OffsetPtr);
+  if (OffsetPtr == PreReadOffset)
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Failed to read argument record at offset %d.", OffsetPtr);
+
   Enter.Type = RecordTypes::ENTER_ARG;
-  Enter.CallArgs.emplace_back(RecordExtractor.getU64(&OffsetPtr));
+  Enter.CallArgs.emplace_back(Arg);
+
+  // Advance the offset pointer by enough bytes accounting for the padding in a
+  // metadata record, after reading the payload.
+  OffsetPtr += kFDRMetadataBodySize - 8;
   return Error::success();
 }
 
 /// Advances the state machine for reading the FDR record type by reading one
 /// Metadata Record and updating the State appropriately based on the kind of
 /// record encountered. The RecordKind is encoded in the first byte of the
 /// Record, which the caller should pass in because they have already read it
 /// to determine that this is a metadata record as opposed to a function record.
 ///
 /// Beginning with Version 2 of the FDR log, we do not depend on the size of the
 /// buffer, but rather use the extents to determine how far to read in the log
 /// for this particular buffer.
 ///
 /// In Version 3, FDR log now includes a pid metadata record after
 /// WallTimeMarker
-Error processFDRMetadataRecord(FDRState &State, uint8_t RecordFirstByte,
-                               DataExtractor &RecordExtractor,
-                               size_t &RecordSize,
+Error processFDRMetadataRecord(FDRState &State, DataExtractor &RecordExtractor,
+                               uint32_t &OffsetPtr,
                                std::vector<XRayRecord> &Records,
-                               uint16_t Version) {
-  // The remaining 7 bits are the RecordKind enum.
-  uint8_t RecordKind = RecordFirstByte >> 1;
-  switch (RecordKind) {
+                               uint16_t Version, uint8_t FirstByte) {
+  // The remaining 7 bits of the first byte are the RecordKind enum for each
+  // Metadata Record.
+  switch (FirstByte >> 1) {
   case 0: // NewBuffer
-    if (auto E =
-            processFDRNewBufferRecord(State, RecordFirstByte, RecordExtractor))
+    if (auto E = processFDRNewBufferRecord(State, RecordExtractor, OffsetPtr))
       return E;
     break;
   case 1: // EndOfBuffer
     if (Version >= 2)
       return make_error<StringError>(
           "Since Version 2 of FDR logging, we no longer support EOB records.",
           std::make_error_code(std::errc::executable_format_error));
-    if (auto E = processFDREndOfBufferRecord(State, RecordFirstByte,
-                                             RecordExtractor))
+    if (auto E = processFDREndOfBufferRecord(State, OffsetPtr))
       return E;
     break;
   case 2: // NewCPUId
-    if (auto E =
-            processFDRNewCPUIdRecord(State, RecordFirstByte, RecordExtractor))
+    if (auto E = processFDRNewCPUIdRecord(State, RecordExtractor, OffsetPtr))
       return E;
     break;
   case 3: // TSCWrap
-    if (auto E =
-            processFDRTSCWrapRecord(State, RecordFirstByte, RecordExtractor))
+    if (auto E = processFDRTSCWrapRecord(State, RecordExtractor, OffsetPtr))
       return E;
     break;
   case 4: // WallTimeMarker
-    if (auto E =
-            processFDRWallTimeRecord(State, RecordFirstByte, RecordExtractor))
+    if (auto E = processFDRWallTimeRecord(State, RecordExtractor, OffsetPtr))
       return E;
     // In Version 3 and and above, a PidRecord is expected after WallTimeRecord
     if (Version >= 3)
       State.Expects = FDRState::Token::PID_RECORD;
     break;
   case 5: // CustomEventMarker
-    if (auto E = processCustomEventMarker(State, RecordFirstByte,
-                                          RecordExtractor, RecordSize))
+    if (auto E = processCustomEventMarker(State, RecordExtractor, OffsetPtr))
       return E;
     break;
   case 6: // CallArgument
-    if (auto E = processFDRCallArgumentRecord(State, RecordFirstByte,
-                                              RecordExtractor, Records))
+    if (auto E = processFDRCallArgumentRecord(State, RecordExtractor, Records,
+                                              OffsetPtr))
       return E;
     break;
   case 7: // BufferExtents
-    if (auto E = processBufferExtents(State, RecordFirstByte, RecordExtractor))
+    if (auto E = processBufferExtents(State, RecordExtractor, OffsetPtr))
       return E;
     break;
   case 9: // Pid
-    if (auto E = processFDRPidRecord(State, RecordFirstByte, RecordExtractor))
+    if (auto E = processFDRPidRecord(State, RecordExtractor, OffsetPtr))
       return E;
     break;
   default:
-    // Widen the record type to uint16_t to prevent conversion to char.
-    return make_error<StringError>(
-        Twine("Illegal metadata record type: ")
-            .concat(Twine(static_cast<unsigned>(RecordKind))),
-        std::make_error_code(std::errc::executable_format_error));
+    return createStringError(
+        std::make_error_code(std::errc::executable_format_error),
+        "Illegal metadata record type: '%d' at offset %d.", FirstByte >> 1,
+        OffsetPtr);
   }
   return Error::success();
 }
 
 /// Reads a function record from an FDR format log, appending a new XRayRecord
 /// to the vector being populated and updating the State with a new value
 /// reference value to interpret TSC deltas.
 ///
 /// The XRayRecord constructed includes information from the function record
 /// processed here as well as Thread ID and CPU ID formerly extracted into
 /// State.
-Error processFDRFunctionRecord(FDRState &State, uint8_t RecordFirstByte,
-                               DataExtractor &RecordExtractor,
+Error processFDRFunctionRecord(FDRState &State, DataExtractor &RecordExtractor,
+                               uint32_t &OffsetPtr, uint8_t FirstByte,
                                std::vector<XRayRecord> &Records) {
   switch (State.Expects) {
   case FDRState::Token::NEW_BUFFER_RECORD_OR_EOF:
     return make_error<StringError>(
         "Malformed log. Received Function Record before new buffer setup.",
         std::make_error_code(std::errc::executable_format_error));
   case FDRState::Token::WALLCLOCK_RECORD:
     return make_error<StringError>(
         "Malformed log. Received Function Record when expecting wallclock.",
         std::make_error_code(std::errc::executable_format_error));
   case FDRState::Token::PID_RECORD:
     return make_error<StringError>(
         "Malformed log. Received Function Record when expecting pid.",
         std::make_error_code(std::errc::executable_format_error));
   case FDRState::Token::NEW_CPU_ID_RECORD:
     return make_error<StringError>(
         "Malformed log. Received Function Record before first CPU record.",
         std::make_error_code(std::errc::executable_format_error));
   default:
     Records.emplace_back();
     auto &Record = Records.back();
     Record.RecordType = 0; // Record is type NORMAL.
     // Strip off record type bit and use the next three bits.
-    uint8_t RecordType = (RecordFirstByte >> 1) & 0x07;
-    switch (RecordType) {
-    case static_cast<uint8_t>(RecordTypes::ENTER):
+    auto T = (FirstByte >> 1) & 0x07;
+    switch (T) {
+    case static_cast<decltype(T)>(RecordTypes::ENTER):
       Record.Type = RecordTypes::ENTER;
       break;
-    case static_cast<uint8_t>(RecordTypes::EXIT):
+    case static_cast<decltype(T)>(RecordTypes::EXIT):
       Record.Type = RecordTypes::EXIT;
       break;
-    case static_cast<uint8_t>(RecordTypes::TAIL_EXIT):
+    case static_cast<decltype(T)>(RecordTypes::TAIL_EXIT):
       Record.Type = RecordTypes::TAIL_EXIT;
       break;
+    case static_cast<decltype(T)>(RecordTypes::ENTER_ARG):
+      Record.Type = RecordTypes::ENTER_ARG;
+      State.Expects = FDRState::Token::CALL_ARGUMENT;
+      break;
     default:
-      // Cast to an unsigned integer to not interpret the record type as a char.
-      return make_error<StringError>(
-          Twine("Illegal function record type: ")
-              .concat(Twine(static_cast<unsigned>(RecordType))),
-          std::make_error_code(std::errc::executable_format_error));
+      return createStringError(
+          std::make_error_code(std::errc::executable_format_error),
+          "Illegal function record type '%d' at offset %d.", T, OffsetPtr);
     }
     Record.CPU = State.CPUId;
     Record.TId = State.ThreadId;
     Record.PId = State.ProcessId;
-    // Back up to read first 32 bits, including the 4 we pulled RecordType
-    // and RecordKind out of. The remaining 28 are FunctionId.
-    uint32_t OffsetPtr = 0;
+
+    // Back up one byte to re-read the first byte, which is important for
+    // computing the function id for a record.
+    --OffsetPtr;
+
     // Despite function Id being a signed int on XRayRecord,
     // when it is written to an FDR format, the top bits are truncated,
     // so it is effectively an unsigned value. When we shift off the
     // top four bits, we want the shift to be logical, so we read as
     // uint32_t.
+    auto PreReadOffset = OffsetPtr;
     uint32_t FuncIdBitField = RecordExtractor.getU32(&OffsetPtr);
+    if (OffsetPtr == PreReadOffset)
+      return createStringError(
+          std::make_error_code(std::errc::executable_format_error),
+          "Failed reading truncated function id field at offset %d.",
+          OffsetPtr);
+
     Record.FuncId = FuncIdBitField >> 4;
+
     // FunctionRecords have a 32 bit delta from the previous absolute TSC
     // or TSC delta. If this would overflow, we should read a TSCWrap record
     // with an absolute TSC reading.
+    PreReadOffset = OffsetPtr;
     uint64_t NewTSC = State.BaseTSC + RecordExtractor.getU32(&OffsetPtr);
+    if (OffsetPtr == PreReadOffset)
+      return createStringError(
+          std::make_error_code(std::errc::executable_format_error),
+          "Failed reading TSC delta at offset %d.", OffsetPtr);
+
     State.BaseTSC = NewTSC;
     Record.TSC = NewTSC;
   }
   return Error::success();
 }
 
 /// Reads a log in FDR mode for version 1 of this binary format. FDR mode is
 /// defined as part of the compiler-rt project in xray_fdr_logging.h, and such

 /// EOB: *deprecated*
 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.
-  // 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));
+  DataExtractor Reader(Data, true, 8);
+  uint32_t OffsetPtr = 0;
 
-  if (auto E = readBinaryFormatHeader(Data, FileHeader))
+  if (auto E = readBinaryFormatHeader(Reader, OffsetPtr, FileHeader))
     return E;
 
   uint64_t BufferSize = 0;
   {
     StringRef ExtraDataRef(FileHeader.FreeFormData, 16);
     DataExtractor ExtraDataExtractor(ExtraDataRef, true, 8);
     uint32_t ExtraDataOffset = 0;
     BufferSize = ExtraDataExtractor.getU64(&ExtraDataOffset);
default:
     return make_error<StringError>(
         Twine("Unsupported version '") + Twine(FileHeader.Version) + "'",
         std::make_error_code(std::errc::executable_format_error));
   }
   FDRState State{0, 0, 0, 0, InitialExpectation, 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;
+  while (Reader.isValidOffset(OffsetPtr)) {
+    auto BeginOffset = OffsetPtr;
     if (State.Expects == FDRState::Token::SCAN_TO_END_OF_THREAD_BUF) {
-      RecordSize = State.CurrentBufferSize - State.CurrentBufferConsumed;
-      if (S.size() < RecordSize) {
-        return make_error<StringError>(
-            Twine("Incomplete thread buffer. Expected at least ") +
-                Twine(RecordSize) + " bytes but found " + Twine(S.size()),
-            make_error_code(std::errc::invalid_argument));
-      }
+      OffsetPtr += State.CurrentBufferSize - State.CurrentBufferConsumed;
       State.CurrentBufferConsumed = 0;
       State.Expects = FDRState::Token::NEW_BUFFER_RECORD_OR_EOF;
       continue;
     }
-    uint8_t BitField = RecordExtractor.getU8(&OffsetPtr);
+    auto PreReadOffset = OffsetPtr;
+    uint8_t BitField = Reader.getU8(&OffsetPtr);
+    if (OffsetPtr == PreReadOffset)
+      return createStringError(
+          std::make_error_code(std::errc::executable_format_error),
+          "Failed reading first byte of record at offset %d.", OffsetPtr);
     bool isMetadataRecord = BitField & 0x01uL;
     bool isBufferExtents =
         (BitField >> 1) == 7; // BufferExtents record kind == 7
     if (isMetadataRecord) {
-      RecordSize = 16;
-      if (auto E =
-              processFDRMetadataRecord(State, BitField, RecordExtractor,
-                                       RecordSize, Records, FileHeader.Version))
+      if (auto E = processFDRMetadataRecord(State, Reader, OffsetPtr, Records,
+                                            FileHeader.Version, BitField))
         return E;
     } else { // Process Function Record
-      RecordSize = 8;
-      if (auto E = processFDRFunctionRecord(State, BitField, RecordExtractor,
+      if (auto E = processFDRFunctionRecord(State, Reader, OffsetPtr, BitField,
                                             Records))
         return E;
     }
 
     // The BufferExtents record is technically not part of the buffer, so we
     // don't count the size of that record against the buffer's actual size.
     if (!isBufferExtents)
-      State.CurrentBufferConsumed += RecordSize;
+      State.CurrentBufferConsumed += OffsetPtr - BeginOffset;
+
     assert(State.CurrentBufferConsumed <= State.CurrentBufferSize);
+
     if ((FileHeader.Version == 2 || FileHeader.Version == 3) &&
         State.CurrentBufferSize == State.CurrentBufferConsumed) {
       // In Version 2 of the log, we don't need to scan to the end of the thread
       // buffer if we've already consumed all the bytes we need to.
       State.Expects = FDRState::Token::BUFFER_EXTENTS;
       State.CurrentBufferSize = BufferSize;
       State.CurrentBufferConsumed = 0;
     }
case FLIGHT_DATA_RECORDER_FORMAT:
     break;
   default:
     if (auto E = loadYAMLLog(Data, T.FileHeader, T.Records))
       return std::move(E);
   }
 
   if (Sort)
     std::stable_sort(T.Records.begin(), T.Records.end(),
                      [&](const XRayRecord &L, const XRayRecord &R) {
                        return L.TSC < R.TSC;
                      });
 
   return std::move(T);
 }