[llvm-cov] Fix branch counts of template functions (#111743) #113925

stma247 · 2024-10-28T15:21:28Z

Added option "--unify-instantiations" to llvm-cov export to combine branch execution counts of C++ template instantiations.

on-behalf-of: @e-solutions-GmbH info@esolutions.de

github-actions · 2024-10-28T15:21:51Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

stma247 · 2024-11-05T10:46:45Z

Hi @evodius96, could you please review my changes.

stma247 · 2024-11-13T19:54:52Z

Ping

evodius96 · 2024-11-13T23:04:59Z

Sorry for the delay, I was out of town for a family emergency. I will take a look at this this week! Thanks

evodius96 · 2024-11-15T23:44:37Z

llvm/tools/llvm-cov/CodeCoverage.cpp

@@ -1270,13 +1270,18 @@ int CodeCoverageTool::doExport(int argc, const char **argv,
                              cl::desc("Don't export branch data (LCOV)"),
                              cl::cat(ExportCategory));

+  cl::opt<bool> UnifyInstantiations("unify-instantiations", cl::Optional,
+                                    cl::desc("Unify function instantiations"),
+                                    cl::init(false), cl::cat(ExportCategory));


Would it be worthwhile to turn this ON by default? Then the summary would be more consistent with the summary output of llvm-cov show.

Okay, I changed the default to true.

evodius96 · 2024-11-15T23:45:10Z

llvm/tools/llvm-cov/CoverageExporterLcov.cpp

-    if (ViewDepth == 0)
-      SrcLine = Expansion.Region.LineStart;
+    // Track the path to the nested expansions
+    NestedPath.push_back(Expansion.Region.startLoc());


Much better than tracking LineStart. Thanks!

Yes, it's required to avoid unifying different expanded branches of the same root line.

llvm/tools/llvm-cov/CoverageExporterLcov.cpp

evodius96 · 2024-11-21T19:15:34Z

llvm/tools/llvm-cov/CoverageExporterLcov.cpp

+  // Contains the path to default and expanded branches
+  // Size is 1 for default branches and greater 1 for expanded branches.
+  std::vector<LineColPair> NestedPath;
+  // Indicates whether this item should at rendering


Nit: For LLVM, it's typically to end comments with a period.

Ah, I didn't know that and updated all my added comments now.

evodius96

Other than the nit about comments, LGTM. Thanks!

stma247 · 2024-11-22T14:29:40Z

Thanks for the review.
When will this PR be merged?

evodius96 · 2024-11-25T01:56:28Z

Thanks for the review.

When will this PR be merged?

You can trigger the merge yourself from this pull request.

Added option "--unify-instantiations" to llvm-cov export to combine branch execution counts of C++ template instantiations. on-behalf-of: @e-solutions-GmbH <info@esolutions.de>

stma247 · 2024-11-27T19:11:51Z

Thanks for the review.
When will this PR be merged?

You can trigger the merge yourself from this pull request.

The merge button is always disabled for me with info:
Only those with [write access] to this repository can merge pull requests.

stma247 · 2024-12-02T15:17:39Z

Ping

evodius96 · 2024-12-02T20:23:13Z

The merge button is always disabled for me with info:
Only those with [write access] to this repository can merge pull requests.

Apologies -- last week was a holiday in the US. Ah, I see. I think you should request write access to the repo, but the process has changed since I did it. Nevertheless, I will attempt to merge this for you since I have the ability to do that.

github-actions · 2024-12-02T20:24:36Z

@stma247 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvm-ci · 2024-12-02T21:40:26Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-expensive-checks-debian running on gribozavr4 while building llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/9919

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM :: tools/llvm-cov/branch-export-lcov-unify-instances.test' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 2: /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llvm-profdata merge /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/Inputs/branch-templates.proftext -o /b/1/llvm-clang-x86_64-expensive-checks-debian/build/test/tools/llvm-cov/Output/branch-export-lcov-unify-instances.test.tmp.profdata
+ /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llvm-profdata merge /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/Inputs/branch-templates.proftext -o /b/1/llvm-clang-x86_64-expensive-checks-debian/build/test/tools/llvm-cov/Output/branch-export-lcov-unify-instances.test.tmp.profdata
RUN: at line 3: /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llvm-cov export --format=lcov --unify-instantiations=true /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/Inputs/branch-templates.o32l -instr-profile /b/1/llvm-clang-x86_64-expensive-checks-debian/build/test/tools/llvm-cov/Output/branch-export-lcov-unify-instances.test.tmp.profdata | /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/FileCheck /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/branch-export-lcov-unify-instances.test -check-prefix=UNIFY
+ /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llvm-cov export --format=lcov --unify-instantiations=true /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/Inputs/branch-templates.o32l -instr-profile /b/1/llvm-clang-x86_64-expensive-checks-debian/build/test/tools/llvm-cov/Output/branch-export-lcov-unify-instances.test.tmp.profdata
+ /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/FileCheck /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/branch-export-lcov-unify-instances.test -check-prefix=UNIFY
RUN: at line 19: /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llvm-profdata merge /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/Inputs/branch-templates.proftext -o /b/1/llvm-clang-x86_64-expensive-checks-debian/build/test/tools/llvm-cov/Output/branch-export-lcov-unify-instances.test.tmp.profdata
+ /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llvm-profdata merge /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/Inputs/branch-templates.proftext -o /b/1/llvm-clang-x86_64-expensive-checks-debian/build/test/tools/llvm-cov/Output/branch-export-lcov-unify-instances.test.tmp.profdata
RUN: at line 20: /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llvm-cov export --format=lcov --unify-instantiations=false /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/Inputs/branch-templates.o32l -instr-profile /b/1/llvm-clang-x86_64-expensive-checks-debian/build/test/tools/llvm-cov/Output/branch-export-lcov-unify-instances.test.tmp.profdata | /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/FileCheck /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/branch-export-lcov-unify-instances.test
+ /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llvm-cov export --format=lcov --unify-instantiations=false /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/Inputs/branch-templates.o32l -instr-profile /b/1/llvm-clang-x86_64-expensive-checks-debian/build/test/tools/llvm-cov/Output/branch-export-lcov-unify-instances.test.tmp.profdata
+ /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/FileCheck /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/branch-export-lcov-unify-instances.test
/b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/branch-export-lcov-unify-instances.test:24:15: error: CHECK-DAG: expected string not found in input
// CHECK-DAG: BRDA:14,1,2,1
              ^
<stdin>:1:1: note: scanning from here
SF:/tmp/branch-templates.cpp
^
<stdin>:31:1: note: possible intended match here
BRDA:14,1,2,0
^

Input file: <stdin>
Check file: /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/test/tools/llvm-cov/branch-export-lcov-unify-instances.test

-dump-input=help explains the following input dump.

Input was:
<<<<<<
          1: SF:/tmp/branch-templates.cpp 
dag:24'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
          2: FN:29,main 
dag:24'0     ~~~~~~~~~~~
          3: FN:13,_Z4funcIiEiT_ 
dag:24'0     ~~~~~~~~~~~~~~~~~~~~
          4: FN:13,_Z4funcIbEiT_ 
dag:24'0     ~~~~~~~~~~~~~~~~~~~~
          5: FN:13,_Z4funcIfEiT_ 
dag:24'0     ~~~~~~~~~~~~~~~~~~~~
          6: FNDA:1,main 
dag:24'0     ~~~~~~~~~~~~
          .
          .
          .
         26: DA:36,1 
dag:24'0     ~~~~~~~~
         27: DA:37,1 
...

…113925)" This reverts commit 0fe1f85.

…#118398) Reverts #113925 Reverting to address buildbot failures.

evodius96 · 2024-12-02T21:49:19Z

@stma247 Looks like the LIT test failed on a couple of the build bots. I reverted the commit until that can be addressed.

stma247 · 2025-04-09T08:26:32Z

@stma247 Looks like the LIT test failed on a couple of the build bots. I reverted the commit until that can be addressed.

Hi @evodius96, I must have missed the reverted commit somehow.
However, now I took the time to reproduce the issue and it only seems to show up if EXPENSIVE_CHECKS are enabled. It is related to "llvm::sort" that applies some kind of shuffle if expensive checks are on (see https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/STLExtras.h#L1676).
The reason why this issue came up with my PR, is that no test for this template ambiguity existed before (see new llvm/test/tools/llvm-cov/branch-export-lcov-unify-instances.test). I can see this indeterministic behavior even on branch "main", if I add test code from "branch-export-lcov-unify-instances.test".

I am working on a solution and will create a new PR.

Fixed flaky test if EXPENSIVE_CHECKS are enabled, caused by an ambiguous comparator. Keep orginal position in NestedCountedRegion to sort distinct. on-behalf-of: @e-solutions-GmbH <info@esolutions.de>

stma247 · 2025-04-09T19:50:42Z

The new PR is #135074

Fixed flaky test if EXPENSIVE_CHECKS are enabled, caused by an ambiguous comparator. Keep orginal position in NestedCountedRegion to sort distinct. on-behalf-of: @e-solutions-GmbH <info@esolutions.de>

…135074) This PR is a second attempt for issue #111743 to finish reverted PR #113925. Added option "--unify-instantiations" to llvm-cov export to combine branch execution counts of C++ template instantiations. Fix non-deterministic behavior.

…lvm#135074) This PR is a second attempt for issue llvm#111743 to finish reverted PR llvm#113925. Added option "--unify-instantiations" to llvm-cov export to combine branch execution counts of C++ template instantiations. Fix non-deterministic behavior.

stma247 force-pushed the users/stma247/llvm_cov_export_with_unified_function_instantiations branch 2 times, most recently from 785f306 to 74764e5 Compare October 29, 2024 09:22

evodius96 reviewed Nov 15, 2024

View reviewed changes

llvm/tools/llvm-cov/CoverageExporterLcov.cpp Outdated Show resolved Hide resolved

stma247 force-pushed the users/stma247/llvm_cov_export_with_unified_function_instantiations branch 2 times, most recently from e69e9a5 to ecde8c3 Compare November 21, 2024 10:19

evodius96 reviewed Nov 21, 2024

View reviewed changes

evodius96 approved these changes Nov 21, 2024

View reviewed changes

stma247 force-pushed the users/stma247/llvm_cov_export_with_unified_function_instantiations branch from ecde8c3 to d94626e Compare November 22, 2024 14:16

stma247 force-pushed the users/stma247/llvm_cov_export_with_unified_function_instantiations branch 11 times, most recently from 5346252 to 53a9786 Compare November 27, 2024 17:52

[llvm-cov] Fix branch counts of template functions (llvm#111743)

0a20607

Added option "--unify-instantiations" to llvm-cov export to combine branch execution counts of C++ template instantiations. on-behalf-of: @e-solutions-GmbH <info@esolutions.de>

stma247 force-pushed the users/stma247/llvm_cov_export_with_unified_function_instantiations branch from 53a9786 to 0a20607 Compare November 27, 2024 19:05

evodius96 merged commit 0fe1f85 into llvm:main Dec 2, 2024
7 checks passed

evodius96 added a commit that referenced this pull request Dec 2, 2024

Revert "[llvm-cov] Fix branch counts of template functions (#111743) (#…

c4ab7f0

…113925)" This reverts commit 0fe1f85.

evodius96 mentioned this pull request Dec 2, 2024

Revert "[llvm-cov] Fix branch counts of template functions (#111743)" #118398

Merged

evodius96 added a commit that referenced this pull request Dec 2, 2024

Revert "[llvm-cov] Fix branch counts of template functions (#111743)" (…

92f5314

…#118398) Reverts #113925 Reverting to address buildbot failures.

stma247 mentioned this pull request Apr 9, 2025

[llvm-cov] Fix branch counts of template functions (second attempt) #135074

Merged

[llvm-cov] Fix branch counts of template functions (#111743) #113925

[llvm-cov] Fix branch counts of template functions (#111743) #113925

Uh oh!

Conversation

stma247 commented Oct 28, 2024

Uh oh!

github-actions bot commented Oct 28, 2024

Uh oh!

stma247 commented Nov 5, 2024

Uh oh!

stma247 commented Nov 13, 2024

Uh oh!

evodius96 commented Nov 13, 2024

Uh oh!

evodius96 Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

stma247 Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

evodius96 Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

stma247 Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evodius96 Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

stma247 Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

evodius96 left a comment

Choose a reason for hiding this comment

Uh oh!

stma247 commented Nov 22, 2024

Uh oh!

evodius96 commented Nov 25, 2024

Uh oh!

stma247 commented Nov 27, 2024

Uh oh!

stma247 commented Dec 2, 2024

Uh oh!

evodius96 commented Dec 2, 2024

Uh oh!

Uh oh!

github-actions bot commented Dec 2, 2024

Uh oh!

llvm-ci commented Dec 2, 2024

Uh oh!

evodius96 commented Dec 2, 2024

Uh oh!

stma247 commented Apr 9, 2025

Uh oh!

stma247 commented Apr 9, 2025

Uh oh!

Uh oh!