| Message ID | 20171026091938.59247-7-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote: > config GCOV > bool "Gcov Support" > depends on !LIVEPATCH && !LLVM_COVERAGE (will review in detail later)
On Thu, Oct 26, 2017 at 11:03:13AM +0100, Wei Liu wrote: > On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote: > > config GCOV > > bool "Gcov Support" > > depends on !LIVEPATCH > > && !LLVM_COVERAGE That was my idea, but sadly that's not possible because you generate a circular dependency. The best solution that I found is to only mark one as exclusive (ie: have depends !GCOV in the LLVM_COVERAGE option below). Thanks, Roger.
On Thu, Oct 26, 2017 at 11:08:21AM +0100, Roger Pau Monné wrote: > On Thu, Oct 26, 2017 at 11:03:13AM +0100, Wei Liu wrote: > > On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote: > > > config GCOV > > > bool "Gcov Support" > > > depends on !LIVEPATCH > > > > && !LLVM_COVERAGE > > That was my idea, but sadly that's not possible because you generate a > circular dependency. The best solution that I found is to only mark > one as exclusive (ie: have depends !GCOV in the LLVM_COVERAGE option > below). > In that case, why not just use "choice" to let user pick the implementation?
On 26/10/17 10:19, Roger Pau Monne wrote: > diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile > index 0e0510679e..e4541a1233 100644 > --- a/xen/common/coverage/Makefile > +++ b/xen/common/coverage/Makefile > @@ -1,3 +1,4 @@ > +ifeq ($(CONFIG_GCOV),y) > obj-y += gcov_base.o gcov.o coverage.o > obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o > obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o > @@ -9,3 +10,6 @@ obj-$(CONFIG_GCOV_FORMAT_AUTODETECT) += $(call cc-ifversion,lt,0x040700, \ > gcc_4_7.o, $(call cc-ifversion,lt,0x050000, \ > gcc_4_9.o, $(call cc-ifversion,lt,0x070000, \ > gcc_5.o, gcc_7.o)))) > +else > +obj-y += coverage.o > +endif How about obj-y += coverage.o ifeq ($(CONFIG_GCOV),y) ... endif seeing as coverage.o is common now? ~Andrew
>>> On 26.10.17 at 12:24, <andrew.cooper3@citrix.com> wrote: > On 26/10/17 10:19, Roger Pau Monne wrote: >> diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile >> index 0e0510679e..e4541a1233 100644 >> --- a/xen/common/coverage/Makefile >> +++ b/xen/common/coverage/Makefile >> @@ -1,3 +1,4 @@ >> +ifeq ($(CONFIG_GCOV),y) >> obj-y += gcov_base.o gcov.o coverage.o >> obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o >> obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o >> @@ -9,3 +10,6 @@ obj-$(CONFIG_GCOV_FORMAT_AUTODETECT) += $(call > cc-ifversion,lt,0x040700, \ >> gcc_4_7.o, $(call cc-ifversion,lt,0x050000, \ >> gcc_4_9.o, $(call cc-ifversion,lt,0x070000, \ >> gcc_5.o, gcc_7.o)))) >> +else >> +obj-y += coverage.o >> +endif > > How about > > obj-y += coverage.o > > ifeq ($(CONFIG_GCOV),y) > ... > endif Except please obj-$(CONFIG_GCOV) instead of ifeq. Jan
>>> On 26.10.17 at 12:10, <wei.liu2@citrix.com> wrote: > On Thu, Oct 26, 2017 at 11:08:21AM +0100, Roger Pau Monné wrote: >> On Thu, Oct 26, 2017 at 11:03:13AM +0100, Wei Liu wrote: >> > On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote: >> > > config GCOV >> > > bool "Gcov Support" >> > > depends on !LIVEPATCH >> > >> > && !LLVM_COVERAGE >> >> That was my idea, but sadly that's not possible because you generate a >> circular dependency. The best solution that I found is to only mark >> one as exclusive (ie: have depends !GCOV in the LLVM_COVERAGE option >> below). > > In that case, why not just use "choice" to let user pick the > implementation? Plus then this choice should probably have an auto-detect option just like gcov's gcc version one - at least I assume that it should be pretty straightforward to tell at build time which variant to use. In fact - what would happen if one enabled the wrong option for the compiler in use? IOW I wonder whether auto-detection (and then also for the gcc version) should be the only valid mode). Jan
>>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote: > --- a/xen/Kconfig.debug > +++ b/xen/Kconfig.debug > @@ -28,10 +28,17 @@ config FRAME_POINTER > maybe slower, but it gives very useful debugging information > in case of any Xen bugs. > > +# Hidden option enabled when either GCOV or LLVM coverage support is enabled. > +# This allows to enable the shared coverage bits in Xen without having to > +# check for each possible coverage implementation. > +config COVERAGE > + bool In case this is still needed after the conversion to "choice", the comment should be dropped - it is the very purpose of prompt-less options that is being described there. > --- a/xen/Rules.mk > +++ b/xen/Rules.mk > @@ -119,6 +119,10 @@ ifeq ($(CONFIG_GCOV),y) > $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fprofile-arcs -ftest-coverage > endif > > +ifeq ($(CONFIG_LLVM_COVERAGE),y) > +$(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fprofile-instr-generate -fcoverage-mapping > +endif The use of nogcov-y here suggests that the earlier patch abstracting away the "g" should be extended to remove that letter here, too. > --- a/xen/common/sysctl.c > +++ b/xen/common/sysctl.c > @@ -396,7 +396,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) > } > break; > > -#ifdef CONFIG_GCOV > +#ifdef CONFIG_COVERAGE > case XEN_SYSCTL_cov_op: > ret = sysctl_cov_op(&op->u.cov_op); > copyback = 1; Couldn't you better do away with the #ifdef altogether, by introducing a suitable inline function for the coverage-disabled case (returning -EOPNOTSUPP)? Jan
On Wed, Nov 08, 2017 at 01:13:29AM -0700, Jan Beulich wrote: > >>> On 26.10.17 at 12:10, <wei.liu2@citrix.com> wrote: > > On Thu, Oct 26, 2017 at 11:08:21AM +0100, Roger Pau Monné wrote: > >> On Thu, Oct 26, 2017 at 11:03:13AM +0100, Wei Liu wrote: > >> > On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote: > >> > > config GCOV > >> > > bool "Gcov Support" > >> > > depends on !LIVEPATCH > >> > > >> > && !LLVM_COVERAGE > >> > >> That was my idea, but sadly that's not possible because you generate a > >> circular dependency. The best solution that I found is to only mark > >> one as exclusive (ie: have depends !GCOV in the LLVM_COVERAGE option > >> below). > > > > In that case, why not just use "choice" to let user pick the > > implementation? > > Plus then this choice should probably have an auto-detect option > just like gcov's gcc version one - at least I assume that it should > be pretty straightforward to tell at build time which variant to use. > In fact - what would happen if one enabled the wrong option for > the compiler in use? IOW I wonder whether auto-detection (and > then also for the gcc version) should be the only valid mode). I don't plan to introduce llvm/clang version choices here, I think autodetection should be fine. I can remove the gcc ones also, leaving this as a boolean choice (ie: enable code coverage support), but I would like to have some confirmation from the gcc side. Thanks, Roger.
>>> On 08.11.17 at 09:49, <roger.pau@citrix.com> wrote: > On Wed, Nov 08, 2017 at 01:13:29AM -0700, Jan Beulich wrote: >> >>> On 26.10.17 at 12:10, <wei.liu2@citrix.com> wrote: >> > On Thu, Oct 26, 2017 at 11:08:21AM +0100, Roger Pau Monné wrote: >> >> On Thu, Oct 26, 2017 at 11:03:13AM +0100, Wei Liu wrote: >> >> > On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote: >> >> > > config GCOV >> >> > > bool "Gcov Support" >> >> > > depends on !LIVEPATCH >> >> > >> >> > && !LLVM_COVERAGE >> >> >> >> That was my idea, but sadly that's not possible because you generate a >> >> circular dependency. The best solution that I found is to only mark >> >> one as exclusive (ie: have depends !GCOV in the LLVM_COVERAGE option >> >> below). >> > >> > In that case, why not just use "choice" to let user pick the >> > implementation? >> >> Plus then this choice should probably have an auto-detect option >> just like gcov's gcc version one - at least I assume that it should >> be pretty straightforward to tell at build time which variant to use. >> In fact - what would happen if one enabled the wrong option for >> the compiler in use? IOW I wonder whether auto-detection (and >> then also for the gcc version) should be the only valid mode). > > I don't plan to introduce llvm/clang version choices here, I think > autodetection should be fine. And I didn't think about version choices for clang here anyway - the point really was just about gcc vs clang. > I can remove the gcc ones also, leaving this as a boolean choice (ie: > enable code coverage support), but I would like to have some > confirmation from the gcc side. So let's ask Wei: What was the reason for the Kconfig level version choice here in the first place? After all, if the wrong one is being selected, the build will fail afaict due to the #error directives in the version specific files. Jan
On Wed, Nov 08, 2017 at 04:07:35AM -0700, Jan Beulich wrote: > >>> On 08.11.17 at 09:49, <roger.pau@citrix.com> wrote: > > On Wed, Nov 08, 2017 at 01:13:29AM -0700, Jan Beulich wrote: > >> >>> On 26.10.17 at 12:10, <wei.liu2@citrix.com> wrote: > >> > On Thu, Oct 26, 2017 at 11:08:21AM +0100, Roger Pau Monné wrote: > >> >> On Thu, Oct 26, 2017 at 11:03:13AM +0100, Wei Liu wrote: > >> >> > On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote: > >> >> > > config GCOV > >> >> > > bool "Gcov Support" > >> >> > > depends on !LIVEPATCH > >> >> > > >> >> > && !LLVM_COVERAGE > >> >> > >> >> That was my idea, but sadly that's not possible because you generate a > >> >> circular dependency. The best solution that I found is to only mark > >> >> one as exclusive (ie: have depends !GCOV in the LLVM_COVERAGE option > >> >> below). > >> > > >> > In that case, why not just use "choice" to let user pick the > >> > implementation? > >> > >> Plus then this choice should probably have an auto-detect option > >> just like gcov's gcc version one - at least I assume that it should > >> be pretty straightforward to tell at build time which variant to use. > >> In fact - what would happen if one enabled the wrong option for > >> the compiler in use? IOW I wonder whether auto-detection (and > >> then also for the gcc version) should be the only valid mode). > > > > I don't plan to introduce llvm/clang version choices here, I think > > autodetection should be fine. > > And I didn't think about version choices for clang here anyway - > the point really was just about gcc vs clang. > > > I can remove the gcc ones also, leaving this as a boolean choice (ie: > > enable code coverage support), but I would like to have some > > confirmation from the gcc side. > > So let's ask Wei: What was the reason for the Kconfig level > version choice here in the first place? After all, if the wrong one > is being selected, the build will fail afaict due to the #error > directives in the version specific files. > The #error on versions was added in later iterations IIRC. Looking back I think it would make sense to just have autodetect.
diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug index 8d70f63743..46c72ea8bb 100644 --- a/xen/Kconfig.debug +++ b/xen/Kconfig.debug @@ -28,10 +28,17 @@ config FRAME_POINTER maybe slower, but it gives very useful debugging information in case of any Xen bugs. +# Hidden option enabled when either GCOV or LLVM coverage support is enabled. +# This allows to enable the shared coverage bits in Xen without having to +# check for each possible coverage implementation. +config COVERAGE + bool + config GCOV bool "Gcov Support" depends on !LIVEPATCH select SUPPRESS_DUPLICATE_SYMBOL_WARNINGS + select COVERAGE ---help--- Enable gcov (a test coverage program in GCC) support. @@ -83,6 +90,15 @@ config GCOV_FORMAT_3_4 endchoice +config LLVM_COVERAGE + bool "LLVM coverage support" + depends on !LIVEPATCH && !GCOV + select COVERAGE + ---help--- + Enable LLVM coverage support. + + If unsure, say N here. + config LOCK_PROFILE bool "Lock Profiling" ---help--- diff --git a/xen/Rules.mk b/xen/Rules.mk index 2659f8a4d1..076ba93185 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -119,6 +119,10 @@ ifeq ($(CONFIG_GCOV),y) $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fprofile-arcs -ftest-coverage endif +ifeq ($(CONFIG_LLVM_COVERAGE),y) +$(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fprofile-instr-generate -fcoverage-mapping +endif + ifeq ($(CONFIG_UBSAN),y) $(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined endif diff --git a/xen/common/Makefile b/xen/common/Makefile index ad181636f6..3a349f478b 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -74,7 +74,7 @@ tmem-y := tmem.o tmem_xen.o tmem_control.o tmem-$(CONFIG_COMPAT) += compat/tmem_xen.o obj-$(CONFIG_TMEM) += $(tmem-y) -subdir-$(CONFIG_GCOV) += coverage +subdir-$(CONFIG_COVERAGE) += coverage subdir-$(CONFIG_UBSAN) += ubsan subdir-y += libelf diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile index 0e0510679e..e4541a1233 100644 --- a/xen/common/coverage/Makefile +++ b/xen/common/coverage/Makefile @@ -1,3 +1,4 @@ +ifeq ($(CONFIG_GCOV),y) obj-y += gcov_base.o gcov.o coverage.o obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o @@ -9,3 +10,6 @@ obj-$(CONFIG_GCOV_FORMAT_AUTODETECT) += $(call cc-ifversion,lt,0x040700, \ gcc_4_7.o, $(call cc-ifversion,lt,0x050000, \ gcc_4_9.o, $(call cc-ifversion,lt,0x070000, \ gcc_5.o, gcc_7.o)))) +else +obj-y += coverage.o +endif diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c index da3e1246b1..e989b2711d 100644 --- a/xen/common/sysctl.c +++ b/xen/common/sysctl.c @@ -396,7 +396,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) } break; -#ifdef CONFIG_GCOV +#ifdef CONFIG_COVERAGE case XEN_SYSCTL_cov_op: ret = sysctl_cov_op(&op->u.cov_op); copyback = 1; diff --git a/xen/include/xen/coverage.h b/xen/include/xen/coverage.h index de400620bf..666bf624f9 100644 --- a/xen/include/xen/coverage.h +++ b/xen/include/xen/coverage.h @@ -1,7 +1,7 @@ #ifndef _XEN_GCOV_H #define _XEN_GCOV_H -#ifdef CONFIG_GCOV +#ifdef CONFIG_COVERAGE #include <public/sysctl.h> struct cov_sysctl_ops {
Just add the Kconfig option and modify the makefiles so the llvm coverage specific code can be added in a follow up patch. 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> --- xen/Kconfig.debug | 16 ++++++++++++++++ xen/Rules.mk | 4 ++++ xen/common/Makefile | 2 +- xen/common/coverage/Makefile | 4 ++++ xen/common/sysctl.c | 2 +- xen/include/xen/coverage.h | 2 +- 6 files changed, 27 insertions(+), 3 deletions(-)