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 6:01 PM, Eduardo Habkost <address@hidden> wrote:
> On Fri, Jan 25, 2013 at 05:47:46PM +0000, Blue Swirl wrote:
>> On Thu, Jan 24, 2013 at 12:07 PM, Eduardo Habkost <address@hidden> wrote:
>> > On Wed, Jan 23, 2013 at 08:49:58PM +0100, Andreas Färber wrote:
>> >> Am 23.01.2013 18:58, schrieb Eduardo Habkost:
> [...]
>> >> > diff --git a/tests/Makefile b/tests/Makefile
>> >> > index d86e95a..4b98d4f 100644
>> >> > --- a/tests/Makefile
>> >> > +++ b/tests/Makefile
>> >> > @@ -45,6 +45,10 @@ gcov-files-test-aio-$(CONFIG_WIN32) = aio-win32.c
>> >> >  gcov-files-test-aio-$(CONFIG_POSIX) = aio-posix.c
>> >> >  check-unit-y += tests/test-thread-pool$(EXESUF)
>> >> >  gcov-files-test-thread-pool-y = thread-pool.c
>> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
>> >> > +# all code tested by test-x86-cpuid is inside topology.h,
>> >> > +# so add the test file itself to the gcov list
>> >> > +gcov-files-test-x86-cpuid-y = tests/test-x86-cpuid.c
>> >> >
>> >> >  check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
>> >> >
>> >>
>> >> With patch 7/9 dropped I am more comfortable with the test integration.
>> >>
>> >> I wonder however whether the gcov line is correct - won't this screw up
>> >> the statistics so that it's better to drop that line and to add
>> >> hw/pc_piix.c or target-i386/cpu.c in 9/9 instead? Blue?
>> >
>> > I want to make gcov check for coverage only of topology.h (that's where the
>> > tested code lives). Including test-x86-cpuid.c is the closest I could get 
>> > to
>> > that[1]. Including pc_piix.c or cpu.c would surely screw up the numbers, 
>> > as the
>> > tests don't cover any of the pc_piix.c or target-i386/cpu.c code.
>>
>> In my first version of the gcov patch, the statistics were gathered
>> for all tests, but that did not work well since there were double
>> counts due to coverage from an emulator run and standalone test case
>> runs.
>
> I see. Thanks for the explanation.
>
>
>> >
>> > [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.

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.

>
> --
> Eduardo