[Coverage] Prevent false instantiations detection in case of macro expansions.

[ikudrin]

Summary

The root of the problem was that findMainViewFileID(File, Function) could return some ID for any given file,
even though that file was not the main for that function.
This patch ensures that the result of this function is conformed with the result of findMainViewFileID(Function).

[ikudrin]

Use the following commands to reproduce the issue:

$ cat > sample.cpp << EOF
#include "sample.h"

void func1() {
  DO_SOMETHING();
}

void func2() {
  DO_SOMETHING();
}

int main() {
  for (int I = 0; I < 10; ++I)
    if (I < 7)
      func1();
    else
      func2();
  return 0;
}
EOF
$ cat > sample.h << EOF
#define DO_SOMETHING() \
  do { \
  } while(0)

/*
 * We need some lines here to show the issue.
 *
 *
 *
 */
EOF
$ clang++ -fprofile-instr-generate -fcoverage-mapping sample.cpp 
$ ./a.out
$ llvm-profdata merge -o default.profdata default.profraw
$ llvm-cov show a.out -instr-profile default.profdata -filename-equivalence sample.h 
       |    1|#define DO_SOMETHING() \
     10|    2|  do { \
     10|    3|  } while(0)
       |    4|
       |    5|/*
  ------------------
  | _Z5func1v:
  |      7|    3|  } while(0)
  |      7|    4|
  |      7|    5|/*
  ------------------
       |    6| * We need some lines here to show the issue.
       |    7| *
       |    8| *
       |    9| *
  ------------------
  | _Z5func2v:
  |      3|    7| *
  |      3|    8| *
  |      3|    9| *
  ------------------
       |   10| */

[davidxl]

Can you also add the motivation case as one of the test case under tests/tools/llvm-cov ?

Otherwise looks good.

llvm/trunk/lib/ProfileData/CoverageMapping.cpp

379

while you are at it, please add a brief comment for this method.

384

Another fix is to check

&& FilenameEquivalence[CR.ExpandedFileID])

...

but I think your refactored code looks better.

390

Add a comment for the function.

[ikudrin]

• Added the motivation case to the tests for llvm-cov.
• Added comments.
• Updated findMainViewFileID(File, Function) according to Justin's comment.

[davidxl]

lgtm

llvm/trunk/lib/ProfileData/CoverageMapping.cpp

static SmallBitVector gatherFileIDs(StringRef SourceFile,
                                     const FunctionRecord &Function) {
   SmallBitVector FilenameEquivalence(Function.Filenames.size(), false);
   for (unsigned I = 0, E = Function.Filenames.size(); I < E; ++I)
     if (SourceFile == Function.Filenames[I])
       FilenameEquivalence[I] = true;
   return FilenameEquivalence;
 }
 
-static Optional<unsigned> findMainViewFileID(StringRef SourceFile,
-                                             const FunctionRecord &Function) {
+/// Return the ID of the file where the definition of the function is located.
+static Optional<unsigned> findMainViewFileID(const FunctionRecord &Function) {
   SmallBitVector IsNotExpandedFile(Function.Filenames.size(), true);
-  SmallBitVector FilenameEquivalence = gatherFileIDs(SourceFile, Function);
   for (const auto &CR : Function.CountedRegions)
-    if (CR.Kind == CounterMappingRegion::ExpansionRegion &&
-        FilenameEquivalence[CR.FileID])
+    if (CR.Kind == CounterMappingRegion::ExpansionRegion)
       IsNotExpandedFile[CR.ExpandedFileID] = false;
-  IsNotExpandedFile &= FilenameEquivalence;
   int I = IsNotExpandedFile.find_first();
   if (I == -1)
     return None;
   return I;
 }
 
-static Optional<unsigned> findMainViewFileID(const FunctionRecord &Function) {
-  SmallBitVector IsNotExpandedFile(Function.Filenames.size(), true);
-  for (const auto &CR : Function.CountedRegions)
-    if (CR.Kind == CounterMappingRegion::ExpansionRegion)
-      IsNotExpandedFile[CR.ExpandedFileID] = false;
-  int I = IsNotExpandedFile.find_first();
-  if (I == -1)
-    return None;
+/// Check if SourceFile is the file that contains the definition of
+/// the Function. Return the ID of the file in that case or None otherwise.
+static Optional<unsigned> findMainViewFileID(StringRef SourceFile,
+                                             const FunctionRecord &Function) {
+  Optional<unsigned> I = findMainViewFileID(Function);
+  if (I && SourceFile == Function.Filenames[*I])
     return I;
+  return None;
 }
 
 /// Sort a nested sequence of regions from a single file.
 template <class It> static void sortNestedRegions(It First, It Last) {
   std::sort(First, Last,
             [](const CountedRegion &LHS, const CountedRegion &RHS) {
     if (LHS.startLoc() == RHS.startLoc())
       // When LHS completely contains RHS, we sort LHS first.
       return RHS.endLoc() < LHS.endLoc();
     return LHS.startLoc() < RHS.startLoc();
   });
 }
 
 static bool isExpansion(const CountedRegion &R, unsigned FileID) {
   return R.Kind == CounterMappingRegion::ExpansionRegion && R.FileID == FileID;
 }
 
 CoverageData CoverageMapping::getCoverageForFile(StringRef Filename) {
   CoverageData FileCoverage(Filename);
   std::vector<coverage::CountedRegion> Regions;
 
   for (const auto &Function : Functions) {
     auto MainFileID = findMainViewFileID(Filename, Function);
-    if (!MainFileID)
-      continue;
     auto FileIDs = gatherFileIDs(Filename, Function);
     for (const auto &CR : Function.CountedRegions)
       if (FileIDs.test(CR.FileID)) {
         Regions.push_back(CR);
-        if (isExpansion(CR, *MainFileID))
+        if (MainFileID && isExpansion(CR, *MainFileID))
           FileCoverage.Expansions.emplace_back(CR, Function);
       }
   }
 
   sortNestedRegions(Regions.begin(), Regions.end());
   DEBUG(dbgs() << "Emitting segments for file: " << Filename << "\n");
   FileCoverage.Segments = SegmentBuilder::buildSegments(Regions);
 

llvm/trunk/test/tools/llvm-cov/Inputs/prevent_false_instantiations.cpp

+#include "prevent_false_instantiations.h"
+
+void func1() {
+  DO_SOMETHING();
+}
+
+void func2() {
+  DO_SOMETHING();
+}
+
+int main() {
+  func1();
+  func2();
+  return 0;
+}

llvm/trunk/test/tools/llvm-cov/Inputs/prevent_false_instantiations.proftext

+_Z5func1v
+# Func Hash:
+3
+# Num Counters:
+2
+# Counter Values:
+1
+0
+
+_Z5func2v
+# Func Hash:
+3
+# Num Counters:
+2
+# Counter Values:
+1
+0
+
+main
+# Func Hash:
+0
+# Num Counters:
+1
+# Counter Values:
+1
+

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

-config.suffixes = ['.test', '.m', '.cpp', '.c']
+config.suffixes = ['.test', '.m', '.cpp', '.c', '.h']

llvm/trunk/test/tools/llvm-cov/prevent_false_instantiations.h

+// Checks that function instantiations don't go to a wrong file.
+
+// CHECK-NOT: {{_Z5func[1,2]v}}
+
+// RUN: llvm-profdata merge %S/Inputs/prevent_false_instantiations.proftext -o %t.profdata
+// RUN: llvm-cov show %S/Inputs/prevent_false_instantiations.covmapping -instr-profile %t.profdata -filename-equivalence %s | FileCheck %s
+
+#define DO_SOMETHING() \
+  do {                 \
+  } while (0)

llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp

TEST_P(MaybeSparseCoverageMappingTest, strip_unknown_filename_prefix) {
 
   std::vector<std::string> Names;
   for (const auto &Func : LoadedCoverage->getCoveredFunctions())
     Names.push_back(Func.Name);
   ASSERT_EQ(1U, Names.size());
   ASSERT_EQ("func", Names[0]);
 }
 
+TEST_P(MaybeSparseCoverageMappingTest, dont_detect_false_instantiations) {
+  InstrProfRecord Record1("foo", 0x1234, {10});
+  InstrProfRecord Record2("bar", 0x2345, {20});
+  ProfileWriter.addRecord(std::move(Record1));
+  ProfileWriter.addRecord(std::move(Record2));
+
+  startFunction("foo", 0x1234);
+  addCMR(Counter::getCounter(0), "expanded", 1, 1, 1, 10);
+  addExpansionCMR("main", "expanded", 4, 1, 4, 5);
+
+  startFunction("bar", 0x2345);
+  addCMR(Counter::getCounter(0), "expanded", 1, 1, 1, 10);
+  addExpansionCMR("main", "expanded", 9, 1, 9, 5);
+
+  loadCoverageMapping();
+
+  std::vector<const FunctionRecord *> Instantiations =
+      LoadedCoverage->getInstantiations("expanded");
+  ASSERT_TRUE(Instantiations.empty());
+}
+
+TEST_P(MaybeSparseCoverageMappingTest, load_coverage_for_expanded_file) {
+  InstrProfRecord Record("func", 0x1234, {10});
+  ProfileWriter.addRecord(std::move(Record));
+
+  startFunction("func", 0x1234);
+  addCMR(Counter::getCounter(0), "expanded", 1, 1, 1, 10);
+  addExpansionCMR("main", "expanded", 4, 1, 4, 5);
+
+  loadCoverageMapping();
+
+  CoverageData Data = LoadedCoverage->getCoverageForFile("expanded");
+  std::vector<CoverageSegment> Segments(Data.begin(), Data.end());
+  ASSERT_EQ(2U, Segments.size());
+  EXPECT_EQ(CoverageSegment(1, 1, 10, true), Segments[0]);
+  EXPECT_EQ(CoverageSegment(1, 10, false), Segments[1]);
+}
+
 INSTANTIATE_TEST_CASE_P(MaybeSparse, MaybeSparseCoverageMappingTest,
                         ::testing::Bool());
 
 } // end anonymous namespace