| Message ID | 20171026091938.59247-8-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 26, 2017 at 10:19:36AM +0100, Roger Pau Monne wrote: > Introduce the functionality in order to fill the hooks of the > cov_sysctl_ops struct. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Tim Deegan <tim@xen.org> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: llvm-dev@lists.llvm.org > --- > Note that the file that contains the helpers is under a BSD 2-clause > license. This is done so it can be shared with other OSes that use the > llvm/clang compiler. It would be helpful if you can put a reference to the original code somewhere, either in commit message or in the comment section of the file. > --- > xen/common/coverage/Makefile | 2 +- > xen/common/coverage/llvm.c | 148 +++++++++++++++++++++++++++++++++++++++++++ > xen/include/public/sysctl.h | 6 ++ > 3 files changed, 155 insertions(+), 1 deletion(-) > create mode 100644 xen/common/coverage/llvm.c > > diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile > index e4541a1233..f2ffb2b8de 100644 > --- a/xen/common/coverage/Makefile > +++ b/xen/common/coverage/Makefile > @@ -11,5 +11,5 @@ obj-$(CONFIG_GCOV_FORMAT_AUTODETECT) += $(call cc-ifversion,lt,0x040700, \ > gcc_4_9.o, $(call cc-ifversion,lt,0x070000, \ > gcc_5.o, gcc_7.o)))) > else > -obj-y += coverage.o > +obj-y += llvm.o coverage.o > endif > diff --git a/xen/common/coverage/llvm.c b/xen/common/coverage/llvm.c > new file mode 100644 > index 0000000000..c8905070a3 > --- /dev/null > +++ b/xen/common/coverage/llvm.c > +#include <xen/errno.h> > +#include <xen/guest_access.h> > +#include <xen/types.h> > +#include <xen/coverage.h> > + Order.
>>> On 26.10.17 at 11:19, <roger.pau@citrix.com> 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
On Wed, Nov 08, 2017 at 01:38:59AM -0700, Jan Beulich wrote: > >>> On 26.10.17 at 11:19, <roger.pau@citrix.com> 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.
>>> On 08.11.17 at 09:56, <roger.pau@citrix.com> wrote: > On Wed, Nov 08, 2017 at 01:38:59AM -0700, Jan Beulich wrote: >> >>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote: >> > --- 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. Okay. Jan
- llvm-dev, since the code review can continue without that audience. vedant > On Nov 8, 2017, at 3:08 AM, Jan Beulich via llvm-dev <llvm-dev@lists.llvm.org> wrote: > >>>> On 08.11.17 at 09:56, <roger.pau@citrix.com> wrote: >> On Wed, Nov 08, 2017 at 01:38:59AM -0700, Jan Beulich wrote: >>>>>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote: >>>> --- 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. > > Okay. > > Jan > > _______________________________________________ > LLVM Developers mailing list > llvm-dev@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile index e4541a1233..f2ffb2b8de 100644 --- a/xen/common/coverage/Makefile +++ b/xen/common/coverage/Makefile @@ -11,5 +11,5 @@ obj-$(CONFIG_GCOV_FORMAT_AUTODETECT) += $(call cc-ifversion,lt,0x040700, \ gcc_4_9.o, $(call cc-ifversion,lt,0x070000, \ gcc_5.o, gcc_7.o)))) else -obj-y += coverage.o +obj-y += llvm.o coverage.o endif diff --git a/xen/common/coverage/llvm.c b/xen/common/coverage/llvm.c new file mode 100644 index 0000000000..c8905070a3 --- /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 + +#if __clang_major__ >= 4 || (__clang_major__ == 3 && __clang_minor__ == 9) +#define LLVM_PROFILE_VERSION 4 +#define LLVM_PROFILE_NUM_KINDS 2 +#else +#error "clang version not supported with coverage" +#endif + +struct llvm_profile_data { + uint64_t name_ref; + uint64_t function_hash; + void *counter; + void *function; + void *values; + uint32_t nr_counters; + uint16_t nr_value_sites[LLVM_PROFILE_NUM_KINDS]; +}; + +struct llvm_profile_header { + uint64_t magic; + uint64_t version; + uint64_t data_size; + uint64_t counters_size; + uint64_t names_size; + uint64_t counters_delta; + uint64_t names_delta; + uint64_t value_kind_last; +}; + +int __llvm_profile_runtime; + +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; + +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; + +static void reset_counters(void) +{ + memset(start_counters, 0, end_counters - start_counters); +} + +static uint32_t get_size(void) +{ + return ROUNDUP(sizeof(struct llvm_profile_header) + end_data - start_data + + end_counters - start_counters + end_names - start_names, 8); +} + +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 + .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 ) \ + return -ENOMEM; \ + copy_to_guest_offset(buffer, off, src, size); \ + off += size; \ +}) + APPEND_TO_BUFFER((char *)&header, sizeof(struct llvm_profile_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); +#undef APPEND_TO_BUFFER + + clear_guest_offset(buffer, off, *buf_size - off); + + return 0; +} + +struct cov_sysctl_ops cov_ops = { + .get_size = get_size, + .reset_counters = reset_counters, + .dump = dump, +}; + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 654b37cdce..62824b18f2 100644 --- 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 */ +/* + * Ouput format of LLVM coverage data is just a raw stream, as would be + * written by the compiler_rt run time library into a .profraw file. There + * are no special Xen tags or delimiters because none are needed. + */ + #define XEN_SYSCTL_COV_get_size 0 /* Get total size of output data */ #define XEN_SYSCTL_COV_read 1 /* Read output data */ #define XEN_SYSCTL_COV_reset 2 /* Reset all counters */
Introduce the functionality in order to fill the hooks of the cov_sysctl_ops struct. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> Cc: llvm-dev@lists.llvm.org --- Note that the file that contains the helpers is under a BSD 2-clause license. This is done so it can be shared with other OSes that use the llvm/clang compiler. --- xen/common/coverage/Makefile | 2 +- xen/common/coverage/llvm.c | 148 +++++++++++++++++++++++++++++++++++++++++++ xen/include/public/sysctl.h | 6 ++ 3 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 xen/common/coverage/llvm.c