[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-next 7/9] coverage: introduce support for llvm profiling
>>> On 26.10.17 at 11:19, <roger.pau@xxxxxxxxxx> wrote: > --- /dev/null > +++ b/xen/common/coverage/llvm.c > @@ -0,0 +1,148 @@ > +/* > + * Copyright (C) 2017 Citrix Systems R&D > + * All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#include <xen/errno.h> > +#include <xen/guest_access.h> > +#include <xen/types.h> > +#include <xen/coverage.h> > + > +#ifndef __clang__ > +#error "LLVM coverage selected without clang compiler" > +#endif > + > +#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. > +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). > +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. > +static int dump(XEN_GUEST_HANDLE_PARAM(char) buffer, uint32_t *buf_size) > +{ > + struct llvm_profile_header header = { > +#if BITS_PER_LONG == 64 > + .magic = LLVM_PROFILE_MAGIC_64, > +#else > + .magic = LLVM_PROFILE_MAGIC_32, > +#endif I think this should just be LLVM_PROFILE_MAGIC, with the #ifdef moved to the #define-s. > + .version = LLVM_PROFILE_VERSION, > + .data_size = (end_data - start_data) / sizeof(struct > llvm_profile_data), > + .counters_size = (end_counters - start_counters) / sizeof(uint64_t), > + .names_size = end_names - start_names, > + .counters_delta = (uintptr_t)start_counters, > + .names_delta = (uintptr_t)start_names, > + .value_kind_last = LLVM_PROFILE_NUM_KINDS - 1, > + }; > + unsigned int off = 0; > + > +#define APPEND_TO_BUFFER(src, size) \ > +({ \ > + if ( off + size > *buf_size ) \ (size) > + return -ENOMEM; \ > + copy_to_guest_offset(buffer, off, src, size); \ > + off += size; \ Ideally here too, albeit only the comma operator has lower precedence, and that would require parenthesizing in the macro invocation already (which is also why the arguments to copy_to_guest_offset() explicitly do not need extra parentheses added). > +}) > + APPEND_TO_BUFFER((char *)&header, sizeof(struct llvm_profile_header)); sizeof(header) > + 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. > +#undef APPEND_TO_BUFFER > + > + clear_guest_offset(buffer, off, *buf_size - off); > + > + return 0; > +} > + > +struct cov_sysctl_ops cov_ops = { const > --- 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)? Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |