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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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...

> 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?

> 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.

> 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.

> +        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?

> --- /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.

> +#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.

> +#ifdef TEST_COVERAGE
> +void __init init_coverage(void);

No __init annotations in declarations please (they only belong on
the actual definition).

Jan


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