[PATCH] D50169: [XRay] Improve error reporting when loading traces (NFC)

[Keith via Phabricator via llvm-commits]

kpw added inline comments.


================
Comment at: llvm/lib/XRay/Trace.cpp:71
   FileHeader.NonstopTSC = Bitfield & 1uL << 1;
   FileHeader.CycleFrequency = HeaderExtractor.getU64(&OffsetPtr);
+  std::memcpy(&FileHeader.FreeFormData,
----------------
Missing error check here.


================
Comment at: llvm/lib/XRay/Trace.cpp:113
   //   (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)) {
+    auto PreReadOffset = OffsetPtr;
----------------
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.


================
Comment at: llvm/lib/XRay/Trace.cpp:244
+    // basic mode logs.
+    OffsetPtr += 8;
   }
----------------
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.


================
Comment at: llvm/lib/XRay/Trace.cpp:325
+  // padding in a metadata record.
+  OffsetPtr += kFDRMetadataBodySize - 2;
   return Error::success();
----------------
asserts would be good at the end of these blocks to check that the OffsetPtr has advanced by the body size as expected.


================
Comment at: llvm/lib/XRay/Trace.cpp:478
+  // record.
+  OffsetPtr += (kFDRMetadataBodySize - (8 + 4)) + DataSize;
   return Error::success();
----------------
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.


================
Comment at: llvm/lib/XRay/Trace.cpp:549
+                               uint16_t Version, uint8_t FirstByte) {
+  switch (FirstByte >> 1) {
   case 0: // NewBuffer
----------------
In my opinion this deserves an explanation, but you've deleted the comment.


================
Comment at: llvm/lib/XRay/Trace.cpp:635-637
+    auto T = (FirstByte >> 1) & 0x07;
+    switch (T) {
+    case static_cast<decltype(T)>(RecordTypes::ENTER):
----------------
Do you really believe this combination of auto and decltype is more clear?


================
Comment at: llvm/lib/XRay/Trace.cpp:646-649
+    case static_cast<decltype(T)>(RecordTypes::ENTER_ARG):
+      Record.Type = RecordTypes::ENTER_ARG;
+      State.Expects = FDRState::Token::CALL_ARGUMENT;
+      break;
----------------
While I think this is an improvement, it runs counter to the NFC assertion, no?


================
Comment at: llvm/lib/XRay/Trace.cpp:793-801
     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))
----------------
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.


https://reviews.llvm.org/D50169