Re: [HACKERS] coverage analysis improvements
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
Re: [HACKERS] coverage analysis improvements
On Tue, Sep 26, 2017 at 3:45 PM, Michael Paquierwrote: > 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
Re: [HACKERS] coverage analysis improvements
On Fri, Sep 22, 2017 at 11:34 PM, Peter Eisentrautwrote: > 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
Re: [HACKERS] coverage analysis improvements
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
Re: [HACKERS] coverage analysis improvements
Peter Eisentrautwrites: > 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 use a lighthouse. It's good to know where it is, but you generally don'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
Re: [HACKERS] coverage analysis improvements
On Thu, Sep 21, 2017 at 1:48 AM, Peter Eisentrautwrote: > 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
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
Re: [HACKERS] coverage analysis improvements
Hi Peter, Peter Eisentrautwrites: > 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
Re: [HACKERS] coverage analysis improvements
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 From 39969d232e9d137aadae55f67a45fa2306f43df9 Mon Sep 17 00:00:00 2001 From: Peter EisentrautDate: Thu, 10 Aug 2017 23:33:47 -0400 Subject: [PATCH v3 1/9] Run only top-level recursive lcov This is the way lcov was intended to be used. It is much faster and more robust and makes the makefiles simpler than running it in each subdirectory. The previous coding ran gcov before lcov, but that is useless because lcov/geninfo call gcov internally and use that information. Moreover, this led to complications and failures during parallel make. This separates the two targets: You either use "make coverage" to get textual output from gcov or "make coverage-html" to get an HTML report via lcov. (Using both is still problematic because they write the same output files.) --- .gitignore| 1 + doc/src/sgml/regress.sgml | 13 + src/Makefile.global.in| 28 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/.gitignore b/.gitignore index 4976fd9119..94e2c582f5 100644 --- a/.gitignore +++ b/.gitignore @@ -23,6 +23,7 @@ objfiles.txt *.gcov.out lcov.info coverage/ +coverage-html-stamp *.vcproj *.vcxproj win32ver.rc diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml index 7c2b1029c2..14747e5f3b 100644 --- a/doc/src/sgml/regress.sgml +++ b/doc/src/sgml/regress.sgml @@ -706,6 +706,19 @@ Test Coverage Examination The make commands also work in subdirectories. + +If you don't have lcov or prefer text output over an +HTML report, you can also run + +make coverage + +instead of make coverage-html, which will +produce .gcov output files for each source file +relevant to the test. (make coverage and make +coverage-html will overwrite each other's files, so mixing them +might be confusing.) + + To reset the execution counts between test runs, run: diff --git a/src/Makefile.global.in b/src/Makefile.global.in index fae8068150..f352ba20e2 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -874,25 +874,29 @@ endif # enable_nls ifeq ($(enable_coverage), yes) -# There is a strange interaction between lcov and existing .gcov -# output files. Hence the rm command and the ordering dependency. +# make coverage -- text output -gcda_files := $(wildcard *.gcda) +local_gcda_files = $(wildcard *.gcda) -lcov.info: $(gcda_files) - rm -f *.gcov .*.gcov - $(if $^,$(LCOV) -d . -c -o $@ $(LCOVFLAGS) --gcov-tool $(GCOV)) +coverage: $(local_gcda_files:.gcda=.c.gcov) -%.c.gcov: %.gcda | lcov.info +%.c.gcov: %.gcda $(GCOV) -b -f -p -o . $(GCOVFLAGS) $*.c >$*.c.gcov.out -coverage: $(gcda_files:.gcda=.c.gcov) lcov.info +# make coverage-html -- HTML output via lcov .PHONY: coverage-html -coverage-html: coverage +coverage-html: coverage-html-stamp + +coverage-html-stamp: lcov.info rm -rf coverage - mkdir coverage - $(GENHTML) --show-details --legend --output-directory=coverage --title=PostgreSQL --num-spaces=4 --prefix=$(abs_top_srcdir) `find . -name lcov.info -print` + $(GENHTML) --show-details --legend --output-directory=coverage --title=PostgreSQL --num-spaces=4 --prefix=$(abs_top_srcdir) $< + touch $@ + +all_gcda_files = $(shell find . -name '*.gcda' -print) + +lcov.info: $(all_gcda_files) + $(LCOV) -d . -c -o $@ $(LCOVFLAGS) --gcov-tool $(GCOV) # hook for clean-up @@ -900,7 +904,7 @@ clean distclean maintainer-clean: clean-coverage .PHONY: clean-coverage clean-coverage: - rm -rf coverage + rm -rf coverage coverage-html-stamp rm -f *.gcda *.gcno lcov.info *.gcov .*.gcov *.gcov.out base-commit: d42294fc00da4b97d04ddb4401b76295e8d86816 -- 2.14.1 From 48c874997f2714268ef2dbdf44d65ebca2a2d5a3 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Thu, 10
Re: [HACKERS] coverage analysis improvements
On Wed, Aug 23, 2017 at 3:47 AM, Peter Eisentrautwrote: > 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 -- 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
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 From 7b2f5087a2d0710e6a5c7ffa946dcfabd163f987 Mon Sep 17 00:00:00 2001 From: Peter EisentrautDate: Thu, 10 Aug 2017 23:33:47 -0400 Subject: [PATCH v2 1/9] Run only top-level recursive lcov This is the way lcov was intended to be used. It is much faster and more robust and makes the makefiles simpler than running it in each subdirectory. This also removes the direct gcov calls and lets lcov do it instead. The direct gcov calls are useless because lcov/geninfo call gcov internally and use that information. --- .gitignore| 3 +-- GNUmakefile.in| 2 +- doc/src/sgml/regress.sgml | 2 +- src/Makefile.global.in| 33 +++-- 4 files changed, 14 insertions(+), 26 deletions(-) diff --git a/.gitignore b/.gitignore index 4976fd9119..2052f719d1 100644 --- a/.gitignore +++ b/.gitignore @@ -19,10 +19,9 @@ objfiles.txt .deps/ *.gcno *.gcda -*.gcov -*.gcov.out lcov.info coverage/ +coverage-stamp *.vcproj *.vcxproj win32ver.rc diff --git a/GNUmakefile.in b/GNUmakefile.in index dc76a5d11d..8d77b01eea 100644 --- a/GNUmakefile.in +++ b/GNUmakefile.in @@ -41,7 +41,7 @@ install-world-contrib-recurse: install-world-src-recurse $(call recurse,installdirs uninstall init-po update-po,doc src config) -$(call recurse,distprep coverage,doc src config contrib) +$(call recurse,distprep,doc src config contrib) # clean, distclean, etc should apply to contrib too, even though # it's not built by default diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml index 7c2b1029c2..796cdc26ff 100644 --- a/doc/src/sgml/regress.sgml +++ b/doc/src/sgml/regress.sgml @@ -699,7 +699,7 @@ Test Coverage Examination ./configure --enable-coverage ... OTHER OPTIONS ... make make check # or other test suite -make coverage-html +make coverage Then point your HTML browser to coverage/index.html. diff --git a/src/Makefile.global.in b/src/Makefile.global.in index e8b3a519cb..e39ed884e7 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -19,7 +19,7 @@ # # Meta configuration -standard_targets = all install installdirs uninstall distprep clean distclean maintainer-clean coverage check installcheck init-po update-po +standard_targets = all install installdirs uninstall distprep clean distclean maintainer-clean check installcheck init-po update-po # these targets should recurse even into subdirectories not being built: standard_always_targets = distprep clean distclean maintainer-clean @@ -863,34 +863,23 @@ endif # enable_nls # (by gcc -ftest-coverage) # foo.gcda gcov data file, created when the program is run (for # programs compiled with gcc -fprofile-arcs) -# foo.c.gcov gcov output file with coverage information, created by -# gcov from foo.gcda (by "make coverage") -# foo.c.gcov.out stdout captured when foo.c.gcov is created, mildly -# interesting # lcov.info lcov tracefile, built from gcda files in one directory, # later collected by "make coverage-html" ifeq ($(enable_coverage), yes) -# There is a strange interaction between lcov and existing .gcov -# output files. Hence the rm command and the ordering dependency. - -gcda_files := $(wildcard *.gcda) +gcda_files = $(shell find . -name '*.gcda' -print) lcov.info: $(gcda_files) - rm -f *.gcov .*.gcov - $(if $^,$(LCOV) -d . -c -o $@ $(LCOVFLAGS) --gcov-tool $(GCOV)) - -%.c.gcov: %.gcda | lcov.info - $(GCOV) -b -f -p -o . $(GCOVFLAGS) $*.c >$*.c.gcov.out - -coverage: $(gcda_files:.gcda=.c.gcov) lcov.info + $(LCOV) -d . -c -o $@ $(LCOVFLAGS) --gcov-tool $(GCOV) -.PHONY: coverage-html -coverage-html: coverage +coverage-stamp: lcov.info rm -rf coverage - mkdir coverage - $(GENHTML) --show-details --legend --output-directory=coverage --title=PostgreSQL --num-spaces=4 --prefix=$(abs_top_srcdir) `find . -name lcov.info -print` + $(GENHTML) --show-details --legend --output-directory=coverage --title=PostgreSQL --num-spaces=4 --prefix=$(abs_top_srcdir) $< + touch $@ + +.PHONY: coverage coverage-html +coverage coverage-html: coverage-stamp # hook for clean-up @@ -898,8 +887,8 @@ clean distclean maintainer-clean: clean-coverage .PHONY: clean-coverage clean-coverage: - rm -rf coverage - rm -f *.gcda *.gcno lcov.info *.gcov .*.gcov *.gcov.out + rm -rf coverage/ coverage-stamp + rm -f *.gcda *.gcno lcov.info # User-callable target to reset counts between test runs base-commit: 2bfd1b1ee562c4e4fd065c7f7d1beaa9b9852070 -- 2.14.1 From
Re: [HACKERS] coverage analysis improvements
On Fri, Aug 11, 2017 at 1:05 PM, Peter Eisentrautwrote: > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers