Re: [HACKERS] code coverage patch
Michelle Caisse wrote: Thanks, I'll take a look at these issues. I have committed your patch with some rework that mainly addresses the concerns also found by Alvaro with regard to cleaning and dependency handling. I have renamed the out target to coverage-html, to be more in line with our other targets. So the workflow is now ./configure --enable-coverage make make check make coverage-html $BROWSER coverage/index.html There are a couple of platform-specific problems that I have come across: * Linux (Debian) works OK * FreeBSD build fails, apparently because libgcov.a was not compiled with -fPIC * Mac OS X builds and runs OK but the server does not shut down in finite time at the end of the regression tests; it has to be killed manually. I have uploaded an example run here: http://developer.postgresql.org/~petere/coverage/ Current test coverage is about 66% overall. -- 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] code coverage patch
Peter Eisentraut [EMAIL PROTECTED] writes: I have uploaded an example run here: http://developer.postgresql.org/~petere/coverage/ Current test coverage is about 66% overall. With some pretty glaring gaps: 0% coverage of geqo, 0% coverage of logtape which implies no tuplesorts are spilling to disk, no coverage of mark/restore on index scans... -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Get trained by Bruce Momjian - ask me about EnterpriseDB's PostgreSQL training! -- 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] code coverage patch
Gregory Stark wrote: Peter Eisentraut [EMAIL PROTECTED] writes: I have uploaded an example run here: http://developer.postgresql.org/~petere/coverage/ Current test coverage is about 66% overall. With some pretty glaring gaps: 0% coverage of geqo, 0% coverage of logtape which implies no tuplesorts are spilling to disk, no coverage of mark/restore on index scans... Yah, that kinda shocked me too. Clearly we should spend some effort to expand the regression tests a bit. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- 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] code coverage patch
Michelle Caisse wrote: I've attached a patch that allows the generation of code coverage statistics. To test it, apply the patch, then: autoconf ./configure --enable-coverage make make check (or execute any other application against the database to see the coverage of that app) make coverage make coverage_out make clean does not work for me; it doesn't remove the .gcda and .gcno files. Apparently the problem is that $(enable_coverage) is not defined, so that part of common.mk is not called. Note: one thing to keep in mind is directories like src/port. There are some .gcda and .gcno files in there too, but even if common.mk is fixed, they will not be cleaned because src/port does not seem to use common.mk. Another thing that I'm unsure about is the coverage_out target. It does work, but is it running the coverage target each time it is invoked? Because if so, it's removing all of ./coverage and creating it again ... is this the desired behavior? This patch is missing a installation.sgml patch, at a minimum. I think it would be useful to mention that we support gcov, and the make targets we have, in some other part of the documentation. I can't readily find a good spot, but I think a new file to be inserted in the internals chapter would be appropriate. Two hunks no longer apply, but that's OK because they were working around a problem that no longer exists. Other than the minor gripes above, the patch looks OK to me. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- 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] code coverage patch
Thanks, I'll take a look at these issues. -- Michelle Alvaro Herrera wrote: Michelle Caisse wrote: I've attached a patch that allows the generation of code coverage statistics. To test it, apply the patch, then: autoconf ./configure --enable-coverage make make check (or execute any other application against the database to see the coverage of that app) make coverage make coverage_out make clean does not work for me; it doesn't remove the .gcda and .gcno files. Apparently the problem is that $(enable_coverage) is not defined, so that part of common.mk is not called. Note: one thing to keep in mind is directories like src/port. There are some .gcda and .gcno files in there too, but even if common.mk is fixed, they will not be cleaned because src/port does not seem to use common.mk. Another thing that I'm unsure about is the coverage_out target. It does work, but is it running the coverage target each time it is invoked? Because if so, it's removing all of ./coverage and creating it again ... is this the desired behavior? This patch is missing a installation.sgml patch, at a minimum. I think it would be useful to mention that we support gcov, and the make targets we have, in some other part of the documentation. I can't readily find a good spot, but I think a new file to be inserted in the internals chapter would be appropriate. Two hunks no longer apply, but that's OK because they were working around a problem that no longer exists. Other than the minor gripes above, the patch looks OK to me. -- Michelle Caisse Sun Microsystems California, U.S. http://sun.com/postgresql
Re: [HACKERS] code coverage patch
Peter Eisentraut wrote: Michelle Caisse wrote: gcov gets confused when source files are generated. I eliminated src/backend/bootstrap and ../parser from coverage analysis to avoid errors of this type. The problem with those files is that the source file contains lines like this: #line 1042 y.tab.c but that source file does not exist, as it is renamed to gram.c. This problem is now fixed, so the workaround in the coverage patch can be dropped. -- 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] code coverage patch
Michelle Caisse wrote: gcov gets confused when source files are generated. I eliminated src/backend/bootstrap and ../parser from coverage analysis to avoid errors of this type. The problem with those files is that the source file contains lines like this: #line 1042 y.tab.c but that source file does not exist, as it is renamed to gram.c. We could fix that in one of two ways: 1) Use bison's -o option to put the output file in the right place directly, if we are dealing with bison (and don't bother to support code coverage analysis with other yaccs), or 2) Run a pattern replacement across the grammar output files as their are renamed. Comments? -- 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] code coverage patch
Michelle Caisse wrote: Also, two tests fail with the following diff when the build is configured with --enable-coverage. RETURNS trigger AS '/home/michelle/trunkClean/pgsql/src/test/regress/../../../contrib/spi/refi nt.so' LANGUAGE C; + ERROR: could not load library /home/michelle/trunkClean/pgsql/src/test/regress/../../../contrib/spi/refi nt.so: /home/michelle/trunkClean/pgsql/src/test/regress/../../../contrib/spi/refin t.so: undefined symbol: __gcov_merge_add The reason for that problem is that the shared object needs to be linked with -fprofile-arcs -ftest-coverage. (One of these causes -lgcov to be linked, which includes the missing symbol.) This is not done because the shared object link rules don't use CFLAGS. I think for most platforms it would actually be more correct to use CFLAGS in linking. There may be the odd exception, but usually CFLAGS contains some assortment of -O, -g, -W, and maybe -f options, which should do more good than harm when linking. Any concerns about selectively adding CFLAGS to shared library linking rules? -- 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] code coverage patch
Peter Eisentraut [EMAIL PROTECTED] writes: The reason for that problem is that the shared object needs to be linked with -fprofile-arcs -ftest-coverage. (One of these causes -lgcov to be linked, which includes the missing symbol.) This is not done because the shared object link rules don't use CFLAGS. Shared object link rules should use another variable (LDFLAGS?) and those options should be added that variable as well. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Get trained by Bruce Momjian - ask me about EnterpriseDB's PostgreSQL training! -- 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] code coverage patch
Gregory Stark wrote: Peter Eisentraut [EMAIL PROTECTED] writes: The reason for that problem is that the shared object needs to be linked with -fprofile-arcs -ftest-coverage. (One of these causes -lgcov to be linked, which includes the missing symbol.) This is not done because the shared object link rules don't use CFLAGS. Shared object link rules should use another variable (LDFLAGS?) and those options should be added that variable as well. When linking executables, we already use both CFLAGS and LDFLAGS. This is the standard way in the GNU-enabled world. And it does exactly the right thing in this gcov case. If we invented another variable, we would disrupt that system and would further differentiate between different types of linking, while we should ultimately aim to make it less different. -- 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] code coverage patch
The problem with those files is that the source file contains lines like this: #line 1042 y.tab.c but that source file does not exist, as it is renamed to gram.c. We could fix that in one of two ways: 1) Use bison's -o option to put the output file in the right place directly, if we are dealing with bison (and don't bother to support code coverage analysis with other yaccs), or 2) Run a pattern replacement across the grammar output files as their are renamed. Why not use the %output directive in the grammar file instead; that way you don't need to add any special flags to the Makefile. -- Korry -- 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] code coverage patch
Korry Douglas wrote: 1) Use bison's -o option to put the output file in the right place directly, if we are dealing with bison (and don't bother to support code coverage analysis with other yaccs), or 2) Run a pattern replacement across the grammar output files as their are renamed. Why not use the %output directive in the grammar file instead; that way you don't need to add any special flags to the Makefile. I think only bison supports that directive. -- 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] code coverage patch
Peter Eisentraut [EMAIL PROTECTED] writes: Korry Douglas wrote: Why not use the %output directive in the grammar file instead; that way you don't need to add any special flags to the Makefile. I think only bison supports that directive. We're pretty much assuming bison anyway, no? It's been years since I heard of anyone successfully building the backend grammar with plain yacc. regards, tom lane -- 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] code coverage patch
Tom Lane wrote: We're pretty much assuming bison anyway, no? It's been years since I heard of anyone successfully building the backend grammar with plain yacc. In my recollection, you were the last holdout on that with the occasional HP-UX yacc test. But I seem to recall that that combination actually no longer worked the last time. If no one else has any interest in maintaining other-yacc support, then we might as well drop it. -- 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] code coverage patch
Peter Eisentraut [EMAIL PROTECTED] writes: Tom Lane wrote: We're pretty much assuming bison anyway, no? It's been years since I heard of anyone successfully building the backend grammar with plain yacc. In my recollection, you were the last holdout on that with the occasional HP-UX yacc test. But I seem to recall that that combination actually no longer worked the last time. I don't think I've tried that in this century ;-). Between the sheer size of the grammar and the fact that we're already depending on the behavior of several arcane %-options, I really doubt any tool besides bison will work. Besides, the whole point of shipping the built files in tarballs is to ensure no one has to use any other tool. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
