Re: [Xen-devel] [PATCH 2/4] Adding support for coverage information

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

On Mon, 2013-02-04 at 09:39 +0000, Jan Beulich wrote:
> >>> On 01.02.13 at 15:48, Frediano Ziglio <frediano.ziglio@xxxxxxxxxx> wrote:
> > --- a/Config.mk
> > +++ b/Config.mk
> > @@ -228,3 +228,7 @@ QEMU_TAG ?= 2a1354d655d816feaad7dbdb8364f40a208439c1
> >  # doing and are prepared for some pain.
> >  
> >  CONFIG_TESTS       ?= y
> > +
> > +# Test coverage support
> > +coverage ?= n
> > +
> 
> Alongside debug and debug_symbols please.

Ok.

> > --- a/xen/Rules.mk
> > +++ b/xen/Rules.mk
> > @@ -103,6 +103,11 @@ subdir-all := $(subdir-y) $(subdir-n)
> >  
> >  $(filter %.init.o,$(obj-y) $(obj-bin-y)): CFLAGS += -DINIT_SECTIONS_ONLY
> >  
> > +
> 
> Stray blank line.
> 
> > +ifeq ($(coverage),y)
> > +$(filter-out %.init.o,$(obj-y) $(obj-bin-y)): CFLAGS += -fprofile-arcs 
> > -ftest-coverage -DTEST_COVERAGE
> > +endif
> 
> For one - isn't simply using $(obj-y) here sufficient (i.e. without the
> $(filter-out ...))?
> 
> And second, I would thing this then ought to become a single line:
> 
> $(obj-$(coverage)): CFLAGS += -fprofile-arcs -ftest-coverage -DTEST_COVERAGE
> 

Yes, it works fine and seems to compile all proper files but I don't
understand why.
What's the difference between $(obj-bin-y) and $(obj-y) ??

> > --- /dev/null
> > +++ b/xen/common/gcov/Makefile
> > @@ -0,0 +1,5 @@
> > +ifneq ($(coverage),y)
> > +obj-y += nogcov.o
> > +endif
> > +obj-$(coverage) += gcov.o
> > +
> 
> How about
> 
> obj-y := nogcov.o
> obj-$(coverage) := gcov.o
> 

Using sysctl is even simpler, just 

subdir-$(coverage) += gcov

in common/Makefile and

obj-y += gcov.o

in common/gcov/Makefile

> 
> > +typedef void (*ctor_func_t)(void);
> > +extern struct
> > +{
> > +    unsigned long count;
> > +    ctor_func_t funcs[1];
> > +} __CTOR_LIST__;
> 
> const?
> 

Fine.

> > +
> > +void init_coverage(void)
> 
> __init
> 

I removed from previous and put in the header as suggestion.

> > --- /dev/null
> > +++ b/xen/include/public/gcov.h
> > @@ -0,0 +1,93 @@
> > +/*
> > + *  Profiling infrastructure declarations.
> > + *
> > + *  This file is based on gcc-internal definitions. Data structures are
> > + *  defined to be compatible with gcc counterparts. For a better
> > + *  understanding, refer to gcc source: gcc/gcov-io.h.
> > + *
> > + *    Copyright IBM Corp. 2009
> > + *    Author(s): Peter Oberparleiter <oberpar@xxxxxxxxxxxxxxxxxx>
> > + *
> > + *    Uses gcc-internal data definitions.
> > + */
> > +
> > +#ifndef XEN_PUBLIC_GCOV_H
> > +#define XEN_PUBLIC_GCOV_H XEN_PUBLIC_GCOV_H
> > +
> > +/*
> > + * Profiling data types used for gcc 3.4 and above - these are defined by
> > + * gcc and need to be kept as close to the original definition as possible 
> > to
> > + * remain compatible.
> > + */
> > +#define GCOV_COUNTERS         5
> > +#define GCOV_DATA_MAGIC       ((unsigned int) 0x67636461)
> > +#define GCOV_TAG_FUNCTION     ((unsigned int) 0x01000000)
> > +#define GCOV_TAG_COUNTER_BASE ((unsigned int) 0x01a10000)
> > +#define GCOV_TAG_FOR_COUNTER(count) \
> > +    (GCOV_TAG_COUNTER_BASE + ((unsigned int) (count) << 17))
> > +
> > +#if BITS_PER_LONG >= 64
> > +typedef long gcov_type;
> > +#else
> > +typedef long long gcov_type;
> > +#endif
> 
> What's this???
> 

This came from Linux, probably for Xen a simple

typedef long gcov_type

is ok.

> > +/**
> > + * struct gcov_ctr_info - profiling data per counter type
> > + * @num: number of counter values for this type
> > + * @values: array of counter values for this type
> > + * @merge: merge function for counter values of this type (unused)
> > + *
> > + * This data is generated by gcc during compilation and doesn't change
> > + * at run-time with the exception of the values array.
> > + */
> > +struct gcov_ctr_info
> > +{
> > +    unsigned int num;
> > +    gcov_type *values;
> > +    void (*merge)(gcov_type *, unsigned int);
> > +};
> 
> Pointers, and even more so function ones, can't be part of the
> public interface.
> 

Yes, probably there is no need to expose this to public.

Probably defining only the exported blob would be ok and have no licence
issues.

> Jan
> 

Frediano

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel