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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/10/16 14:41, George Dunlap wrote:
> On 12/10/16 14:34, Andrew Cooper wrote:
>> On 12/10/16 14:26, George Dunlap wrote:
>>> On 12/10/16 14:24, 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>>>> + */
>>>>>>> +
>>>>>>> +#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 mean software provider, not user, of course.  That's what I would want
>>> for CentOS, and I'm sure that's what the XenServer (and probably Oracle)
>>> guys want as well.
>> GCOV is majority invasive, both in terms of performance (every basic
>> block needs to do a locked increment of a counter) and data size
>> (metadata for all basic blocks).
>>
>> No software vendor is ever going to have it enabled in a production
>> scenario.
> You're right, I wasn't thinking.
>
> But also presumably you'd like to be able to minimize the difference
> between the thing you're testing and the thing you ship; having to
> disable LivePatch increases the delta slightly.
>
> Anyway, I still think having them both Kconfig-ured is a good idea, but
> not so much that I'd spend any more time arguing for it.

It is a testing feature.  It would certainly be nice to get code
coverage of the livepatching parts, but that absolutely shouldn't block
making gcov usable in the first place.

It might be worth sticking a #TODO make GCOV worth with Livepatching in
the kconfig file (for anyone who stumbles across this dependency).

~Andrew

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