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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.

 -George

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