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.

llvm-svn: 232346

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<typename value_type, endianness endian, std::size_t alignment>
+template<typename value_type, endianness endian, std::size_t alignment,
-inline value_type readNext(const unsigned char *&memory) {
+         typename CharT>
+inline value_type readNext(const CharT *&memory) {
   value_type ret = read<value_type, endian, alignment>(memory);
   memory += sizeof(value_type);
   return ret;

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 <typename IntPtrT> 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 <typename IntPtrT> 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<BinaryCoverageReader::ProfileMappingRecord> &Records,
     std::vector<StringRef> &Filenames) {
+  using namespace support;
   llvm::DenseSet<T> UniqueFunctionMappingData;
 
   // Read the records in the coverage data section.
-  while (!Data.empty()) {
+  for (const char *Buf = Data.data(), *End = Buf + Data.size(); Buf < End;) {
-    if (Data.size() < sizeof(CoverageMappingTURecord<T>))
+    if (Buf + 4 * sizeof(uint32_t) > End)
       return instrprof_error::malformed;
-    auto TU = reinterpret_cast<const CoverageMappingTURecord<T> *>(Data.data());
+    uint32_t NRecords = endian::readNext<uint32_t, little, unaligned>(Buf);
-    Data = Data.substr(sizeof(CoverageMappingTURecord<T>));
+    uint32_t FilenamesSize = endian::readNext<uint32_t, little, unaligned>(Buf);
-    switch (TU->Version) {
+    uint32_t CoverageSize = endian::readNext<uint32_t, little, unaligned>(Buf);
+    uint32_t Version = endian::readNext<uint32_t, little, unaligned>(Buf);
+
+    switch (Version) {
     case CoverageMappingVersion1:
       break;
     default:
       return instrprof_error::unsupported_version;
     }
-    auto Version = CoverageMappingVersion(TU->Version);
 
-    // Get the function records.
+    // Skip past the function records, saving the start and end for later.
-    auto FunctionRecords =
+    const char *FunBuf = Buf;
-        reinterpret_cast<const CoverageMappingFunctionRecord<T> *>(Data.data());
+    Buf += NRecords * (sizeof(T) + 2 * sizeof(uint32_t) + sizeof(uint64_t));
-    if (Data.size() <
+    const char *FunEnd = Buf;
-        sizeof(CoverageMappingFunctionRecord<T>) * TU->FunctionRecordsSize)
-      return instrprof_error::malformed;
-    Data = Data.substr(sizeof(CoverageMappingFunctionRecord<T>) *
-                       TU->FunctionRecordsSize);
 
     // 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.
+    // We'll read the coverage mapping records in the loop below.
-    if (Data.size() < TU->CoverageMappingsSize)
+    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) {
+    while (FunBuf < FunEnd) {
-      auto &MappingRecord = FunctionRecords[I];
+      // Read the function information
+      T NamePtr = endian::readNext<T, little, unaligned>(FunBuf);
+      uint32_t NameSize = endian::readNext<uint32_t, little, unaligned>(FunBuf);
+      uint32_t DataSize = endian::readNext<uint32_t, little, unaligned>(FunBuf);
+      uint64_t FuncHash = endian::readNext<uint64_t, little, unaligned>(FunBuf);
 
-      // Get the coverage mapping.
+      // Now use that to read the coverage data.
-      if (CoverageMappings.size() < MappingRecord.CoverageMappingSize)
+      if (CovBuf + DataSize > CovEnd)
         return instrprof_error::malformed;
-      auto Mapping =
+      auto Mapping = StringRef(CovBuf, DataSize);
-          CoverageMappings.substr(0, MappingRecord.CoverageMappingSize);
+      CovBuf += DataSize;
-      CoverageMappings =
-          CoverageMappings.substr(MappingRecord.CoverageMappingSize);
 
       // Ignore this record if we already have a record that points to the same
-      // function name.
+      // function name. This is useful to ignore the redundant records for the
-      // This is useful to ignore the redundant records for the functions
+      // functions with ODR linkage.
-      // with ODR linkage.
+      if (!UniqueFunctionMappingData.insert(NamePtr).second)
-      if (!UniqueFunctionMappingData.insert(MappingRecord.FunctionNamePtr)
-               .second)
         continue;
-      StringRef FunctionName;
+
-      if (auto Err =
+      // Finally, grab the name and create a record.
-              ProfileNames.get(MappingRecord.FunctionNamePtr,
+      StringRef FuncName;
-                               MappingRecord.FunctionNameSize, FunctionName))
+      if (std::error_code EC = ProfileNames.get(NamePtr, NameSize, FuncName))
-        return Err;
+        return EC;
       Records.push_back(BinaryCoverageReader::ProfileMappingRecord(
-          Version, FunctionName, MappingRecord.FunctionHash, Mapping,
+          CoverageMappingVersion(Version), FuncName, FuncHash, Mapping,
           FilenamesBegin, Filenames.size() - FilenamesBegin));
     }
   }

test/tools/llvm-cov/Inputs/binary-formats.proftext

@@ -0,0 +1,4 @@
+main
+0x0
+1
+100

test/tools/llvm-cov/binary-formats.c

@@ -0,0 +1,11 @@
+// Checks for reading various formats.
+
+// CHECK: 100| [[@LINE+1]]|int main
+int main(int argc, const char *argv[]) {}
+
+// RUN: llvm-profdata merge %S/Inputs/binary-formats.proftext -o %t.profdata
+// RUN: llvm-cov show %S/Inputs/binary-formats.macho32l -instr-profile %t.profdata -no-colors -filename-equivalence %s | FileCheck %s
+// RUN: llvm-cov show %S/Inputs/binary-formats.macho64l -instr-profile %t.profdata -no-colors -filename-equivalence %s | FileCheck %s
+
+// llvm-cov doesn't work on big endian yet
+// XFAIL: powerpc64-, s390x, mips-, mips64-, sparc

test/tools/llvm-cov/lit.local.cfg

@@ -1,5 +1 @@
 config.suffixes = ['.test', '.m', '.cpp', '.c']
-
-# http://llvm.org/bugs/show_bug.cgi?id=20979
-if 'ubsan' in config.available_features:
-  config.unsupported = True