Re: [Xen-devel] [RFC PATCH] Adding support for coverage informations

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

On Tue, 2013-01-29 at 10:41 +0000, Jan Beulich wrote:
> >>> On 28.01.13 at 22:16, Frediano Ziglio <frediano.ziglio@xxxxxxxxxx> wrote:
> > This patch introduce coverage support to Xen.
> > Currently it allows to compile Xen with coverage support but there is no way
> > to extract them.
> > 
> > The declarations came from Linux source files (as you can see from file
> > headers).
> > 
> > It also add a new hypercall (can somebody reserve a number for this stuff?).
> > 
> > The idea is to have some operations mainly
> > - get filename
> > - get count informations for a file
> > - reset counter for a specific file
> > - reset all counters
> 
> The description you give doesn't really tell me what all this will
> really buy us. And considering that the patch enables this by
> default, I think I'd prefer to know...
> 

This was a mistake, coverage should be disabled by default. Compiler add
an extra increment instruction for every basic block.

The purpose of coverage is usually for testing to detect which part of
code are not well tested. It's used also to optimize code doing
profiling that could be fed up again to compiler to get more optimized
code.

The different with profile (oprof) is that oprof is more statistical and
does not count every line.

> > The op parameter in the hypercall will be the operation.
> > The idx parameter is the index to the file.
> > The arg1 is the argument (a buffer pointer for filename or counters).
> > The arg2 probably will be the size of previous buffer.
> > 
> > Do you think I should pack these parameters and pass a single pointer?
> 
> I don't see a need.
> 
> > Linux use a file system to export these information. I would use the same
> > format as Linux use so to make it easy to reuse tools from Linux (like 
> > lcov).
> > That the reason why I split get filename from get counter informations.
> > 
> > These informations cannot be put in a specific section (allowing a safe 
> > mapping)
> > as gcc use .rodata, .data, .text and .ctors sections.
> 
> What are you trying to tell us here?
> 

If gcc emitted code/data for coverage in particular section we could
pack these structures together and map them to userspace instead of
having to use hypercalls to read them.

> > I added code to handle constructors used in this case to initialize a linked
> > list of files.
> > 
> > Do anybody know why I had to exclude %.init.o files to make it compile?
> 
> Without you telling us what problems not doing so causes, no.
> 

Xen didn't compile. In xen/Rules.mk there is code that change if
*.init.o do not contain some sections (text/data/bss).

> > Are these files used before Xen start?
> 
> No, they're containing code that's being used only during boot
> (when whole files are init-only, we can also move the e.g. string
> literals into .init.* sections).
> 
> > --- a/xen/arch/x86/xen.lds.S    Tue Jan 22 09:33:10 2013 +0100
> > +++ b/xen/arch/x86/xen.lds.S    Mon Jan 28 17:36:14 2013 +0000
> > @@ -78,7 +78,23 @@ SECTIONS
> >         *(.data)
> >         *(.data.rel)
> >         *(.data.rel.*)
> > -       CONSTRUCTORS
> > +#ifdef __x86_64__
> > +        . = ALIGN(8);
> > +        __CTOR_LIST__ = .;
> > +        QUAD((__CTOR_END__ - __CTOR_LIST__) / 8 - 2)
> > +        *(.ctors)
> > +        CONSTRUCTORS
> 
> This doesn't look right: Even if CONSTRUCTORS is presumably
> producing nothing right now, you change its treatment by moving
> it inside the __CTOR_{LIST,END}__ range. Linux has
> CONSTRUCTORS and *(.ctors) separate too, and (validly) puts
> .ctors inside the .init.data section.
> 

Yes, putting in .init.data is fine too. Actually these lines came from
linker manual.

> > +        QUAD(0)
> > +        __CTOR_END__ = .;
> > +#else
> > +        . = ALIGN(4);
> > +        __CTOR_LIST__ = .;
> > +        LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
> > +        *(.ctors)
> > +        CONSTRUCTORS
> > +        LONG(0)
> > +        __CTOR_END__ = .;
> > +#endif
> 
> What case is this #else portion trying to address?
> 

A case (32 bit) which is no more supported by Xen, I'll remove it.

> > --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> > +++ b/xen/common/gcov/gcov.c    Mon Jan 28 17:36:14 2013 +0000
> 
> Please decide for a consistent coding style in the newly added
> files - either all Linux or all Xen, but not a mixture of both.
> 

This file is copied verbatim from Linux cause it require few changes.
I think is better to convert everything to Xen style. Other parts will
require more changes.

> > +#ifdef CONFIG_COMPAT
> > +int compat_coverage_op(int op, int idx, XEN_GUEST_HANDLE_PARAM(void) uarg, 
> > int arg2)
> > +{
> > +    return -EINVAL;
> > +}
> > +#endif
> 
> By suitably defining the interface structures you can avoid the
> need for a separate compat handler.
> 

How to do it?

> > +#ifdef TEST_COVERAGE
> > +void __init init_coverage(void);
> 
> No __init annotations in declarations please (they only belong on
> the actual definition).
> 

Ok

> Jan
> 

Frediano

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