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

[ikudrin]

Summary

If an inline function is observed but unused in a translation unit, a dummy coverage mapping data with a 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.

[ikudrin]

The motivation sample for this patch is the following:

$ cat > sample.h << EOF
inline int sample_func(int A) {
  if (A > 0)
    return A;
  return 0;
}
EOF
$ cat > dummy.cpp << EOF
#include "sample.h"
EOF
$ cat > sample.cpp << EOF
#include "sample.h"

int main() {
  return sample_func(5);
}
EOF
$ clang++ -fprofile-instr-generate -fcoverage-mapping dummy.cpp sample.cpp
$ ./a.out
$ llvm-profdata merge -o default.profdata default.profraw
$ llvm-cov show a.out -instr-profile default.profdata
warning: 1 functions have mismatched data. 
sample.cpp:
       |    1|#include "sample.h"
       |    2|
      1|    3|int main() {
      1|    4|  return sample_func(5);
      1|    5|}

After applying the patch, the output is the following:

$ llvm-cov show a.out -instr-profile default.profdata
sample.h:
      1|    1|inline int sample_func(int A) {
      1|    2|  if (A > 0)
      1|    3|    return A;
      0|    4|  return 0;
      1|    5|}

sample.cpp:
       |    1|#include "sample.h"
       |    2|
      1|    3|int main() {
      1|    4|  return sample_func(5);
      1|    5|}

[vsk]

Thanks, just two comments.

lib/ProfileData/Coverage/CoverageMappingReader.cpp

487

I'd find this a bit easier to read if InsertResult.first->second had its own variable.

487

If !InsertResult.second && FuncHash, wdyt of creating an error if Records[InsertResult.first->second].FunctionHash != FuncHash?

[ikudrin]

lib/ProfileData/Coverage/CoverageMappingReader.cpp

487

It makes sense, but I think it would be better to make that change in another patch.

[vsk]

This lgtm with the nit I had above, then. Please wait a bit in case there are other comments.

[davidxl]

It is possible to handle this without requiring changing the hash value.

What is needed is a helper method to check if the coverageMapping record is 'dummy' -- i.e. has any execution count. It not, it can be skipped or replaced.

See CoverageMapping::load method:

ErrorOr<int64_t> ExecutionCount = Ctx.evaluate(Region.Count);
 if (!ExecutionCount)
   break;

[ikudrin]

• Added the class to check if the coverage mapping data is dummy or real.
• New implementations also works for simple functions with zero hash, so D20287 is not needed anymore.
• The test case was extended to check simple inline functions in addition to regular ones.

[davidxl]

lib/ProfileData/Coverage/CoverageMappingReader.cpp

489

I think it is better to move duplicate resolution code above here (with a helper function) to improve readabiltity. The helper function can be something like:

InsertFunctionRecordIfNeeded (...)
which does all the work of duplicate detection and existing entry update

{

 ..
 AlreadySeen = ..
 if (!AlreadySeen) {
     Records.push_back(..);
     return;
 }
 IsOldDummy = ..
 IsNewDummy = ..
 if (IsNewDummy || !IsOldDummy) 
    return;
// Now replace old entry
Records[OldIndex] = NewRecord;

}

491

It is better to have another wrapper function to check dumminess which also takes FuncHash. The code can be simplified to

If (NewIsDummy || !OldIsDummy)

continue

[ikudrin]

• Rebased to the top
• Extracted the code of filling the mapping records collection, including duplicate resolution logic, into a separate function.

[vsk]

I just have nitpicks. Still looks good :).

lib/ProfileData/Coverage/CoverageMappingReader.cpp

297

nit, could you use Expected<bool> here? That should let you simplify this a bit.

347

Same Expected<bool> nit.

408

nit, existing

[davidxl]

lib/ProfileData/Coverage/CoverageMappingReader.cpp

351

Should this be moved above the if(Hash) so that caller does not need to initialize it.

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

5

do you need another source file that includes prefer_used_to_unused.h but does not call sampleFunc nor simpleFunc in order to force creation of dummies?

[ikudrin]

lib/ProfileData/Coverage/CoverageMappingReader.cpp

297

Thanks! I'll update the patch tomorrow.

351

The caller doesn't need to initialize it. If Hash is 0, RawCoverageMappingDummyChecker::isDummy() does its part of work.

408

Thanks!

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

5

I use prefer_used_to_unused.h itself for this, see the comment at line 6 there.

[davidxl]

lgtm

include/llvm/ProfileData/Coverage/CoverageMappingReader.h

 
 public:
   RawCoverageFilenamesReader(StringRef Data, std::vector<StringRef> &Filenames)
       : RawCoverageReader(Data), Filenames(Filenames) {}
 
   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) {}
+
+  Error isDummy(bool &Result);
+};
+
 /// \brief Reader for the raw coverage mapping data.
 class RawCoverageMappingReader : public RawCoverageReader {
   ArrayRef<StringRef> TranslationUnitFilenames;
   std::vector<StringRef> &Filenames;
   std::vector<CounterExpression> &Expressions;
   std::vector<CounterMappingRegion> &MappingRegions;
 
   RawCoverageMappingReader(const RawCoverageMappingReader &) = delete;

lib/ProfileData/Coverage/CoverageMappingReader.cpp

 //=-- CoverageMappingReader.cpp - Code coverage mapping reader ----*- C++ -*-=//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains support for reading coverage mapping data for
 // instrumentation based coverage.
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
 
for (auto &R : MappingRegions) {
         FileIDExpansionRegionMapping[R.FileID] = nullptr;
       }
     }
   }
 
   return Error::success();
 }
 
+Error RawCoverageMappingDummyChecker::isDummy(bool &Result) {
+  // A dummy coverage mapping data consists of just one region with zero count.
+  Result = false;
+  uint64_t NumFileMappings;
+  if (auto Err = readSize(NumFileMappings))
+    return Err;
+  if (NumFileMappings != 1)
+    return Error::success();
+  // We don't expect any specific value for the filename index, just skip it.
+  uint64_t FilenameIndex;
+  if (auto Err =
+          readIntMax(FilenameIndex, std::numeric_limits<unsigned>::max()))
+    return Err;
+  uint64_t NumExpressions;
+  if (auto Err = readSize(NumExpressions))
+    return Err;
+  if (NumExpressions != 0)
+    return Error::success();
+  uint64_t NumRegions;
+  if (auto Err = readSize(NumRegions))
+    return Err;
+  if (NumRegions != 1)
+    return Error::success();
+  uint64_t EncodedCounterAndRegion;
+  if (auto Err = readIntMax(EncodedCounterAndRegion,
+                            std::numeric_limits<unsigned>::max()))
+    return Err;
+  unsigned Tag = EncodedCounterAndRegion & Counter::EncodingTagMask;
+  if (Tag == Counter::Zero)
+    Result = true;
+  return Error::success();
+}
+
 Error InstrProfSymtab::create(SectionRef &Section) {
   if (auto EC = Section.getContents(Data))
     return errorCodeToError(EC);
   Address = Section.getAddress();
   return Error::success();
 }
 
 StringRef InstrProfSymtab::getFuncName(uint64_t Pointer, size_t Size) {
   if (Pointer < Address)
     return StringRef();
   auto Offset = Pointer - Address;
   if (Offset + Size > Data.size())
     return StringRef();
   return Data.substr(Pointer - Address, Size);
 }
 
+// Check if the mapping data is a dummy, i.e. is emitted for an unused function.
+static Error isCoverageMappingDummy(uint64_t Hash, StringRef Mapping,
+                                    bool &Result) {
+  // The hash value of dummy mapping records is always zero.
+  if (Hash) {
+    Result = false;
+    return Error::success();
+  }
+  return RawCoverageMappingDummyChecker(Mapping).isDummy(Result);
+}
+
 namespace {
 struct CovMapFuncRecordReader {
   // The interface to read coverage mapping function records for
   // a module. \p Buf is a reference to the buffer pointer pointing
   // to the \c CovHeader of coverage mapping data associated with
   // the module.
   virtual Error readFunctionRecords(const char *&Buf, const char *End) = 0;
   virtual ~CovMapFuncRecordReader() {}
   template <class IntPtrT, support::endianness Endian>
   static Expected<std::unique_ptr<CovMapFuncRecordReader>>
   get(coverage::CovMapVersion Version, InstrProfSymtab &P,
       std::vector<BinaryCoverageReader::ProfileMappingRecord> &R,
       std::vector<StringRef> &F);
 };
 
 // A class for reading coverage mapping function records for a module.
 template <coverage::CovMapVersion Version, class IntPtrT,
           support::endianness Endian>
 class VersionedCovMapFuncRecordReader : public CovMapFuncRecordReader {
   typedef typename coverage::CovMapTraits<
       Version, IntPtrT>::CovMapFuncRecordType FuncRecordType;
   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 existent record if it's a dummy and the new record is real.
+    size_t OldRecordIndex = InsertResult.first->second;
+    BinaryCoverageReader::ProfileMappingRecord &OldRecord =
+        Records[OldRecordIndex];
+    bool OldIsDummy;
+    if (Error Err = isCoverageMappingDummy(
+            OldRecord.FunctionHash, OldRecord.CoverageMapping, OldIsDummy))
+      return Err;
+    if (!OldIsDummy)
+      return Error::success();
+    bool NewIsDummy;
+    if (Error Err = isCoverageMappingDummy(FuncHash, Mapping, NewIsDummy))
+      return Err;
+    if (NewIsDummy)
+      return Error::success();
+    OldRecord.FunctionHash = FuncHash;
+    OldRecord.CoverageMapping = Mapping;
+    OldRecord.FilenamesBegin = FilenamesBegin;
+    OldRecord.FilenamesSize = Filenames.size() - FilenamesBegin;
+    return Error::success();
+  }
+
 public:
   VersionedCovMapFuncRecordReader(
       InstrProfSymtab &P,
       std::vector<BinaryCoverageReader::ProfileMappingRecord> &R,
       std::vector<StringRef> &F)
       : ProfileNames(P), Filenames(F), Records(R) {}
   ~VersionedCovMapFuncRecordReader() override {}
 
Error readFunctionRecords(const char *&Buf, const char *End) override {
     // Each coverage map has an alignment of 8, so we need to adjust alignment
     // before reading the next map.
     Buf += alignmentAdjustment(Buf, 8);
 
     auto CFR = reinterpret_cast<const FuncRecordType *>(FunBuf);
     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)
         return make_error<CoverageMapError>(coveragemap_error::malformed);
       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();
   }
 };
 } // end anonymous namespace
 
 template <class IntPtrT, support::endianness Endian>
 Expected<std::unique_ptr<CovMapFuncRecordReader>> CovMapFuncRecordReader::get(
     coverage::CovMapVersion Version, InstrProfSymtab &P,
     std::vector<BinaryCoverageReader::ProfileMappingRecord> &R,
     std::vector<StringRef> &F) {

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

+#include "prefer_used_to_unused.h"
+
+int main() {
+  return sampleFunc(5) + simpleFunc(5);
+}

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

+_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

+// Check that llvm-cov loads coverage mapping data for real function even though
+// an unused function comes first.
+
+// 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]]|}