Skip to content

gh-94759: Collect C-level coverage using llvm-cov #94760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+275 −24
Open

gh-94759: Collect C-level coverage using llvm-cov #94760

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e2eed08
Collect C-level coverage using llvm-cov
mdboom Jul 11, 2022
776e423
Refactor this as a Makefile target
mdboom Jul 13, 2022
1ab7870
Automatically select the coverage approach based on the compiler
mdboom Jul 13, 2022
80f0039
Update configure.ac
mdboom Jul 28, 2022
dfa3fcb
Fix lcov coverage
mdboom Aug 8, 2022
50ba4f4
Remove unnecessary quotes
mdboom Aug 24, 2022
84f042a
Move more logic into configure
mdboom Aug 24, 2022
ceb32ee
Cleanup after recent changes
mdboom Aug 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Automatically select the coverage approach based on the compiler
  • Loading branch information
@mdboom
mdboom committed Oct 31, 2022
commit 1ab7870447b70e0e0c2495776aa56cdde0867a55
2 changes: 1 addition & 1 deletion .github/workflows/c-coverage.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ jobs:
run: ./configure --with-pydebug --with-openssl=$OPENSSL_DIR
- name: Generate coverage report
# Using "-j1" is important, or the Github Action runs out of memory
run: EXTRATESTOPTS=-j1 xvfb-run make coverage-report-llvm
run: EXTRATESTOPTS=-j1 xvfb-run make coverage-report
- name: Publish coverage-report
Copy link
Member

@gpshead gpshead Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does every run clobber the previous run's report? What about CI runs on PRs and CI runs from release branches? Can this be configured to commit the coverage results into a branch in the repo matching the reponame+branchname? Or do branches not render in gh-pages? in which case it'd need to be subdirectories in the repo.

Copy link
Contributor Author

@mdboom mdboom Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, this clobbers the previous run. That is easily changed with a flag, but then we could run into Github repository size limits (each result is around 100MB of HTML). It's probably possible to keep the last N commits, but the tool I'm using to publish doesn't support that directly.

This is currently configured to just run on the main branch once a day. We could do multiple branches that publish to subdirectories if you think there's a good use case for that. (Github pages only publishes a single branch, but subdirectories would work).

Copy link
Member

@tiran tiran Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we would get some funding to run a persistent VM with public hostname from ... let's say Azure, then it would be trivial to create a buildbot worker and serve the LCOV results from HTTP server. They are static HTTP and JS files on the file system after all. Just saying :)

Copy link
Member

@gpshead gpshead Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind main branch only and once a day. Anyone working on coverage is presumably doing their own local coverage runs while creating PRs.

Regarding a buildbot configured to host the results, while I could simply set one up it'd probably make more sense for mdboom to do that and be an admin given who's driving this work. :)

Copy link
Contributor Author

@mdboom mdboom Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd be happy to admin that if it comes to it. I think it's fine to go with the Github Action here as an MVP, and if the amount of history or frequency of runs isn't good enough, we can revisit migrating to our own VM down the road.

uses: JamesIves/github-pages-deploy-action@v4
with:
Expand Down
29 changes: 18 additions & 11 deletions Makefile.pre.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ abs_builddir= @abs_builddir@
CC= @CC@
CXX= @CXX@
LINKCC= @LINKCC@
CC_NAME=@CC_NAME@
AR= @AR@
READELF= @READELF@
SOABI= @SOABI@
Expand Down Expand Up @@ -661,9 +662,20 @@ bolt-opt: @PREBOLT_RULE@
rm -f $(BUILDPYTHON).bolt_inst
mv $(BUILDPYTHON).bolt $(BUILDPYTHON)

.PHONY=coverage-report
coverage-report:
@if [ $(CC_NAME) = "gcc" ]; then \
$(MAKE) coverage-report-lcov; \
elif [ $(CC_NAME) = "clang" ]; then \
$(MAKE) coverage-report-llvm; \
else \
echo "Coverage is not supported with the $(CC_NAME) compiler"; \
exit 1; \
fi

# Compile and run with gcov
.PHONY=coverage coverage-lcov coverage-report
coverage:
.PHONY=coverage-gcc coverage-lcov coverage-report-lcov
coverage-gcc:
@echo "Building with support for coverage checking:"
$(MAKE) clean
$(MAKE) @DEF_MAKE_RULE@ CFLAGS="$(CFLAGS) -O0 -pg --coverage" LDFLAGS="$(LDFLAGS) --coverage"
Expand Down Expand Up @@ -699,7 +711,7 @@ coverage-lcov:
@echo

# Force regeneration of parser and frozen modules
coverage-report: regen-token regen-frozen
coverage-report-lcov: regen-token regen-frozen
@ # build with coverage info
$(MAKE) coverage
@ # run tests, ignore failures
Expand All @@ -708,14 +720,9 @@ coverage-report: regen-token regen-frozen
$(MAKE) coverage-lcov

# Compile and calculate coverage with llvm-cov
.PHONY=check-clang coverage-llvm coverage-profdata coverage-report-llvm

# Check whether the compiler is clang, and if not, error out.
check-clang:
($(CC) --version | grep clang) || \
(echo "LLVM coverage only works with clang. Set CC=clang and CXX=clang++ and re-run ./configure"; exit 1)
.PHONY=coverage-clang coverage-profdata coverage-report-llvm

coverage-llvm: check-clang
coverage-clang:
@echo "Building with support for coverage checking:"
$(MAKE) clean
@ # Override CC rather than CFLAGS since these flags must come first
Expand All @@ -739,7 +746,7 @@ coverage-profdata:
# into temporary directories created by tests.
coverage-report-llvm: regen-token regen-importlib
@ # build with coverage info
$(MAKE) coverage-llvm
$(MAKE) coverage-clang
@ # run tests, ignore failures
LLVM_PROFILE_FILE=${PWD}/python%m.profraw $(TESTRUNNER) $(TESTOPTS) || true
@ # build llvm-cov report
Expand Down
6 changes: 2 additions & 4 deletions Misc/NEWS.d/next/Build/2022-07-13-13-24-01.gh-issue-94759.dTLMod.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,2 @@
A new Makefile target ``coverage-report-llvm`` will use ``clang`` and
``llvm-cov`` to generate a coverage report. This provides more details about
branch coverage and subexpressions than the existing ``gcc`` and ``lcov``
based ``coverage-report``.
The ``coverage-report`` Makefile target will now automatically use ``llvm-cov`` to generate a coverage report when using ``clang``.
This provides more details about branch coverage and subexpressions than the existing ``gcc`` and ``lcov`` based ``coverage-report``.
4 changes: 4 additions & 0 deletions configure
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,6 +869,9 @@ fi
rm -f conftest.c conftest.out
])

AC_SUBST(CC_NAME)
CC_NAME=$ac_cv_cc_name

# checks for UNIX variants that set C preprocessor variables
# may set _GNU_SOURCE, __EXTENSIONS__, _POSIX_PTHREAD_SEMANTICS,
# _POSIX_SOURCE, _POSIX_1_SOURCE, and more
Expand Down