Bug 10887 - config-parser.c is too big or complex (?) for gcov to cope with
Summary: config-parser.c is too big or complex (?) for gcov to cope with
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: All All
: low minor
Assignee: Simon McVittie
QA Contact: John (J5) Palmieri
URL: http://git.collabora.co.uk/?p=user/sm...
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2007-05-08 15:32 UTC by Bernard Leak
Modified: 2011-04-07 03:27 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Don't ignore all files in m4, just the libtool ones (550 bytes, patch)
2011-02-16 06:59 UTC, Simon McVittie
Details | Splinter Review
Import compiler.m4 and lcov.am from telepathy-glib, and use them to replace gcov (81.81 KB, patch)
2011-02-16 07:01 UTC, Simon McVittie
Details | Splinter Review
Break up the monster conditional in config-parser so gcov can cope (7.61 KB, patch)
2011-03-14 05:03 UTC, Simon McVittie
Details | Splinter Review

Description Bernard Leak 2007-05-08 15:32:55 UTC
The core of the test (wrapped up in a miniature C programme) is

if (__GNUC__ >=3 && __GNUC_MINOR__ >= 3) exit (0); else exit (1);

It fails for my 4.1.2 system, of course, but will un-fail again
when it reaches 4.3.  I imagine that at least one of these
behaviours is wrong... As it happens, I've not been able to
build with gcov enabled for a while (attempt to access private
symbols).  This may be because of something odd in my gcc build
(I doubt it), but there is SOMETHING wrong in there anyway.

Be that as it may, shouldn't failing the test mean that I DON'T build
with gcov, even if I asked for it?
Comment 1 Havoc Pennington 2007-05-08 17:44:31 UTC
I don't think the gcov stuff has worked in a long time, the format gcc produces changed once and I updated for it, then it changed at least once more and the gcov support was never updated to match.

The way the --enable options work is that the default is "auto" (enable if possible) and yes means fail if it can't be enabled, no means force disable.
The purpose of "yes" is to force the build to fail in e.g. a spec file if something goes wrong with the configure check.
Comment 2 Bernard Leak 2007-05-09 00:37:30 UTC
(In reply to comment #1)
> The purpose of "yes" is to force the build to fail in e.g. a spec file if
> something goes wrong with the configure check.

I think my wording above was correct, but perhaps too terse to be helpful.
I distinguish between configuration steps and build steps. I expected -
that is, felt entitled to insist - that the configuration step fail, given
that I didn't believe my gcov would work.  The configuration step completed
with no apparent errors: the failure came during the subsequent build.
Just what it shouldn't do...
Comment 3 Havoc Pennington 2007-05-09 09:51:17 UTC
Indeed the gcov check needs fixing. I'm not sure, to be honest, which gcc versions the gcov stuff currently in-tree works with, if any, though...

It may be better to just rip the gcov stuff out since I don't know that anyone will ever have time to fix it and since gcc doesn't seem to hold the format stable it's a lot of ongoing work to keep it working even if someone did fix it.

Comment 4 Simon McVittie 2011-01-05 05:52:57 UTC
FWIW, in dbus-glib I ripped out the gcov support (which hadn't worked since dbus-glib was separated from libdbus, as far as I could see) and replaced it with the lcov integration we use in Telepathy. It might be worth giving libdbus the same treatment.
Comment 5 Simon McVittie 2011-02-16 06:58:39 UTC
From the department of re-using other people's solutions, here's a branch to:

* throw away our unmaintained wrapper for gcov
* use lcov (someone else's maintained wrapper for gcov) to make HTML reports

I've found this to be very useful for feature development, to check which branches are covered by our tests.
Comment 6 Simon McVittie 2011-02-16 06:59:52 UTC
Created attachment 43429 [details] [review]
Don't ignore all files in m4, just the libtool ones

This patch seems safe for 1.4 too.
Comment 7 Simon McVittie 2011-02-16 07:01:25 UTC
Created attachment 43430 [details] [review]
Import compiler.m4 and lcov.am from telepathy-glib, and use them to replace gcov

We might want to avoid this one for 1.4 and apply it on master for 1.5, or other maintainers might consider it OK for 1.4; I'd be happy either way. It's most useful for feature development, which should be done on master rather than 1.4.
Comment 8 Simon McVittie 2011-02-16 07:06:54 UTC
Forgot to mention, there's a nasty workaround in the second patch:

> +# gcov takes ~forever to process config-parser.c for some reason?
> +lcov-report:
> +       true > bus/bus_test-config-parser.gcno
> +       true > bus/dbus_daemon-config-parser.gcno
...

However, it's still better than our gcov "support", which can't give us coverage stats for *anything* at the moment, afaics.

config-parser.c is nearly 100K, so it might just be too big for gcov to cope with. I'd be inclined to blame append_rule_from_element, which has a composite "if" conditional nearly 100 lines long...
Comment 9 Havoc Pennington 2011-02-16 13:52:58 UTC
fwiw I had some decent results with trucov for a code doodle I was working on lately:
https://github.com/havocp/hwf

(trucov is in deps/ and Makefile-test.am has a rule using it)

lcov is surely an improvement and I'd support putting it in, just thought I'd mention trucov too
Comment 10 Colin Walters 2011-02-17 08:21:41 UTC
Review of attachment 43429 [details] [review]:

Looks OK.
Comment 11 Colin Walters 2011-02-17 08:25:31 UTC
Review of attachment 43430 [details] [review]:

This is a big patch, but since it's just a developer tool (not critical path) and it comes from more used/tested code I'd say just put it in =)
Comment 12 Simon McVittie 2011-02-17 09:12:50 UTC
Comment on attachment 43430 [details] [review]
Import compiler.m4 and lcov.am from telepathy-glib, and use them to replace gcov

Patches applied for 1.4.4/1.5.0, but I'm leaving this bug open because we skip config-parser.c.
Comment 13 Simon McVittie 2011-03-14 05:03:21 UTC
Created attachment 44435 [details] [review]
Break up the monster conditional in config-parser so gcov can cope
Comment 14 Colin Walters 2011-03-23 08:28:22 UTC
Review of attachment 44435 [details] [review]:

Looks good to me!
Comment 15 Simon McVittie 2011-04-07 03:27:00 UTC
Fixed in git for 1.4.8, and I'll merge to master shortly.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.