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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

On Tue, 2013-01-29 at 10:56 +0000, Ian Campbell wrote:
> 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).
> 

Easy to change. Actually I discovered that my patch does not even
compile if you disable TEST_COVERAGE (and it should be disabled by
default too). EINVAL say that the hypercall is present but the value
passed in invalid.

The reason why adding a new hypercall instead of a new sysctl is simply
because is easier to have a zero cost if you disable coverage
informations. The best thing would be redirect do_coverage_op to
do_ni_hypercall using linker options but even two small stub would do
(these stubs will return ENOSYS instead).

> > The idea is to have some operations mainly
> > - get filename
> 
> This is get a list of filenames (i.e. input for the next two)?
> 

The idea was just to pass the index and get a filename or a ENOENT error
(or a ENOBUF if buffer is too small). Actually you should scan the
entire list to get a proper filename so this end up in a O(N^2)
operation but as N is constant and not that big it should not be a
problem. If it's a problem I can implement a tree or simply reuse
constructor space to store a simple array of gcov_info*.

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

They cannot be so atomic anyway as the hypervisor is running, I'd not
use a lock or similar. These informations are mainly statistical one.
It's not possible to disable incrementing any counter and also the
increment is done without locking (so it's possible to loose some
number). Unless you pause all machines and stop any userspace program
that is doing some activities with Xen you'll have to consider some
dirty counter. Perhaps would be better to have an operation to
query/reset more filenames at a time. But even in Linux you have to
"cat" different files to get informations.

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

In this case arg1 if the buffer while arg2 would be mainly the length of
arg1 so they cannot be an union.

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

Well.. pass a large enough buffer and handle ENOBUF correctly.

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

You can inference quite a lot of useful (and possibly sensitive)
informations from these statistics so yes, beside should be disabled in
production systems I would limit strictly to privileged domains.

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

They are binary files. The format is defined by gcc. On userspace you
get a single .gcda file for each compile unit. This file is updated when
any program exit (multiple execution update the files). Gcc generate
also .gcno files. You need both .gcda and .gcno at the end to handle
coverage information.

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

The problem would be reading (or resetting) these counter if discarded,
probably would lead to some buffer overwriting which is not good.
Actually these modules have no such informations so it's not a problem.

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

Yes, I think it's safer to disable for them (as it actually).

Are TEST_COVERAGE a good names or would be better to have something like
CONFIG_ENABLE_COVERAGE (do you use any prefix for configure stuff?) ?

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

Yes, I added the two stubs (I'll send the update). init_coverage is not
a problem, header define as an empty static inline function if coverage
is disabled.

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

Well... I think constructor was implemented to support dynamic
initializers in C++ and other high level languages, something like:

int num = get_num(); /* not working on C */

They are used in C to handle initialization of shared objects (using
constructor attribute in gcc) but beside this I didn't see other uses.
The problem with C++ is that functions called in such way should not
make much assumption on other module initialization and are executed
before main(). At this point I don't know a proper use in Xen and when
to initialize them.

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

They are contained in Linux kernel so mainly GPL.
I think that however these header could lead to exception stated in
http://gcc.gnu.org/onlinedocs/libstdc++/manual/license.html so should
allow more uses than GPL.

> Are these interfaces subject to change with different gcc versions?
> 

The last updates was from gcc 3.4. They tend to not change but are not
set on stone. I hope that changes to these structure would lead the
program to not link correctly (for instance calling a __gcov_init2
instead of __gcov_init).

> Ian.
> 

Frediano

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