[XRay] fix and clarify comments in the log file decoder [NFC]

[pelikan]

Summary

For readers unfamiliar with the XRay code base, explain our little-endian views more clearly. For code clarity either get rid of obvious comments or explain their intentions, fix typos, correct coding style according to LLVM's standards and manually CSE long expressions to point out it is the same expression.

[dberris]

Some nits.

lib/XRay/Trace.cpp

62 ↗

(On Diff #102990)

The error was already correct -- there's not enough data for an XRay log.

225 ↗

(On Diff #102990)

see -> set

327 ↗

(On Diff #102990)

unsigned integer?

441–442 ↗

(On Diff #102990)

Empty line between non-comment line and first comment line?

[dberris]

Actually, also change the description to be more prose and contain [NFC] somewhere?

[pelikan]

address code review comments

[pelikan]

lib/XRay/Trace.cpp

62 ↗

(On Diff #102990)

I'd appreciate for it to be more specific but at least now we can keep it NFC.

327 ↗

(On Diff #102990)

You are technically correct. I've made the comment even smaller as this (for me) is a common enough thing to do.

[dberris]

lib/XRay/Trace.cpp

10–11 ↗

(On Diff #102995)

This technically isn't strictly true. While there is an implementation of the naive and FDR mode logging, those may not be written by the implementation in compiler-rt. Making it sound like there's an actual dependency (as opposed to a coincidental dependency) may be misleading.

For instance, it's possible to write logs in this format without using the compiler-rt implementation.

Working on documenting the log details/format as opposed to referring to the compiler-rt implementation would be a strictly better approach. I'd rather change this comment to say the explicit versions of the log we're supporting and not mentioning anything in compiler-rt.

[pelikan]

explain the complicated mess of a situation between LLVM and compiler-rt even
more clearly, to avoid any reader being confused

lib/XRay/Trace.cpp

10–11 ↗

(On Diff #102995)

May not be. But currently (and for the forseeable future) will be. Currently, a person will see two separate and *very* different implementations, and not know if they're supposed to be related or not just because the data format appears to coincide.

The point of this is obviously not documenting the format. It is supposed to tell the reader where an example of the counterpart can be found. I've updated it so it reflects the fact this doesn't have to be a hard dependency.

When/if there is a formal spec to the format, I'll be happy to reword this. (IIUC this isn't a priority for anyone yet, because there are only these two instances so far.)

[dberris]

lib/XRay/Trace.cpp

10–11 ↗

(On Diff #102995)

Or we can just remove this comment, and not need to refer to compiler-rt nor any other implementations at this point.

[pelikan]

remove comment per request

[pelikan]

lib/XRay/Trace.cpp

10–11 ↗

(On Diff #102995)

...and leave this confusing forever.

[dberris]

lib/XRay/Trace.cpp

10–11 ↗

(On Diff #102995)

No, not "forever".

If we're going to make this clearer, it shouldn't involve referring to the details of an implementation. The proper disambiguation to this would be, that we say what version of the log we're supporting, and a pointer to the documentation of the log format.

The feedback I gave you was: "Working on documenting the log details/format as opposed to referring to the compiler-rt implementation would be a strictly better approach. I'd rather change this comment to say the explicit versions of the log we're supporting and not mentioning anything in compiler-rt."

Nowhere did I imply that this should be intentionally confusing forever.

[pelikan]

Rebase to HEAD.

llvm/trunk/lib/XRay/Trace.cpp

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,
                          std::vector<XRayRecord> &Records) {
-  // Check that there is at least a header
   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.",
default:
     case static_cast<uint8_t>(RecordTypes::ENTER):
       Record.Type = RecordTypes::ENTER;
       break;
     case static_cast<uint8_t>(RecordTypes::EXIT):
     case 2: // TAIL_EXIT is not yet defined in RecordTypes.
       Record.Type = RecordTypes::EXIT;
       break;
     default:
-      // When initializing the error, convert to uint16_t so that the record
-      // type isn't interpreted as a char.
+      // 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));
     }
     Record.CPU = State.CPUId;
     Record.TId = State.ThreadId;
     // 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;
     // 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.
     uint32_t FuncIdBitField = RecordExtractor.getU32(&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.
-    uint64_t new_tsc = State.BaseTSC + RecordExtractor.getU32(&OffsetPtr);
-    State.BaseTSC = new_tsc;
-    Record.TSC = new_tsc;
+    uint64_t NewTSC = State.BaseTSC + RecordExtractor.getU32(&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
 /// a log consists of the familiar 32 bit XRayHeader, followed by sequences of
 /// of interspersed 16 byte Metadata Records and 8 byte Function Records.
 ///
 /// The following is an attempt to document the grammar of the format, which is
 /// parsed by this function for little-endian machines. Since the format makes
-/// use of BitFields, when we support big-Endian architectures, we will need to
+/// use of BitFields, when we support big-endian architectures, we will need to
 /// adjust not only the endianness parameter to llvm's RecordExtractor, but also
 /// 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*
Error loadFDRLog(StringRef Data, XRayFileHeader &FileHeader,
   // 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) {
+      if (S.size() < RecordSize) {
         return make_error<StringError>(
-            Twine("Incomplete thread buffer. Expected ") +
-                Twine(State.CurrentBufferSize - State.CurrentBufferConsumed) +
-                " remaining bytes but found " + Twine(S.size()),
+            Twine("Incomplete thread buffer. Expected at least ") +
+                Twine(RecordSize) + " 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,
                                             RecordSize))
         return E;
-      State.CurrentBufferConsumed += RecordSize;
     } else { // Process Function Record
       RecordSize = 8;
       if (auto E = processFDRFunctionRecord(State, BitField, RecordExtractor,
                                             Records))
         return E;
-      State.CurrentBufferConsumed += RecordSize;
     }
+    State.CurrentBufferConsumed += RecordSize;
   }
-  // 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))
+
+  // Having iterated over everything we've been given, we've either consumed
+  // everything and ended up in the end state, or were told to skip the rest.
+  bool Finished = State.Expects == FDRState::Token::SCAN_TO_END_OF_THREAD_BUF &&
+        State.CurrentBufferSize == State.CurrentBufferConsumed;
+  if (State.Expects != FDRState::Token::NEW_BUFFER_RECORD_OR_EOF && !Finished)
     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.
   YAMLXRayTrace Trace;
   Input In(Data);
   In >> Trace;
   if (In.error())
     return make_error<StringError>("Failed loading YAML Data.", In.error());
 
   FileHeader.Version = Trace.Header.Version;
   FileHeader.Type = Trace.Header.Type;

 
 Expected<Trace> llvm::xray::loadTraceFile(StringRef Filename, bool Sort) {
   int Fd;
   if (auto EC = sys::fs::openFileForRead(Filename, Fd)) {
     return make_error<StringError>(
         Twine("Cannot read log from '") + Filename + "'", EC);
   }
 
-  // Attempt to get the filesize.
   uint64_t FileSize;
   if (auto EC = sys::fs::file_size(Filename, FileSize)) {
     return make_error<StringError>(
         Twine("Cannot read log from '") + Filename + "'", EC);
   }
   if (FileSize < 4) {
     return make_error<StringError>(
         Twine("File '") + Filename + "' too small for XRay.",
         std::make_error_code(std::errc::executable_format_error));
   }
 
-  // Attempt to mmap the file.
+  // Map the opened file into memory and use a StringRef to access it later.
   std::error_code EC;
   sys::fs::mapped_file_region MappedFile(
       Fd, sys::fs::mapped_file_region::mapmode::readonly, FileSize, 0, EC);
   if (EC) {
     return make_error<StringError>(
         Twine("Cannot read log from '") + Filename + "'", EC);
   }
+  auto Data = StringRef(MappedFile.data(), MappedFile.size());
 
   // Attempt to detect the file type using file magic. We have a slight bias
   // towards the binary format, and we do this by making sure that the first 4
   // bytes of the binary file is some combination of the following byte
-  // patterns:
+  // patterns: (observe the code loading them assumes they're little endian)
   //
-  //   0x0001 0x0000 - version 1, "naive" format
-  //   0x0001 0x0001 - version 1, "flight data recorder" format
+  //   0x01 0x00 0x00 0x00 - version 1, "naive" format
+  //   0x01 0x00 0x01 0x00 - version 1, "flight data recorder" format
   //
-  // YAML files dont' typically have those first four bytes as valid text so we
+  // YAML files don't typically have those first four bytes as valid text so we
   // try loading assuming YAML if we don't find these bytes.
   //
   // Only if we can't load either the binary or the YAML format will we yield an
   // error.
   StringRef Magic(MappedFile.data(), 4);
   DataExtractor HeaderExtractor(Magic, true, 8);
   uint32_t OffsetPtr = 0;
   uint16_t Version = HeaderExtractor.getU16(&OffsetPtr);
   uint16_t Type = HeaderExtractor.getU16(&OffsetPtr);
 
   enum BinaryFormatType { NAIVE_FORMAT = 0, FLIGHT_DATA_RECORDER_FORMAT = 1 };
 
   Trace T;
   if (Version == 1 && Type == NAIVE_FORMAT) {
     if (auto E =
-            loadNaiveFormatLog(StringRef(MappedFile.data(), MappedFile.size()),
-                               T.FileHeader, T.Records))
+            loadNaiveFormatLog(Data, T.FileHeader, T.Records))
       return std::move(E);
   } else if (Version == 1 && Type == FLIGHT_DATA_RECORDER_FORMAT) {
-    if (auto E = loadFDRLog(StringRef(MappedFile.data(), MappedFile.size()),
-                            T.FileHeader, T.Records))
+    if (auto E = loadFDRLog(Data, T.FileHeader, T.Records))
       return std::move(E);
   } else {
-    if (auto E = loadYAMLLog(StringRef(MappedFile.data(), MappedFile.size()),
-                             T.FileHeader, T.Records))
+    if (auto E = loadYAMLLog(Data, T.FileHeader, T.Records))
       return std::move(E);
   }
 
   if (Sort)
     std::sort(T.Records.begin(), T.Records.end(),
               [&](const XRayRecord &L, const XRayRecord &R) {
                 return L.TSC < R.TSC;
               });
 
   return std::move(T);
 }