Fix branch coverage merging across function instantiations

[alanphipps]

Summary

When I made the upstream commit for branch coverage support in code-coverage in January, I made an incorrect assumption that branch coverage numbers (covered branch and total branches) should be accumulated across function instantiations. This is not what is done for lines and regions, where the maximum line/region coverage found in a function instantiation across an instantiation group is returned. For example, if a function template definition has 2 total branches, the branch coverage would be reported as an accumulated total across all instantiations (8 total branches across 4 instantiations).

The correct assumption for the FunctionCoverageSummary::get(const InstantiationGroup &Group, ...) routine is that the summary it returns should agree with the function definition in the source code on lines, regions, and branches. So we should do the same thing for branch coverage as we do for line and region coverage. So if a function template definition has 2 total branches, the summary should also reflect branch coverage for 2 total branches.

This change corrects the implementation for the branch coverage summary to do the same thing for branches that is done for lines and regions. That is, across function instantiations in an instantiation group, the maximum branch coverage found in any of those instantiations is returned, with the total number of branches being the same across instantiations.

[snidertm]

LGTM

[thakis]

Looks like this breaks tests on windows: http://45.33.8.238/win/38516/step_11.txt

PTAL, and revert for now if it takes a while to fix.

[alanphipps]

In D102193#2750971, @thakis wrote:

Looks like this breaks tests on windows: http://45.33.8.238/win/38516/step_11.txt

PTAL, and revert for now if it takes a while to fix.

Thanks -- Backslash issue was tripping up Windows. I fixed it and will reland.

[tstellar]

I saw this fix was backported to the release/12.x branch, was there a bug for this?

[alanphipps]

In D102193#2752558, @tstellar wrote:

I saw this fix was backported to the release/12.x branch, was there a bug for this?

I did not file an official bug for this issue -- should I have? My original commit on 12.x also needed to be fixed, but if I missed something in the process for updating release branches, let me know. Thanks!

[tstellar]

In D102193#2752595, @alanphipps wrote:

In D102193#2752558, @tstellar wrote:

I saw this fix was backported to the release/12.x branch, was there a bug for this?

I did not file an official bug for this issue -- should I have? My original commit on 12.x also needed to be fixed, but if I missed something in the process for updating release branches, let me know. Thanks!

OK, I will revert that commit on the 12.x branch then. Can you please file a bug for this patch and mark it as blocker of the release-12.0.1 metabug? I typically review and try to get an ack from at least on other person before backporting patches.

[alanphipps]

OK, I will revert that commit on the 12.x branch then. Can you please file a bug for this patch and mark it as blocker of the release-12.0.1 metabug? I typically review and try to get an ack from at least on other person before backporting patches.

Done - thank you!

https://bugs.llvm.org/show_bug.cgi?id=50394

llvm/test/tools/llvm-cov/branch-templates.cpp

 // RUN: llvm-profdata merge %S/Inputs/branch-templates.proftext -o %t.profdata
 // RUN: llvm-cov show --show-expansions --show-branches=count %S/Inputs/branch-templates.o32l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 // RUN: llvm-cov report --show-branch-summary %S/Inputs/branch-templates.o32l -instr-profile %t.profdata -show-functions -path-equivalence=/tmp,%S %s | FileCheck %s -check-prefix=REPORT
+// RUN: llvm-cov report --show-branch-summary %S/Inputs/branch-templates.o32l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s -check-prefix=REPORTFILE
 
 #include <stdio.h>
-
 template<typename T>
 void unused(T x) {
   return;
 }
 
 template<typename T>
 int func(T x) {
   if(x)       // CHECK: |  Branch ([[@LINE]]:6): [True: 0, False: 1]

 // REPORT: Name                        Regions    Miss   Cover     Lines    Miss   Cover  Branches    Miss   Cover
 // REPORT-NEXT: ---
 // REPORT-NEXT: main                              7       1  85.71%        10       1  90.00%         6       3  50.00%
 // REPORT-NEXT: _Z4funcIiEiT_                     5       2  60.00%         7       3  57.14%         2       1  50.00%
 // REPORT-NEXT: _Z4funcIbEiT_                     5       2  60.00%         7       4  42.86%         2       1  50.00%
 // REPORT-NEXT: _Z4funcIfEiT_                     5       2  60.00%         7       3  57.14%         2       1  50.00%
 // REPORT-NEXT: ---
 // REPORT-NEXT: TOTAL                            22       7  68.18%        31      11  64.52%        12       6  50.00%
+
+// Make sure the covered branch tally for the function instantiation group is
+// merged to reflect maximum branch coverage of a single instantiation, just
+// like what is done for lines and regions. Also, the total branch tally
+// summary for an instantiation group should agree with the total number of
+// branches in the definition (In this case, 2 and 6 for func<>() and main(),
+// respectively).  This is returned by: FunctionCoverageSummary::get(const
+// InstantiationGroup &Group, ...)
+
+// REPORTFILE: Filename                      Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
+// REPORTFILE-NEXT: ---
+// REPORTFILE-NEXT: /tmp/branch-templates.cpp          12                 3    75.00%           2                 0   100.00%          17                 4    76.47%           8                 4    50.00%
+// REPORTFILE-NEXT: ---
+// REPORTFILE-NEXT: TOTAL                              12                 3    75.00%           2                 0   100.00%          17                 4    76.47%           8                 4    50.00%

llvm/tools/llvm-cov/CoverageSummaryInfo.h

public:
   }
 
   BranchCoverageInfo &operator+=(const BranchCoverageInfo &RHS) {
     Covered += RHS.Covered;
     NumBranches += RHS.NumBranches;
     return *this;
   }
 
+  void merge(const BranchCoverageInfo &RHS) {
+    Covered = std::max(Covered, RHS.Covered);
+    NumBranches = std::max(NumBranches, RHS.NumBranches);
+  }
+
   size_t getCovered() const { return Covered; }
 
   size_t getNumBranches() const { return NumBranches; }
 
   bool isFullyCovered() const { return Covered == NumBranches; }
 
   double getPercentCovered() const {
     assert(Covered <= NumBranches && "Covered branches over-counted");

llvm/tools/llvm-cov/CoverageSummaryInfo.cpp

FunctionCoverageSummary::get(const InstantiationGroup &Group,
   FunctionCoverageSummary Summary(Name);
   Summary.ExecutionCount = Group.getTotalExecutionCount();
   Summary.RegionCoverage = Summaries[0].RegionCoverage;
   Summary.LineCoverage = Summaries[0].LineCoverage;
   Summary.BranchCoverage = Summaries[0].BranchCoverage;
   for (const auto &FCS : Summaries.drop_front()) {
     Summary.RegionCoverage.merge(FCS.RegionCoverage);
     Summary.LineCoverage.merge(FCS.LineCoverage);
-
-    // Sum branch coverage across instantiation groups for the summary rather
-    // than "merge" the maximum count. This is a clearer view into whether all
-    // created branches are covered.
-    Summary.BranchCoverage += FCS.BranchCoverage;
+    Summary.BranchCoverage.merge(FCS.BranchCoverage);
   }
   return Summary;
 }