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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


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


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


================
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;
----------------
kpw wrote:
> While I think this is an improvement, it runs counter to the NFC assertion, no?
Right, I'll update the title.


================
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))
----------------
kpw wrote:
> 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.
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.


https://reviews.llvm.org/D50169