Re: [Xen-devel] [PATCH v3 4/7] kconfig/gcov: rename to coverage

[Wei Liu]

On Wed, Jan 24, 2018 at 11:35:51AM +, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH v3 4/7] kconfig/gcov: rename to coverage"):
> > On Wed, Jan 24, 2018 at 11:27:22AM +, Ian Jackson wrote:
> > > This hunk seems erroneous ?  Surely the name should be updated, but
> > > the #ifdef should remain.
> > 
> > No the ifdef needs to go.
> > 
> > There is a new stub introduced in this patch.
> 
> Oh, hrm.  Yes, there is.  But why is this better ?
> 

It is not better or worse but that's how Roger managed to return
-EOPNOTSUPP I assume.

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 4/7] kconfig/gcov: rename to coverage

[Ian Jackson]

Wei Liu writes ("Re: [PATCH v3 4/7] kconfig/gcov: rename to coverage"):
> On Wed, Jan 24, 2018 at 11:27:22AM +, Ian Jackson wrote:
> > This hunk seems erroneous ?  Surely the name should be updated, but
> > the #ifdef should remain.
> 
> No the ifdef needs to go.
> 
> There is a new stub introduced in this patch.

Oh, hrm.  Yes, there is.  But why is this better ?

One reason for all this Kconfig stuff is to be able to disable unused
interfaces for security reasons.  That also means being able to easily
audit what interfaces are enabled, which is a lot simpler if you can
read the main switch statement for the relevant hypercall, than if you
have to go chasing off after definitions.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 4/7] kconfig/gcov: rename to coverage

[Wei Liu]

On Wed, Jan 24, 2018 at 11:27:22AM +, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v3 4/7] kconfig/gcov: rename to coverage"):
> > So it can be used by both gcc and clang. Just add the Kconfig option
> > and modify the makefiles so the llvm coverage specific code can be
> > added in a follow up patch.
> ...
> > diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> > index f2ae6295ff..8e83c33a16 100644
> > --- a/xen/common/sysctl.c
> > +++ b/xen/common/sysctl.c
> > @@ -396,12 +396,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> > u_sysctl)
> >  }
> >  break;
> >  
> > -#ifdef CONFIG_GCOV
> >  case XEN_SYSCTL_coverage_op:
> >  ret = sysctl_cov_op(>u.coverage_op);
> >  copyback = 1;
> >  break;
> > -#endif
> >  
> 
> This hunk seems erroneous ?  Surely the name should be updated, but
> the #ifdef should remain.
> 

No the ifdef needs to go.

There is a new stub introduced in this patch.

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 4/7] kconfig/gcov: rename to coverage

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v3 4/7] kconfig/gcov: rename to coverage"):
> So it can be used by both gcc and clang. Just add the Kconfig option
> and modify the makefiles so the llvm coverage specific code can be
> added in a follow up patch.
...
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index f2ae6295ff..8e83c33a16 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -396,12 +396,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>  }
>  break;
>  
> -#ifdef CONFIG_GCOV
>  case XEN_SYSCTL_coverage_op:
>  ret = sysctl_cov_op(>u.coverage_op);
>  copyback = 1;
>  break;
> -#endif
>  

This hunk seems erroneous ?  Surely the name should be updated, but
the #ifdef should remain.

Thanks,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 4/7] kconfig/gcov: rename to coverage

[Wei Liu]

On Wed, Jan 24, 2018 at 10:01:22AM +, Roger Pau Monne wrote:
> So it can be used by both gcc and clang. 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é 
> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index 3cf40754a6..0bc40fd0e6 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -115,8 +115,13 @@ subdir-all := $(subdir-y) $(subdir-n)
>  
>  $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += 
> -DINIT_SECTIONS_ONLY
>  
> -ifeq ($(CONFIG_GCOV),y)
> -$(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS 
> += -fprofile-arcs -ftest-coverage
> +ifeq ($(CONFIG_COVERAGE),y)
> +ifeq ($(clang),y)
> +COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
> +else
> +COV_FLAGS := -fprofile-arcs -ftest-coverage
> +endif
> +$(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): 
> CFLAGS += $(COV_FLAGS)

Please also change nogcov to nocov. Also remove the leading spaces from
this line.

Other than this, this patch looks good.

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel