Re: [Xen-devel] [PATCH for-next 7/9] coverage: introduce support for llvm profiling

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Nov 08, 2017 at 01:38:59AM -0700, Jan Beulich wrote:
> >>> On 26.10.17 at 11:19, <roger.pau@xxxxxxxxxx> wrote:
> > --- /dev/null
> > +++ b/xen/common/coverage/llvm.c
> > +#define LLVM_PROFILE_MAGIC_64 (uint64_t)255 << 56 | (uint64_t)'l' << 48 | \
> > +       (uint64_t)'p' << 40 | (uint64_t)'r' << 32 | (uint64_t)'o' << 24 |  \
> > +        (uint64_t)'f' << 16 | (uint64_t)'r' << 8 | (uint64_t)129
> > +#define LLVM_PROFILE_MAGIC_32 (uint64_t)255 << 56 | (uint64_t)'l' << 48 | \
> > +       (uint64_t)'p' << 40 | (uint64_t)'r' << 32 | (uint64_t)'o' << 24 |  \
> > +        (uint64_t)'f' << 16 | (uint64_t)'R' << 8 | (uint64_t)129
> 
> Judging by the file having Xen style, I imply this isn't intended to
> very closely match some other original. With that, parentheses
> should be added around all the shift operations above.
> 
> > +#if __clang_major__ >= 4 || (__clang_major__ == 3 && __clang_minor__ == 9)
> 
> Is it certain that there's never going to be a 3.10? Otherwise
> >= might be more suitable for the minor version check.

No, there's not going to be a clang 3.10.

> > +int __llvm_profile_runtime;
> 
> This isn't used anywhere - please add a brief comment explaining
> the need for it (the more that its type being plain int is also
> suspicious).

This is an internal symbol that must be present because Xen is not
linked against the run-time coverage library. It's described as an int
here:

https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#using-the-profiling-runtime-without-static-initializers

Without this symbol linkage fails.

> > +extern struct llvm_profile_data __start___llvm_prf_data;
> > +extern struct llvm_profile_data __stop___llvm_prf_data;
> > +extern uint64_t __start___llvm_prf_cnts;
> > +extern uint64_t __stop___llvm_prf_cnts;
> > +extern char __start___llvm_prf_names;
> > +extern char __stop___llvm_prf_names;
> 
> I guess all of these really want to have [] added, making ...
> 
> > +static void *start_data = &__start___llvm_prf_data;
> > +static void *end_data = &__stop___llvm_prf_data;
> > +static void *start_counters = &__start___llvm_prf_cnts;
> > +static void *end_counters = &__stop___llvm_prf_cnts;
> > +static void *start_names = &__start___llvm_prf_names;
> > +static void *end_names = &__stop___llvm_prf_names;
> 
> ... the &s here unnecessary. But then - do these really need to
> be statics (rather than #define-s)?
> 
> I would also guess that at least the names ones could be const.

Could make them defines indeed.

> > +    APPEND_TO_BUFFER((char *)start_data, end_data - start_data);
> > +    APPEND_TO_BUFFER((char *)start_counters, end_counters - 
> > start_counters);
> > +    APPEND_TO_BUFFER((char *)start_names, end_names - start_names);
> 
> The casts should all be to const char*, and perhaps that would
> better be done inside the macro anyway.

Seems better indeed.

> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -646,6 +646,12 @@ struct xen_sysctl_scheduler_op {
> >  
> >  #define XEN_GCOV_FORMAT_MAGIC    0x58434f56 /* XCOV */
> 
> Hmm, shouldn't the private magic #define-s actually be put here
> (in which case you'd indeed need to retain both 32- and 64-bit
> variants)?

I don't think so, here XEN_GCOV_FORMAT_MAGIC is a Xen specific gcov
magic number.

OTOH LLVM_PROFILE_MAGIC_{64/32} is an llvm defined magic number,
that's not under our control. Hence I don't think it should be
exported in Xen public headers.

Roger.

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