On Fri, Jan 25, 2013 at 7:51 PM, Eduardo Habkost <ehabk...@redhat.com> wrote: > On Fri, Jan 25, 2013 at 06:27:47PM +0000, Blue Swirl wrote: > [...] >> >> > [1] If I set gcov-files-test-x86-cpuid-y = target-i386/topology.h, I >> >> > get: >> >> > >> >> > GTESTER tests/test-x86-cpuid >> >> > Gcov report for target-i386/topology.h: >> >> > target-i386/topology.gcno:cannot open graph file >> >> > >> >> > It looks like the .gcno file generation is per-object-file, not >> >> > per-source-file >> >> > (gcov-files-*-y being a list of .c files confused me). If that's the >> >> > case, then >> >> > the only valid value for gcov-files-test-x86-cpuid-y is really >> >> > tests/test-x86-cpuid.c, because all the tested code is being compiled >> >> > inside >> >> > tests/test-x86-cpuid.o. >> >> >> >> But we are not interested in the coverage of the test suite code. >> >> >> >> Perhaps the design of gcov is not so well thought out and it is not >> >> very suitable for this kind of files. >> > >> > I agree it is not perfect, but including test-x86-cpuid.c in the list is >> > the only way to get a report of the topology.h test coverage. Do you see >> > any reason to not do it? >> >> The statistics for topology.h would be slightly biased by >> test-x86-cpuid.c, meaning that even if test-x86-cpuid.c didn't test >> anything but still gained some coverage, the some total coverage would >> be reported. Also the opposite case could decrease 100% test coverage. > > Good point. I believe it is more than "slightly biased", as all the > functions in topology.h are static inline: any code that wouldn't be > covered by the test generate any machine code at all. > > I quickly tested this by adding extra functions to topology.h without > adding any test code, and coverage report was still 100%, as the > functions are inline and didn't generated any machine code in > test-x86-cpuid.o. That makes the topology.h coverage statistics useless > and misleading.
This supports my view that gcov is not useful for all cases. Maybe this could work: Change topology.h to not inline always: #ifdef QTEST #define TOPOLOGY_H_INLINE #else #define TOPOLOGY_H_INLINE inline #endif and use TOPOLOGY_H_INLINE instead of 'inline'. Then introduce topology.c: #define QTEST #include "topology.h" >> >> But I think this would only matter if we made a huge effort to reach >> 100% coverage for whole of QEMU and the opposite case somehow became >> the only block keeping us from reaching that 100%. Until that day >> comes (in the late 2050s perhaps, or it could be never if new code is >> always added faster than coverage is gained) slightly biased >> statistics are much better than no statistics at all. > > In the topology.h case I would call it "completely useless" statistics, > as any percentage reported for that file would mean nothing. I prefer to > not have any coverage statistics from that file at all, than having > misleading statistics. > > So I am OK with simply removing gcov-files-test-x86-cpuid-y from my > patch. OK. > The only thing that gcov-files-test-x86-cpuid-y does is to make "make > check" print the (useless) percentages. Coverage reports can still be > generated manually by simply running "gcov tests/test-x86-cpuid.gcno" if > somebody wants to see the line-by-line coverage report. > > -- > Eduardo
