[Coverage] Fix an issue where improper coverage mapping data could be loaded for an inline function.

If an inline function is observed but unused in a translation unit, dummy
coverage mapping data with zero hash is stored for this function.
If such a coverage mapping section came earlier than real one, the latter
was ignored. As a result, llvm-cov was unable to show coverage information
for those functions.

Differential Revision: http://reviews.llvm.org/D20286


git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@270194 91177308-0d34-0410-b5e6-96231b3b80d8

include/llvm/ProfileData/Coverage/CoverageMappingReader.h

@@ -103,6 +103,16 @@ public:
   Error read();
 };
 
+/// \brief Checks if the given coverage mapping data is exported for
+/// an unused function.
+class RawCoverageMappingDummyChecker : public RawCoverageReader {
+public:
+  RawCoverageMappingDummyChecker(StringRef MappingData)
+      : RawCoverageReader(MappingData) {}
+
+  Expected<bool> isDummy();
+};
+
 /// \brief Reader for the raw coverage mapping data.
 class RawCoverageMappingReader : public RawCoverageReader {
   ArrayRef<StringRef> TranslationUnitFilenames;

lib/ProfileData/Coverage/CoverageMappingReader.cpp

@@ -13,7 +13,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ProfileData/Coverage/CoverageMappingReader.h"
-#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Debug.h"
@@ -294,6 +294,36 @@ Error RawCoverageMappingReader::read() {
   return Error::success();
 }
 
+Expected<bool> RawCoverageMappingDummyChecker::isDummy() {
+  // A dummy coverage mapping data consists of just one region with zero count.
+  uint64_t NumFileMappings;
+  if (Error Err = readSize(NumFileMappings))
+    return std::move(Err);
+  if (NumFileMappings != 1)
+    return false;
+  // We don't expect any specific value for the filename index, just skip it.
+  uint64_t FilenameIndex;
+  if (Error Err =
+          readIntMax(FilenameIndex, std::numeric_limits<unsigned>::max()))
+    return std::move(Err);
+  uint64_t NumExpressions;
+  if (Error Err = readSize(NumExpressions))
+    return std::move(Err);
+  if (NumExpressions != 0)
+    return false;
+  uint64_t NumRegions;
+  if (Error Err = readSize(NumRegions))
+    return std::move(Err);
+  if (NumRegions != 1)
+    return false;
+  uint64_t EncodedCounterAndRegion;
+  if (Error Err = readIntMax(EncodedCounterAndRegion,
+                             std::numeric_limits<unsigned>::max()))
+    return std::move(Err);
+  unsigned Tag = EncodedCounterAndRegion & Counter::EncodingTagMask;
+  return Tag == Counter::Zero;
+}
+
 Error InstrProfSymtab::create(SectionRef &Section) {
   if (auto EC = Section.getContents(Data))
     return errorCodeToError(EC);
@@ -310,6 +340,14 @@ StringRef InstrProfSymtab::getFuncName(uint64_t Pointer, size_t Size) {
   return Data.substr(Pointer - Address, Size);
 }
 
+// Check if the mapping data is a dummy, i.e. is emitted for an unused function.
+static Expected<bool> isCoverageMappingDummy(uint64_t Hash, StringRef Mapping) {
+  // The hash value of dummy mapping records is always zero.
+  if (Hash)
+    return false;
+  return RawCoverageMappingDummyChecker(Mapping).isDummy();
+}
+
 namespace {
 struct CovMapFuncRecordReader {
   // The interface to read coverage mapping function records for
@@ -334,11 +372,56 @@ class VersionedCovMapFuncRecordReader : public CovMapFuncRecordReader {
   typedef typename coverage::CovMapTraits<Version, IntPtrT>::NameRefType
       NameRefType;
 
-  llvm::DenseSet<NameRefType> UniqueFunctionMappingData;
+  // Maps function's name references to the indexes of their records
+  // in \c Records.
+  llvm::DenseMap<NameRefType, size_t> FunctionRecords;
   InstrProfSymtab &ProfileNames;
   std::vector<StringRef> &Filenames;
   std::vector<BinaryCoverageReader::ProfileMappingRecord> &Records;
 
+  // Add the record to the collection if we don't 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.
+  // In addition, prefer records with real coverage mapping data to dummy
+  // records, which were emitted for inline functions which were seen but
+  // not used in the corresponding translation unit.
+  Error insertFunctionRecordIfNeeded(const FuncRecordType *CFR,
+                                     StringRef Mapping, size_t FilenamesBegin) {
+    uint64_t FuncHash = CFR->template getFuncHash<Endian>();
+    NameRefType NameRef = CFR->template getFuncNameRef<Endian>();
+    auto InsertResult =
+        FunctionRecords.insert(std::make_pair(NameRef, Records.size()));
+    if (InsertResult.second) {
+      StringRef FuncName;
+      if (Error Err = CFR->template getFuncName<Endian>(ProfileNames, FuncName))
+        return Err;
+      Records.emplace_back(Version, FuncName, FuncHash, Mapping, FilenamesBegin,
+                           Filenames.size() - FilenamesBegin);
+      return Error::success();
+    }
+    // Update the existing record if it's a dummy and the new record is real.
+    size_t OldRecordIndex = InsertResult.first->second;
+    BinaryCoverageReader::ProfileMappingRecord &OldRecord =
+        Records[OldRecordIndex];
+    Expected<bool> OldIsDummyExpected = isCoverageMappingDummy(
+        OldRecord.FunctionHash, OldRecord.CoverageMapping);
+    if (Error Err = OldIsDummyExpected.takeError())
+      return Err;
+    if (!*OldIsDummyExpected)
+      return Error::success();
+    Expected<bool> NewIsDummyExpected =
+        isCoverageMappingDummy(FuncHash, Mapping);
+    if (Error Err = NewIsDummyExpected.takeError())
+      return Err;
+    if (*NewIsDummyExpected)
+      return Error::success();
+    OldRecord.FunctionHash = FuncHash;
+    OldRecord.CoverageMapping = Mapping;
+    OldRecord.FilenamesBegin = FilenamesBegin;
+    OldRecord.FilenamesSize = Filenames.size() - FilenamesBegin;
+    return Error::success();
+  }
+
 public:
   VersionedCovMapFuncRecordReader(
       InstrProfSymtab &P,
@@ -387,7 +470,6 @@ public:
     while ((const char *)CFR < FunEnd) {
       // Read the function information
       uint32_t DataSize = CFR->template getDataSize<Endian>();
-      uint64_t FuncHash = CFR->template getFuncHash<Endian>();
 
       // Now use that to read the coverage data.
       if (CovBuf + DataSize > CovEnd)
@@ -395,21 +477,9 @@ public:
       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.
-      NameRefType NameRef = CFR->template getFuncNameRef<Endian>();
-      if (!UniqueFunctionMappingData.insert(NameRef).second) {
-        CFR++;
-        continue;
-      }
-
-      StringRef FuncName;
-      if (Error E = CFR->template getFuncName<Endian>(ProfileNames, FuncName))
-        return E;
-      Records.push_back(BinaryCoverageReader::ProfileMappingRecord(
-          Version, FuncName, FuncHash, Mapping, FilenamesBegin,
-          Filenames.size() - FilenamesBegin));
+      if (Error Err =
+              insertFunctionRecordIfNeeded(CFR, Mapping, FilenamesBegin))
+        return Err;
       CFR++;
     }
     return Error::success();

test/tools/llvm-cov/Inputs/prefer_used_to_unused.cpp

@@ -0,0 +1,5 @@
+#include "prefer_used_to_unused.h"
+
+int main() {
+  return sampleFunc(5) + simpleFunc(5);
+}

test/tools/llvm-cov/Inputs/prefer_used_to_unused.proftext

@@ -0,0 +1,25 @@
+_Z10sampleFunci
+# Func Hash:
+10
+# Num Counters:
+2
+# Counter Values:
+1
+1
+
+main
+# Func Hash:
+0
+# Num Counters:
+1
+# Counter Values:
+1
+
+_Z10simpleFunci
+# Func Hash:
+0
+# Num Counters:
+1
+# Counter Values:
+1
+

test/tools/llvm-cov/prefer_used_to_unused.h

@@ -0,0 +1,24 @@
+// Check that llvm-cov loads real coverage mapping data for a function
+// even though dummy coverage data for that function comes first.
+// Dummy coverage data is exported if the definition of an inline function
+// is seen but the function is not used in the translation unit.
+
+// If you need to rebuild the 'covmapping' file for this test, please use
+// the following commands:
+// clang++ -fprofile-instr-generate -fcoverage-mapping -o tmp -x c++ prefer_used_to_unused.h prefer_used_to_unused.cpp
+// llvm-cov convert-for-testing -o prefer_used_to_unused.covmapping tmp
+
+// RUN: llvm-profdata merge %S/Inputs/prefer_used_to_unused.proftext -o %t.profdata
+// RUN: llvm-cov show %S/Inputs/prefer_used_to_unused.covmapping -instr-profile %t.profdata -filename-equivalence %s | FileCheck %s
+
+// Coverage data for this function has a non-zero hash value if it is used in the translation unit.
+inline int sampleFunc(int A) { // CHECK:      1| [[@LINE]]|inline int sampleFunc(int A) {
+  if (A > 0)                   // CHECK-NEXT: 1| [[@LINE]]|  if (A > 0)
+    return A;                  // CHECK-NEXT: 1| [[@LINE]]|    return A;
+  return 0;                    // CHECK-NEXT: 0| [[@LINE]]|  return 0;
+}                              // CHECK-NEXT: 1| [[@LINE]]|}
+
+// The hash for this function is zero in both cases, either it is used in the translation unit or not.
+inline int simpleFunc(int A) { // CHECK:      1| [[@LINE]]|inline int simpleFunc(int A) {
+  return A;                    // CHECK-NEXT: 1| [[@LINE]]|  return A;
+}                              // CHECK-NEXT: 1| [[@LINE]]|}