Re: [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Oct 12, 2016 at 02:24:51PM +0100, George Dunlap wrote:
> On 12/10/16 14:06, Wei Liu wrote:
> > On Wed, Oct 12, 2016 at 06:42:53AM -0600, Jan Beulich wrote:
> >>>>> On 11.10.16 at 12:31, <wei.liu2@citrix.com> wrote:
> >>> --- /dev/null
> >>> +++ b/xen/common/gcov/gcc_4_7.c
> >>> @@ -0,0 +1,205 @@
> >>> +/*
> >>> + *  This code provides functions to handle gcc's profiling data format
> >>> + *  introduced with gcc 4.7.
> >>> + *
> >>> + *  This file is based heavily on gcc_3_4.c file.
> >>> + *
> >>> + *  For a better understanding, refer to gcc source:
> >>> + *  gcc/gcov-io.h
> >>> + *  libgcc/libgcov.c
> >>> + *
> >>> + *  Uses gcc-internal data definitions.
> >>> + *
> >>> + *  Imported from Linux and modified for Xen by
> >>> + *    Wei Liu <wei.liu2@citrix.com>
> >>> + */
> >>> +
> >>> +#include <xen/string.h>
> >>> +
> >>> +#include "gcov.h"
> >>> +
> >>> +#if GCC_VERSION < 40700
> >>> +#error "Wrong version of GCC used to compile gcov"
> >>> +#endif
> >>> +
> >>> +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1)
> >>> +#define GCOV_COUNTERS                   10
> >>> +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9
> >>> +#define GCOV_COUNTERS                   9
> >>> +#else
> >>> +#define GCOV_COUNTERS                   8
> >>> +#endif
> >>
> >> I'm sorry for not having pointed this out on v2 (I had noticed it,
> >> but then didn't finish analyzing the situation), but I'm afraid this
> >> together with ...
> >>
> >>> +struct gcov_info {
> >>> +    unsigned int version;
> >>> +    struct gcov_info *next;
> >>> +    unsigned int stamp;
> >>> +    const char *filename;
> >>> +    void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int);
> >>> +    unsigned int n_functions;
> >>> +    struct gcov_fn_info **functions;
> >>> +};
> >>
> >> ... this structure's trailing fields actually getting used by the code
> >> won't work well when changing compiler versions without cleaning
> >> the tree.  I think instead you need thin gcc_5.c and gcc_4_9.c
> >> #define-ing their GCOV_COUNTERS and then #include-ing this
> >> shared source file. Plus btw, I don't think gcc 5.0.x (the
> >> development variant of 5.x) would use anything different from
> >> 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not
> >> normally be necessary anymore with gcc 5+.
> >>
> > 
> > Right. I will do something about this. Thanks for catching this.
> > 
> >> And then - how is all of this supposed to be working in conjucntion
> >> with live patching, where the patch may have been created by yet
> >> another compiler version?
> >>
> > 
> > There is a version field in gcov_info, so we can compare that and reject
> > incompatible version.
> > 
> > We need to use hooks in livepatching to call the constructor /
> > destructor when applying / reverting a live-patch.  We might need to be
> > cautious about locks or whatever, but I'm sure it can be figured out.
> > 
> > But I have no idea how useful it would be to use gcov and livepatching
> > together.  For now the easiest thing to do is to
> > 
> >    depends on !LIVEPATCH
> > 
> > in Kconfig.
> 
> Wouldn't it be just as easy, and more useful, to set a "has been
> livepatched" flag, and return errors for all gcov hypercalls if its' set?
> 
> I would expect most users to want to build a single hypervisor that can
> be used for both gcov testing and live patching (under different
> circumstances).

I actually would welcome livepatching and gcov to see if the test-cases I wrote
cover most of the code.

Adding in checking livepatch (common/livepatch.c: prepare_payload) to examine
the .gcov_info and see if it matches the hypervisor one, is fine too.

> 
>  -George
> 

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