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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-01-28 at 21:16 +0000, Frediano Ziglio wrote:
> From: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>
> 
> 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?).

You can simply patch xen/include/public/xen.h to declare the new
__HYPERVISOR_foo_op. (which I now see you have done in this patch!). If
you just want to reserve the number it is also allowed to send just that
hunk to reserve a number pending the implementation.

BTW I'd suggest leaving the stub implementation out of the patch until
you've decided what it will look like, so it returns ENOSYS instead of
EINVAL (or change your stub to return ENOSYS).

> The idea is to have some operations mainly
> - get filename

This is get a list of filenames (i.e. input for the next two)?

> - get count informations for a file
> - reset counter for a specific file

How atomic do these need to be? Will it be necessary to obtain
consistent snapshots of the counters for multiple files? Even for a
single file I assume there will be many counters so getting a consistent
snapshot at the file level might even be tricky?

Or perhaps since you are measuring coverage you only care about one bit
per basic block (rather than a counter as such) and if you race then, oh
well, you'll see it next time.

> - reset all counters
> 
> 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.

Often the best way to deal with arg1/arg2 would be to pass a union of
the types specific to each operation.

But how does the caller determine the size of the buffer? I guess it
various from file to file? Is this what comes from "get count
informations for a file"?

Is this a dom0 only operation? Perhaps some sysctl subops would be
better than a whole new op?

> Do you think I should pack these parameters and pass a single pointer?
> 
> 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.

You could also synthesize this at the tools layer. I don't know enough
of how this stuff works to say what makes sense at the h/v layer.

What form does this information take internally?

> Do anybody know why I had to exclude %.init.o files to make it compile?
> Are these files used before Xen start?

They are used during start of day bring up and then discarded once Xen
is up and running. I'm not sure what that would affect the link,
although it would obviously be disastrous if you were to touch the
counters associated with those files again after they were thrown away.

We might have some checks in the tree to ensure that non-init code
cannot reference init code by mistake -- maybe that's what broke the
build for you?

> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>
> 
> diff -r 5af4f2ab06f3 Config.mk
> --- a/Config.mk Tue Jan 22 09:33:10 2013 +0100
> +++ b/Config.mk Mon Jan 28 17:36:14 2013 +0000
> @@ -228,3 +228,7 @@ QEMU_TAG ?= 2a1354d655d816feaad7dbdb8364
>  # doing and are prepared for some pain.
> 
>  CONFIG_TESTS       ?= y
> +
> +# Test coverage support
> +TEST_COVERAGE ?= y

Off by default I think.

> diff -r 5af4f2ab06f3 xen/arch/x86/x86_64/compat/entry.S
> --- a/xen/arch/x86/x86_64/compat/entry.S        Tue Jan 22 09:33:10 2013 +0100
> +++ b/xen/arch/x86/x86_64/compat/entry.S        Mon Jan 28 17:36:14 2013 +0000
> @@ -413,6 +413,8 @@ ENTRY(compat_hypercall_table)
>          .quad do_domctl
>          .quad compat_kexec_op
>          .quad do_tmem_op
> +        .quad compat_ni_hypercall
> +        .quad compat_coverage_op

This is in gcov.c which is only conditionally compiled, so you need a
stub for the case where coverage is not enabled. Same goes for the
non-compat call and init_coverage().

> +typedef void (*ctor_func_t)(void);
> +extern struct {
> +    unsigned long count;
> +    ctor_func_t funcs[1];
> +} __CTOR_LIST__;
> +
> +void __init init_coverage(void)
> +{
> +    unsigned long n;
> +    for (n = 0; n < __CTOR_LIST__.count; ++n)
> +        __CTOR_LIST__.funcs[n]();
> +
> +#ifndef NDEBUG
> +    printk(XENLOG_INFO "Initialized %u coverage strucures\n", num_info);
> +    if (info_list)
> +        printk(XENLOG_INFO "First coverage file %s\n", info_list->filename);
> +#endif
> +}

I think this could be a more generic run_ctors type function, its use as
part of the coverage stuff is a little bit incidental.

Unless for some reason we want to discourage the use of ctors for other
purposes?

> diff -r 5af4f2ab06f3 xen/include/xen/gcov.h
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/xen/include/xen/gcov.h    Mon Jan 28 17:36:14 2013 +0000
> @@ -0,0 +1,141 @@
> +/*
> + *  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.

Any idea what the license on these is?

Are these interfaces subject to change with different gcc versions?

Ian.


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