Thread: [HACKERS] coverage analysis improvements
Here is a patch series with some significant reworking and adjusting of how the coverage analysis tools are run. The result should be that the "make coverage" runs are faster and more robust, the results are more accurate, and the code is simpler. Please see the individual patches for details. I have replaced "make coverage-html" with "make coverage" (but kept the previous name for compatibility), since the old meaning of the latter has gone away and was never very useful. Other than that, the usage of everything stays the same. I will add it to the commit fest. Testing with different versions of tools would be especially valuable. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
- 0001-Run-only-top-level-recursive-lcov.patch
- 0002-Have-lcov-exclude-external-files.patch
- 0003-Add-lcov-initial.patch
- 0004-Add-PostgreSQL-version-to-coverage-output.patch
- 0005-Remove-coverage-details-view.patch
- 0006-Run-coverage-commands-quietly.patch
- 0007-Improve-vpath-support-in-plperl-build.patch
- 0008-Support-coverage-on-vpath-builds.patch
- 0009-Exclude-flex-generated-code-from-coverage-testing.patch
On Fri, Aug 11, 2017 at 1:05 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Here is a patch series with some significant reworking and adjusting of > how the coverage analysis tools are run. The result should be that the > "make coverage" runs are faster and more robust, the results are more > accurate, and the code is simpler. Please see the individual patches > for details. > > I have replaced "make coverage-html" with "make coverage" (but kept the > previous name for compatibility), since the old meaning of the latter > has gone away and was never very useful. Other than that, the usage of > everything stays the same. > > I will add it to the commit fest. Testing with different versions of > tools would be especially valuable. Patch 0001 fails to apply as of c629324. Which versions of lcov and gcov did you use for your tests? -- Michael
On 8/21/17 01:23, Michael Paquier wrote: > Patch 0001 fails to apply as of c629324. Updated patches attached. > Which versions of lcov and gcov did you use for your tests? LCOV version 1.13, and gcc-7 and gcc-6 -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
- v2-0001-Run-only-top-level-recursive-lcov.patch
- v2-0002-Have-lcov-exclude-external-files.patch
- v2-0003-Add-lcov-initial.patch
- v2-0004-Add-PostgreSQL-version-to-coverage-output.patch
- v2-0005-Remove-coverage-details-view.patch
- v2-0006-Run-coverage-commands-quietly.patch
- v2-0007-Improve-vpath-support-in-plperl-build.patch
- v2-0008-Support-coverage-on-vpath-builds.patch
- v2-0009-Exclude-flex-generated-code-from-coverage-testing.patch
On Wed, Aug 23, 2017 at 3:47 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 8/21/17 01:23, Michael Paquier wrote: >> Patch 0001 fails to apply as of c629324. > > Updated patches attached. > >> Which versions of lcov and gcov did you use for your tests? > > LCOV version 1.13, and gcc-7 and gcc-6 LCOV can be compiled from here (I have for example just changed PREFIX in the main makefile): https://github.com/linux-test-project/lcov.gi And testing down to 1.11 I am not seeing issues with your patches. I have used gcc 7.1.1. Patch 0001 removes the .gcov files, which offer a text representation of the coverage. Sometimes I use that with a terminal... Not sure for the others, but that's my status on the matter. This also removes the target coverage. Please note that on some distributions, like, err... ArchLinux, lcov is not packaged in the core packages and it is necessary to make use of external sources (AUR). It would be nice to keep the original gcov calls as well, and keep the split between coverage-html and coverage. I think as well that html generate should be done only if lcov is found, and that text generation should be done only if gcov is used. It is annoying to see --enable-coverage fail because lcov only is missing, but it is not mandatory for coverage. Patches 2, 4, 5, 6 and 9 are good independent ideas. -- Michael
On 8/24/17 04:12, Michael Paquier wrote: > Patch 0001 removes the .gcov files, which offer a text representation > of the coverage. Sometimes I use that with a terminal... Not sure for > the others, but that's my status on the matter. This also removes the > target coverage. Please note that on some distributions, like, err... > ArchLinux, lcov is not packaged in the core packages and it is > necessary to make use of external sources (AUR). It would be nice to > keep the original gcov calls as well, and keep the split between > coverage-html and coverage. I think as well that html generate should > be done only if lcov is found, and that text generation should be done > only if gcov is used. It is annoying to see --enable-coverage fail > because lcov only is missing, but it is not mandatory for coverage. OK, I was not aware that people are using it that way. So updated patch set there, which separates coverage and coverage-html into two independent targets. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
- v3-0001-Run-only-top-level-recursive-lcov.patch
- v3-0002-Have-lcov-exclude-external-files.patch
- v3-0003-Add-lcov-initial.patch
- v3-0004-Add-PostgreSQL-version-to-coverage-output.patch
- v3-0005-Remove-coverage-details-view.patch
- v3-0006-Run-coverage-commands-quietly.patch
- v3-0007-Improve-vpath-support-in-plperl-build.patch
- v3-0008-Support-coverage-on-vpath-builds.patch
- v3-0009-Exclude-flex-generated-code-from-coverage-testing.patch
Re: [HACKERS] coverage analysis improvements
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
Hi Peter, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > OK, I was not aware that people are using it that way. So updated patch > set there, which separates coverage and coverage-html into two > independent targets. I have no opinion on the bulk of this patch set, but skimming it out of curiosity I noticed that the plperl change seems to have lost the dependency on plperl_helpers.h from the xsubpp targets: > diff --git a/src/pl/plperl/GNUmakefile b/src/pl/plperl/GNUmakefile > index 191f74067a..66a2c3d4c9 100644 > --- a/src/pl/plperl/GNUmakefile > +++ b/src/pl/plperl/GNUmakefile > @@ -81,13 +81,9 @@ perlchunks.h: $(PERLCHUNKS) > > all: all-lib > > -SPI.c: SPI.xs plperl_helpers.h > +%.c: %.xs > @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi > - $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $< >$@ > - > -Util.c: Util.xs plperl_helpers.h > - @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi > - $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $< >$@ > + $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap -output $@ $< > > > install: all install-lib install-data -- - Twitter seems more influential [than blogs] in the 'gets reported in the mainstream press' sense at least. - Matt McLeod - That'd be because the content of a tweet is easier to condense down to a mainstream media article. - Calle Dybedahl -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 9/20/17 13:13, Dagfinn Ilmari Mannsåker wrote: > I have no opinion on the bulk of this patch set, but skimming it out of > curiosity I noticed that the plperl change seems to have lost the > dependency on plperl_helpers.h from the xsubpp targets: Those commands don't actually require plperl_helpers.h, do they? >> diff --git a/src/pl/plperl/GNUmakefile b/src/pl/plperl/GNUmakefile >> index 191f74067a..66a2c3d4c9 100644 >> --- a/src/pl/plperl/GNUmakefile >> +++ b/src/pl/plperl/GNUmakefile >> @@ -81,13 +81,9 @@ perlchunks.h: $(PERLCHUNKS) >> >> all: all-lib >> >> -SPI.c: SPI.xs plperl_helpers.h >> +%.c: %.xs >> @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi >> - $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $< >$@ >> - >> -Util.c: Util.xs plperl_helpers.h >> - @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi >> - $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $< >$@ >> + $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap -output $@ $< -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Sep 21, 2017 at 1:48 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > OK, I was not aware that people are using it that way. At least one. > So updated patch > set there, which separates coverage and coverage-html into two > independent targets. Thanks for the new versions. Patches 4 and 5 could be merged. They step on each other's code paths. Some other things could be merged as well. coverage-html-stamp: lcov_base.info lcov_test.info rm -rf coverage - $(GENHTML) $(GENHTML_FLAGS) -o coverage --title='$(GENHTML_TITLE)' --num-spaces=4 --prefix='$(abs_top_srcdir)' $^ + $(GENHTML) $(GENHTML_FLAGS) -o coverage --title='$(GENHTML_TITLE)' --num-spaces=4 $^ touch $@ Actually this is very nice. I have been always enforcing abs_top_srcdir when using coverage stuff on things out of the main tree. -SPI.c: SPI.xs plperl_helpers.h +%.c: %.xs @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi - $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $< >$@ Doing coverage on plperl with this patch applied, those do not seem necessary. But I don't know enough this code to give a clear opinion. Running coverage-html with all the patches, I am seeing the following warnings with a fresh build on my macos laptop 10.11: geninfo: WARNING: gcov did not create any files for /Users/mpaquier/git/postgres/src/backend/access/transam/rmgr.gcda! I don't think that this is normal. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] coverage analysis improvements
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > On 9/20/17 13:13, Dagfinn Ilmari Mannsåker wrote: >> I have no opinion on the bulk of this patch set, but skimming it out of >> curiosity I noticed that the plperl change seems to have lost the >> dependency on plperl_helpers.h from the xsubpp targets: > > Those commands don't actually require plperl_helpers.h, do they? No, but the .xs files #include it. I guess it's SPI.o and Util.o that should depend on the header.. - ilmari -- "I use RMS as a guide in the same way that a boat captain would usea lighthouse. It's good to know where it is, but yougenerallydon't want to find yourself in the same spot." - Tollef Fog Heen -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 9/21/17 03:42, Michael Paquier wrote: > -SPI.c: SPI.xs plperl_helpers.h > +%.c: %.xs > @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch > --with-perl was not specified."; exit 1; fi > - $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap > $(perl_privlibexp)/ExtUtils/typemap $< >$@ > Doing coverage on plperl with this patch applied, those do not seem > necessary. But I don't know enough this code to give a clear opinion. That patch is necessary for doing make coverage in vpath builds. Otherwise it makes no difference. > Running coverage-html with all the patches, I am seeing the following > warnings with a fresh build on my macos laptop 10.11: > geninfo: WARNING: gcov did not create any files for > /Users/mpaquier/git/postgres/src/backend/access/transam/rmgr.gcda! > I don't think that this is normal. Apparently, rmgr.c doesn't contain any instrumentable code. I don't see this warning, but it might depend on tool versions and compiler options. Note that rmgr.c doesn't show up here either: https://coverage.postgresql.org/src/backend/access/transam/index.html -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Sep 22, 2017 at 11:34 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Apparently, rmgr.c doesn't contain any instrumentable code. I don't see > this warning, but it might depend on tool versions and compiler options. Even on HEAD I am seeing the same problem, this is not outlined just because the quiet mode of lcov is not used, but the warnings are there like in this report: https://www.postgresql.org/message-id/ec92eb95-26de-6da8-9862-ded3ff678c5c%40BlueTreble.com So please let me discard my concerns about this portion of the patch. Except for the plperl patch, I don't have more comments to offer about this patch set. It would be nice to make configure a bit smarter for lcov and gcov detection by not hard-failing if gcov can be found but not lcov. It is after all possible to run coverage without lcov, like on ArchLinux. Still that's a different request than what this patch set is doing, so this is not a blocker for this patch set. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 26, 2017 at 3:45 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > Except for the plperl patch, I don't have more comments to offer about > this patch set. It would be nice to make configure a bit smarter for > lcov and gcov detection by not hard-failing if gcov can be found but > not lcov. It is after all possible to run coverage without lcov, like > on ArchLinux. Still that's a different request than what this patch > set is doing, so this is not a blocker for this patch set. -SPI.c: SPI.xs plperl_helpers.h +%.c: %.xs @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi - $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $< >$@ - -Util.c: Util.xs plperl_helpers.h - @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi - $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $< >$@ + $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap -output $@ $< I just looked at the plperl portion of this patch, and I think that 01d83ffd has done some unnecessary things here. Contrary to perlchunks.h and plperl_opmask.h which are generated during the build, plperl_helpers.h is part of the source code so there is no meaning in having a dependency with it. The .c files do not need this header anyway, but their .o files would, still there is no need for that either as plperl_helpers.h will not go away. I am marking the full set of patches as ready for committer. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 9/27/17 01:52, Michael Paquier wrote: > I am marking the full set of patches as ready for committer. All these patches have now been committed. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers