Skip to content

bpo-39575: Change -lgcov to --coverage #18382

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.

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

Merged
merged 1 commit into from
Feb 7, 2020
Merged

Conversation

MaskRay
Copy link
Contributor

@MaskRay MaskRay commented Feb 6, 2020

This allows clang to get rid of the dependency on libgcov.
When linking, GCC passes -lgcov while clang passes the path to libclang_rt.profile-$arch.a

https://bugs.python.org/issue39575

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@MaskRay

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@csabella csabella requested a review from brettcannon February 6, 2020 22:10
@csabella csabella assigned tiran and unassigned tiran Feb 6, 2020
@csabella csabella requested a review from tiran February 6, 2020 22:10
@MaskRay MaskRay changed the title Change -lgcov to --coverage bpo-361528: Change -lgcov to --coverage Feb 7, 2020
@MaskRay MaskRay changed the title bpo-361528: Change -lgcov to --coverage bpo-39575: Change -lgcov to --coverage Feb 7, 2020
Makefile.pre.in Outdated
@@ -513,7 +513,7 @@ profile-opt: profile-run-stamp
coverage:
@echo "Building with support for coverage checking:"
$(MAKE) clean
$(MAKE) @DEF_MAKE_RULE@ CFLAGS="$(CFLAGS) -O0 -pg -fprofile-arcs -ftest-coverage" LIBS="$(LIBS) -lgcov"
$(MAKE) @DEF_MAKE_RULE@ CFLAGS="$(CFLAGS) -O0 -pg -fprofile-arcs -ftest-coverage" LIBS="$(LIBS) --coverage"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we replace -fprofile-arcs -ftest-coverage in the cflags with --coverage, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but that does not provide any benefits. I slighty prefer writing explicit compiler flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it's beneficial because it expresses the desired result, coverage, rather than the implementation, profiling of edges.

(Do you pass -faggressive-loop-optimizations -falign-functions -falign-jumps -falign-labels -falign-loops -fassume-phsa -fasynchronous-unwind-tables -fauto-inc-dec -fbranch-count-reg -fcaller-saves -fcode-hoisting -fcombine-stack-adjustments -fcompare-elim -fcprop-registers -fcrossjumping -fcse-follow-jumps -fdce -fdefer-pop -fdelete-null-pointer-checks -fdevirtualize -fdevirtualize-speculatively -fdse -fearly-inlining -fexpensive-optimizations -fforward-propagate -ffp-int-builtin-inexact -ffunction-cse -fgcse -fgcse-lm -fguess-branch-probability -fhoist-adjacent-loads -fif-conversion -fif-conversion2 -findirect-inlining -finline -finline-atomics -finline-functions-called-once -finline-small-functions -fipa-bit-cp -fipa-cp -fipa-icf -fipa-icf-functions -fipa-icf-variables -fipa-profile -fipa-pure-const -fipa-ra -fipa-reference -fipa-reference-addressable -fipa-sra -fipa-stack-alignment -fipa-vrp -fira-hoist-pressure -fira-share-save-slots -fira-share-spill-slots -fisolate-erroneous-paths-dereference -fivopts -fjump-tables -flifetime-dse -flra-remat -fmath-errno -fmove-loop-invariants -fomit-frame-pointer -foptimize-sibling-calls -foptimize-strlen -fpartial-inlining -fpeephole -fpeephole2 -fplt -fprefetch-loop-arrays -fprintf-return-value -freg-struct-return -frename-registers -freorder-blocks -freorder-blocks-and-partition -freorder-functions -frerun-cse-after-loop -frtti -fsched-critical-path-heuristic -fsched-dep-count-heuristic -fsched-group-heuristic -fsched-interblock -fsched-last-insn-heuristic -fsched-rank-heuristic -fsched-spec -fsched-spec-insn-heuristic -fsched-stalled-insns-dep -fschedule-fusion -fschedule-insns2 -fshort-enums -fshrink-wrap -fshrink-wrap-separate -fsigned-zeros -fsplit-ivs-in-unroller -fsplit-wide-types -fssa-backprop -fssa-phiopt -fstdarg-opt -fstore-merging -fstrict-aliasing -fstrict-volatile-bitfields -fthread-jumps -fno-threadsafe-statics -ftrapping-math -ftree-bit-ccp -ftree-builtin-call-dce -ftree-ccp -ftree-ch -ftree-coalesce-vars -ftree-copy-prop -ftree-cselim -ftree-dce -ftree-dominator-opts -ftree-dse -ftree-forwprop -ftree-fre -ftree-loop-if-convert -ftree-loop-im -ftree-loop-ivcanon -ftree-loop-optimize -ftree-phiprop -ftree-pre -ftree-pta -ftree-reassoc -ftree-scev-cprop -ftree-sink -ftree-slsr -ftree-sra -ftree-switch-conversion -ftree-tail-merge -ftree-ter -ftree-vrp -funwind-tables -fvar-tracking -fvar-tracking-assignments -fweb instead of -O2?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed -fprofile-arcs -ftest-coverage to --coverage

This allows clang to get rid of the dependency on libgcov.
When linking, GCC passes -lgcov while clang passes the path to libclang_rt.profile-$arch.a
@codecov
Copy link

codecov bot commented Feb 7, 2020

Codecov Report

Merging #18382 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18382      +/-   ##
==========================================
- Coverage   82.11%   82.11%   -0.01%     
==========================================
  Files        1955     1954       -1     
  Lines      588904   583559    -5345     
  Branches    44429    44429              
==========================================
- Hits       483580   479190    -4390     
+ Misses      95673    94719     -954     
+ Partials     9651     9650       -1     
Impacted Files Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py 30.00% <0.00%> (-65.00%) ⬇️
Lib/distutils/command/bdist_rpm.py 7.63% <0.00%> (-56.88%) ⬇️
Lib/test/test_urllib2net.py 76.92% <0.00%> (-13.85%) ⬇️
Lib/test/test_smtpnet.py 78.57% <0.00%> (-7.15%) ⬇️
Lib/ftplib.py 63.85% <0.00%> (-6.06%) ⬇️
Lib/test/test_ftplib.py 87.11% <0.00%> (-4.72%) ⬇️
Tools/scripts/db2pickle.py 17.82% <0.00%> (-3.97%) ⬇️
Tools/scripts/pickle2db.py 16.98% <0.00%> (-3.78%) ⬇️
Lib/test/test_socket.py 71.94% <0.00%> (-3.77%) ⬇️
Lib/test/test_asyncio/test_base_events.py 91.84% <0.00%> (-3.30%) ⬇️
... and 328 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a102ed7...824e09e. Read the comment docs.

@benjaminp benjaminp merged commit 9a978dd into python:master Feb 7, 2020
@MaskRay MaskRay deleted the coverage branch February 8, 2020 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants