[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

440

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

440

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

[ikudrin]

lib/ProfileData/Coverage/CoverageMappingReader.cpp

440

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

445

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

474

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;

}

[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

293

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

343

Same Expected<bool> nit.

394

nit, existing

[davidxl]

lib/ProfileData/Coverage/CoverageMappingReader.cpp

347

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

6

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

293

Thanks! I'll update the patch tomorrow.

347

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

394

Thanks!

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

6

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) {}
 
   std::error_code 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) {}
+
+  ErrorOr<bool> isDummy();
+};
+
 /// \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 std::error_code();
 }
 
+// A dummy coverage mapping data consists from only one region record
+// with zero count.
+ErrorOr<bool> RawCoverageMappingDummyChecker::isDummy() {
+  uint64_t NumFileMappings;
+  if (auto Err = readSize(NumFileMappings))
+    return Err;
+  if (NumFileMappings != 1)
+    return false;
+  // 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 false;
+  uint64_t NumRegions;
+  if (auto Err = readSize(NumRegions))
+    return Err;
+  if (NumRegions != 1)
+    return false;
+  uint64_t EncodedCounterAndRegion;
+  if (auto Err = readIntMax(EncodedCounterAndRegion,
+                            std::numeric_limits<unsigned>::max()))
+    return Err;
+  unsigned Tag = EncodedCounterAndRegion & Counter::EncodingTagMask;
+  if (Tag != Counter::Zero)
+    return false;
+  return true;
+}
+
 std::error_code InstrProfSymtab::create(SectionRef &Section) {
   if (auto Err = Section.getContents(Data))
     return Err;
   Address = Section.getAddress();
   return std::error_code();
 }
 
 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);
 }
 
 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 std::error_code readFunctionRecords(const char *&Buf,
                                               const char *End) = 0;
   virtual ~CovMapFuncRecordReader() {}
   template <class IntPtrT, support::endianness Endian>
   static ErrorOr<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;
 
 public:
   VersionedCovMapFuncRecordReader(
       InstrProfSymtab &P,
       std::vector<BinaryCoverageReader::ProfileMappingRecord> &R,
       std::vector<StringRef> &F)
       : ProfileNames(P), Filenames(F), Records(R) {}
   ~VersionedCovMapFuncRecordReader() override {}
 
   std::error_code readFunctionRecords(const char *&Buf,
                                       const char *End) override {
     using namespace support;
     if (Buf + sizeof(CovMapHeader) > End)
       return coveragemap_error::malformed;
     auto CovHeader = reinterpret_cast<const coverage::CovMapHeader *>(Buf);
     uint32_t NRecords = CovHeader->getNRecords<Endian>();
     uint32_t FilenamesSize = CovHeader->getFilenamesSize<Endian>();
     uint32_t CoverageSize = CovHeader->getCoverageSize<Endian>();
     assert((CovMapVersion)CovHeader->getVersion<Endian>() == Version);
     Buf = reinterpret_cast<const char *>(CovHeader + 1);
 
     // Skip past the function records, saving the start and end for later.
     const char *FunBuf = Buf;
     Buf += NRecords * sizeof(FuncRecordType);
     const char *FunEnd = Buf;
 
     // Get the filenames.
     if (Buf + FilenamesSize > End)
       return coveragemap_error::malformed;
     size_t FilenamesBegin = Filenames.size();
     RawCoverageFilenamesReader Reader(StringRef(Buf, FilenamesSize), Filenames);
     if (auto Err = Reader.read())
       return Err;
     Buf += FilenamesSize;
 
     // 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 coveragemap_error::malformed;
     // 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) {
+    for (auto CFR = reinterpret_cast<const FuncRecordType *>(FunBuf);
+         (const char *)CFR < FunEnd; CFR++) {
       // 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 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.
+      // 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.
       NameRefType NameRef = CFR->template getFuncNameRef<Endian>();
-      if (!UniqueFunctionMappingData.insert(NameRef).second) {
-        CFR++;
+      auto InsertResult =
+          FunctionRecords.insert(std::make_pair(NameRef, Records.size()));
+      bool AlreadySeen = !InsertResult.second;
+      size_t RecordIndex = InsertResult.first->second;
+      if (AlreadySeen) {
+        // If the hash value for the old record is not zero, it's definitely
+        // not a dummy record and we don't need to replace it.
+        if (Records[RecordIndex].FunctionHash)
+          continue;
+        if (!FuncHash) {
+          // The hash value for a used function might be zero, for example,
+          // if the function is very simple. We need to analyse the mapping
+          // data itself in order to distinguish real records from dummy ones.
+          auto NewIsDummyOrErr =
+              RawCoverageMappingDummyChecker(Mapping).isDummy();
+          if (auto EC = NewIsDummyOrErr.getError())
+            return EC;
+          if (NewIsDummyOrErr.get())
+            continue;
+          auto OldIsDummyOrErr = RawCoverageMappingDummyChecker(
+                                     Records[RecordIndex].CoverageMapping)
+                                     .isDummy();
+          if (auto EC = OldIsDummyOrErr.getError())
+            return EC;
+          if (!OldIsDummyOrErr.get())
             continue;
         }
+      }
 
       StringRef FuncName;
       if (std::error_code EC =
               CFR->template getFuncName<Endian>(ProfileNames, FuncName))
         return EC;
-      Records.push_back(BinaryCoverageReader::ProfileMappingRecord(
+      BinaryCoverageReader::ProfileMappingRecord FunctionRecord(
           Version, FuncName, FuncHash, Mapping, FilenamesBegin,
-          Filenames.size() - FilenamesBegin));
-      CFR++;
+          Filenames.size() - FilenamesBegin);
+      if (AlreadySeen) {
+        Records[RecordIndex] = FunctionRecord;
+      } else {
+        Records.push_back(FunctionRecord);
+      }
     }
     return std::error_code();
   }
 };
 } // end anonymous namespace
 
 template <class IntPtrT, support::endianness Endian>
 ErrorOr<std::unique_ptr<CovMapFuncRecordReader>> CovMapFuncRecordReader::get(

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]]|}