Index: lib/XRay/Trace.cpp =================================================================== --- lib/XRay/Trace.cpp +++ lib/XRay/Trace.cpp @@ -7,7 +7,8 @@ // //===----------------------------------------------------------------------===// // -// XRay log reader implementation. +// XRay log reader implementation. Decodes logs written using compiler-rt's +// naive and Flight Data Recorder XRay logging formats. // //===----------------------------------------------------------------------===// #include "llvm/XRay/Trace.h" @@ -56,10 +57,9 @@ Error loadNaiveFormatLog(StringRef Data, XRayFileHeader &FileHeader, std::vector &Records) { - // Check that there is at least a header if (Data.size() < 32) return make_error( - "Not enough bytes for an XRay log.", + "Not enough bytes for an XRay log header.", std::make_error_code(std::errc::invalid_argument)); if (Data.size() - 32 == 0 || Data.size() % 32 != 0) @@ -222,8 +222,8 @@ DataExtractor &RecordExtractor, size_t &RecordSize) { // We can encounter a CustomEventMarker anywhere in the log, so we can handle - // it regardless of the expectation. However, we do se the expectation to read - // a set number of fixed bytes, as described in the metadata. + // it regardless of the expectation. However, we do see the expectation to + // read a set number of fixed bytes, as described in the metadata. uint32_t OffsetPtr = 1; // Read after the first byte. uint32_t DataSize = RecordExtractor.getU32(&OffsetPtr); uint64_t TSC = RecordExtractor.getU64(&OffsetPtr); @@ -324,7 +324,7 @@ Record.Type = RecordTypes::EXIT; break; default: - // When initializing the error, convert to uint16_t so that the record + // When initializing the error, convert to an integer so that the record // type isn't interpreted as a char. return make_error( Twine("Illegal function record type: ") @@ -346,9 +346,9 @@ // 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(); } @@ -360,7 +360,7 @@ /// /// 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 @@ -414,11 +414,10 @@ 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( - 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; @@ -432,19 +431,19 @@ 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( Twine("Encountered EOF with unexpected state expectation ") + fdrStateToTwine(State.Expects) + @@ -457,7 +456,6 @@ Error loadYAMLLog(StringRef Data, XRayFileHeader &FileHeader, std::vector &Records) { - // Load the documents from the MappedFile. YAMLXRayTrace Trace; Input In(Data); In >> Trace; @@ -491,7 +489,6 @@ 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( @@ -503,7 +500,7 @@ 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); @@ -511,16 +508,17 @@ return make_error( 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 @@ -536,16 +534,13 @@ 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); }