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

[Justin Bogner via llvm-commits]

Igor Kudrin <ikudrin.dev at gmail.com> writes:
> ikudrin created this revision.
> ikudrin added reviewers: bogner, davidxl, vsk.
> ikudrin added a subscriber: llvm-commits.
>
> 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).
>
> Please note, that this patch is based on http://reviews.llvm.org/D18758.
>
> http://reviews.llvm.org/D18787
>
> Files:
>   llvm/trunk/lib/ProfileData/CoverageMapping.cpp
>   llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp
>
> Index: llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp
> ===================================================================
> --- llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp
> +++ llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp
> @@ -449,6 +449,46 @@
>    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));
> +  readProfCounts();
> +
> +  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));
> +  readProfCounts();
> +
> +  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());
>  
> Index: llvm/trunk/lib/ProfileData/CoverageMapping.cpp
> ===================================================================
> --- llvm/trunk/lib/ProfileData/CoverageMapping.cpp
> +++ llvm/trunk/lib/ProfileData/CoverageMapping.cpp
> @@ -366,21 +366,6 @@
>    return FilenameEquivalence;
>  }
>  
> -static Optional<unsigned> findMainViewFileID(StringRef SourceFile,
> -                                             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])
> -      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)
> @@ -392,6 +377,14 @@
>    return I;
>  }
>  
> +static Optional<unsigned> findMainViewFileID(StringRef SourceFile,
> +                                             const FunctionRecord &Function) {
> +  Optional<unsigned> I = findMainViewFileID(Function);
> +  if (I && SourceFile != Function.Filenames[*I])
> +    I.reset();

I think "return None" would be clearer than "I.reset()" here.

> +  return I;
> +}
> +
>  /// Sort a nested sequence of regions from a single file.
>  template <class It> static void sortNestedRegions(It First, It Last) {
>    std::sort(First, Last,
> @@ -413,13 +406,11 @@
>  
>    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);
>        }
>    }