From 5e4931b4893b5d3fe1da777d548cbfe9f68ef433 Mon Sep 17 00:00:00 2001 From: Justin Bogner Date: Mon, 16 Mar 2015 06:55:45 +0000 Subject: [PATCH] InstrProf: Do a better job of reading coverage mapping data. This code was casting regions of a memory buffer to a couple of different structs. This is wrong in a few ways: 1. It breaks aliasing rules. 2. If the buffer isn't aligned, it hits undefined behaviour. 3. It completely ignores endianness differences. 4. The structs being defined for this aren't specifying their padding properly, so this doesn't even represent the data properly on some platforms. This commit is mostly NFC, except that it fixes reading coverage for 32 bit binaries as a side effect of getting rid of the mispadded structs. I've included a test for that. I've also baked in that we only handle little endian more explicitly, since that was true in practice already. I'll fix this to handle endianness properly in a followup commit. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@232346 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Support/Endian.h | 5 +- lib/ProfileData/CoverageMappingReader.cpp | 100 +++++++----------- .../llvm-cov/Inputs/binary-formats.macho32l | Bin 0 -> 812 bytes .../llvm-cov/Inputs/binary-formats.macho64l | Bin 0 -> 920 bytes .../llvm-cov/Inputs/binary-formats.proftext | 4 + test/tools/llvm-cov/binary-formats.c | 11 ++ test/tools/llvm-cov/lit.local.cfg | 4 - 7 files changed, 59 insertions(+), 65 deletions(-) create mode 100755 test/tools/llvm-cov/Inputs/binary-formats.macho32l create mode 100755 test/tools/llvm-cov/Inputs/binary-formats.macho64l create mode 100644 test/tools/llvm-cov/Inputs/binary-formats.proftext create mode 100644 test/tools/llvm-cov/binary-formats.c diff --git a/include/llvm/Support/Endian.h b/include/llvm/Support/Endian.h index 17ae651b6b4..e9fe22e5eda 100644 --- a/include/llvm/Support/Endian.h +++ b/include/llvm/Support/Endian.h @@ -58,8 +58,9 @@ inline value_type read(const void *memory) { /// Read a value of a particular endianness from a buffer, and increment the /// buffer past that value. -template -inline value_type readNext(const unsigned char *&memory) { +template +inline value_type readNext(const CharT *&memory) { value_type ret = read(memory); memory += sizeof(value_type); return ret; diff --git a/lib/ProfileData/CoverageMappingReader.cpp b/lib/ProfileData/CoverageMappingReader.cpp index 3f8f76f6094..fde6874fe40 100644 --- a/lib/ProfileData/CoverageMappingReader.cpp +++ b/lib/ProfileData/CoverageMappingReader.cpp @@ -17,6 +17,7 @@ #include "llvm/Object/MachOUniversal.h" #include "llvm/Object/ObjectFile.h" #include "llvm/Support/Debug.h" +#include "llvm/Support/Endian.h" #include "llvm/Support/LEB128.h" using namespace llvm; @@ -288,24 +289,6 @@ std::error_code RawCoverageMappingReader::read() { } namespace { -/// \brief The coverage mapping data for a single function. -/// It points to the function's name. -template struct CoverageMappingFunctionRecord { - IntPtrT FunctionNamePtr; - uint32_t FunctionNameSize; - uint32_t CoverageMappingSize; - uint64_t FunctionHash; -}; - -/// \brief The coverage mapping data for a single translation unit. -/// It points to the array of function coverage mapping records and the encoded -/// filenames array. -template struct CoverageMappingTURecord { - uint32_t FunctionRecordsSize; - uint32_t FilenamesSize; - uint32_t CoverageMappingsSize; - uint32_t Version; -}; /// \brief A helper structure to access the data from a section /// in an object file. @@ -337,72 +320,71 @@ std::error_code readCoverageMappingData( SectionData &ProfileNames, StringRef Data, std::vector &Records, std::vector &Filenames) { + using namespace support; llvm::DenseSet UniqueFunctionMappingData; // Read the records in the coverage data section. - while (!Data.empty()) { - if (Data.size() < sizeof(CoverageMappingTURecord)) + for (const char *Buf = Data.data(), *End = Buf + Data.size(); Buf < End;) { + if (Buf + 4 * sizeof(uint32_t) > End) return instrprof_error::malformed; - auto TU = reinterpret_cast *>(Data.data()); - Data = Data.substr(sizeof(CoverageMappingTURecord)); - switch (TU->Version) { + uint32_t NRecords = endian::readNext(Buf); + uint32_t FilenamesSize = endian::readNext(Buf); + uint32_t CoverageSize = endian::readNext(Buf); + uint32_t Version = endian::readNext(Buf); + + switch (Version) { case CoverageMappingVersion1: break; default: return instrprof_error::unsupported_version; } - auto Version = CoverageMappingVersion(TU->Version); - // Get the function records. - auto FunctionRecords = - reinterpret_cast *>(Data.data()); - if (Data.size() < - sizeof(CoverageMappingFunctionRecord) * TU->FunctionRecordsSize) - return instrprof_error::malformed; - Data = Data.substr(sizeof(CoverageMappingFunctionRecord) * - TU->FunctionRecordsSize); + // Skip past the function records, saving the start and end for later. + const char *FunBuf = Buf; + Buf += NRecords * (sizeof(T) + 2 * sizeof(uint32_t) + sizeof(uint64_t)); + const char *FunEnd = Buf; // Get the filenames. - if (Data.size() < TU->FilenamesSize) + if (Buf + FilenamesSize > End) return instrprof_error::malformed; - auto RawFilenames = Data.substr(0, TU->FilenamesSize); - Data = Data.substr(TU->FilenamesSize); size_t FilenamesBegin = Filenames.size(); - RawCoverageFilenamesReader Reader(RawFilenames, Filenames); + RawCoverageFilenamesReader Reader(StringRef(Buf, FilenamesSize), Filenames); if (auto Err = Reader.read()) return Err; + Buf += FilenamesSize; - // Get the coverage mappings. - if (Data.size() < TU->CoverageMappingsSize) + // We'll read the coverage mapping records in the loop below. + const char *CovBuf = Buf; + Buf += CoverageSize; + const char *CovEnd = Buf; + if (Buf > End) return instrprof_error::malformed; - auto CoverageMappings = Data.substr(0, TU->CoverageMappingsSize); - Data = Data.substr(TU->CoverageMappingsSize); - for (unsigned I = 0; I < TU->FunctionRecordsSize; ++I) { - auto &MappingRecord = FunctionRecords[I]; + while (FunBuf < FunEnd) { + // Read the function information + T NamePtr = endian::readNext(FunBuf); + uint32_t NameSize = endian::readNext(FunBuf); + uint32_t DataSize = endian::readNext(FunBuf); + uint64_t FuncHash = endian::readNext(FunBuf); - // Get the coverage mapping. - if (CoverageMappings.size() < MappingRecord.CoverageMappingSize) + // Now use that to read the coverage data. + if (CovBuf + DataSize > CovEnd) return instrprof_error::malformed; - auto Mapping = - CoverageMappings.substr(0, MappingRecord.CoverageMappingSize); - CoverageMappings = - CoverageMappings.substr(MappingRecord.CoverageMappingSize); + auto Mapping = StringRef(CovBuf, DataSize); + CovBuf += DataSize; // Ignore this record if we already have a record that points to the same - // function name. - // This is useful to ignore the redundant records for the functions - // with ODR linkage. - if (!UniqueFunctionMappingData.insert(MappingRecord.FunctionNamePtr) - .second) + // function name. This is useful to ignore the redundant records for the + // functions with ODR linkage. + if (!UniqueFunctionMappingData.insert(NamePtr).second) continue; - StringRef FunctionName; - if (auto Err = - ProfileNames.get(MappingRecord.FunctionNamePtr, - MappingRecord.FunctionNameSize, FunctionName)) - return Err; + + // Finally, grab the name and create a record. + StringRef FuncName; + if (std::error_code EC = ProfileNames.get(NamePtr, NameSize, FuncName)) + return EC; Records.push_back(BinaryCoverageReader::ProfileMappingRecord( - Version, FunctionName, MappingRecord.FunctionHash, Mapping, + CoverageMappingVersion(Version), FuncName, FuncHash, Mapping, FilenamesBegin, Filenames.size() - FilenamesBegin)); } } diff --git a/test/tools/llvm-cov/Inputs/binary-formats.macho32l b/test/tools/llvm-cov/Inputs/binary-formats.macho32l new file mode 100755 index 0000000000000000000000000000000000000000..2dd4c44866b033b2426f8c614760917409542586 GIT binary patch literal 812 zcmX^2>+L^w1_lOZAZ7z%ULc;r#K6$XAi%%~WY++3e0+#&L1PFE zka`e^k1t8BD1opcd{q6&dJ(FCEEa|a7$0QT0U(Z#cX14Hgo#4z^8j)|W`Jmz+o8?@ zs?W(O%Z*RYFUw6Vz@gs-svk&U1yC~ziqhiq5_40F(cK^d3@jF)P7sX?fZT5Z#PRVy zo_^l0E}qcvlK|?G0E&ZX5g-k6KP=pUOmO%s0BMjKWbs73{8CVmDlsrL7yxNv0VrHR zX2F04)a~3rfdH5w6=-Ro86bOH=01EGdDGCn(Jwjn%K2xRDnKy-kojC(V5|k>gT#P9 z4TwSVpzr{NsV0;!1LT8%HV}j4L3|#N0096BGjSm1gsOwZ6^suOW)#&g$t}=N%FIhF os?<%(FUn0UDb`B{YGq_((O}TZP0Y*#srz-02b8SA)+zuQ0KS?;)c^nh literal 0 HcmV?d00001 diff --git a/test/tools/llvm-cov/Inputs/binary-formats.macho64l b/test/tools/llvm-cov/Inputs/binary-formats.macho64l new file mode 100755 index 0000000000000000000000000000000000000000..0045c435575019e24473a4295c380a9aa9021366 GIT binary patch literal 920 zcmbtTO-lk{5T32=%M83kMG#Arn2~mkx(EtMA9N7v_O^+F;Oe5A7 z=$e=QfQM+FZH8>c2pV{p_nD7p=be{*8N7eY5LuH%G|gCG^e2e!D8VP96~+_8m{Q@Q zgvd`zp>fERvnEY3EXf%YRJy@;VN_51v?9vMrk>{DF;|~qOg>l2A@XI~cQ%G&E{x1q z49%E&z2lj3v*WowN5<(LO1@1e>Ln6T569x&n%O9hVEW-|M9Eb zjPG2YB-h?H4<7EH&kK9|-P;Y0B_JG&g-9z$j4jDP9(%!DTf%#?x9uSw>M;M!u(Kt` zoD}K1;T<)Gl(m=*JU@F?Yq+hOooch?xj{Qq=61`nQ