Re: [Qemu-devel] [PATCH for-1.4 qom-cpu 8/9 v6] target-i386: Topology & APIC ID utility functions

[Blue Swirl]

On Fri, Jan 25, 2013 at 7:51 PM, Eduardo Habkost <address@hidden> 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