Re: [PATCH for-4.8 2/2] Config.mk: fix comment for debug option

[Wei Liu <wei.liu2@citrix.com>]

On Tue, Nov 01, 2016 at 02:58:22PM +0000, Andrew Cooper wrote:
> On 01/11/16 13:47, Wei Liu wrote:
> > On Mon, Oct 31, 2016 at 05:09:46PM +0000, Wei Liu wrote:
> >> Signed-off-by: Wei Liu <wei.liu2@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>
> >> ---
> >>  Config.mk | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Config.mk b/Config.mk
> >> index ebbd9c0..fb836a4 100644
> >> --- a/Config.mk
> >> +++ b/Config.mk
> >> @@ -16,7 +16,8 @@ or       = $(if $(strip $(1)),$(1),$(if $(strip $(2)),$(2),$(if $(strip $(3)),$(
> >>  
> >>  -include $(XEN_ROOT)/.config
> >>  
> >> -# A debug build of Xen and tools?
> >> +# A debug build of tools?
> > For this to hold true, a patch like this is needed:
> >
> > Please let me know what you think.
> 
> Looks like another swamp :s
> 

Indeed. This is something we overlooked early in the release.

> >
> > ---8<---
> > From 0a96ff9f3610622bc4f7114d6e094bf45ca9305f Mon Sep 17 00:00:00 2001
> > From: Wei Liu <wei.liu2@citrix.com>
> > Date: Mon, 31 Oct 2016 17:42:25 +0000
> > Subject: [PATCH] build: make debug option affect tools only
> >
> > The debug option in Config.mk affects hypervisor, tools and stubdom by
> > appending different flags to CFLAGS.
> >
> > It is undesirable because now hypervisor build is affect by both Kconfig
> > and debug.
> >

^
Specifically because of this, I really want to fix this whole thing
properly. Otherwise it is going to be rather confusing to downstream.

> > Disentangle the semantics of debug by pushing relevant options to
> > individual sub-systems.
> >
> > For hypervisor, the flags previously added by debug option is now
> > controlled by CONFIG_DEBUG.
> >
> > For tools, flags are moved from config/*.mk into tools/Rules.mk.
> >
[...]
> > diff --git a/tools/Rules.mk b/tools/Rules.mk
> > index 5a80fec..0e73690 100644
> > --- a/tools/Rules.mk
> > +++ b/tools/Rules.mk
> > @@ -138,9 +138,11 @@ SHLIB_libxenvchan  = $(SHDEPS_libxenvchan) -Wl,-rpath-link=$(XEN_LIBVCHAN)
> >  
> >  ifeq ($(debug),y)
> >  # Disable optimizations and enable debugging information for macros
> > -CFLAGS += -O0 -g3
> > +CFLAGS += -O0 -g3 -fno-omit-frame-pointer
> 
> Perhaps this a suggestion better left for a later patch, but the use of
> -O0 is still bad here.
> 
> Debug builds should use -Og if available, and -O1 otherwise.  As
> identified immediately below, a number of options are incompatible with -O0.
> 
> >  # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=<n>.
> >  PY_CFLAGS += $(PY_NOOPT_CFLAGS)
> > +else
> > +CFLAGS += -O2 -fomit-frame-pointer
> >  endif
> >  
> >  LIBXL_BLKTAP ?= $(CONFIG_BLKTAP2)
> > diff --git a/xen/Rules.mk b/xen/Rules.mk
> > index a9fda71..08cc776 100644
> > --- a/xen/Rules.mk
> > +++ b/xen/Rules.mk
> > @@ -46,6 +46,12 @@ ALL_OBJS-y               += $(BASEDIR)/xsm/built_in.o
> >  ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
> >  ALL_OBJS-$(CONFIG_CRYPTO)   += $(BASEDIR)/crypto/built_in.o
> >  
> > +ifeq ($(CONFIG_DEBUG),y)
> > +CFLAGS += -O1
> > +else
> > +CFLAGS += -O2 -fomit-frame-pointer
> 
> The frame pointer option should be omitted entirely.  A user should be
> able to control debug and frame pointer entirely independently with Kconfig.
> 
> There have been two times where I have specifically needed a release
> build with frame pointers to track down bugs.
> 

I think both are fine suggestions.

I would like to (hopefully) not introduce noticeable changes of the
effect of all combined flags in this patch and adjust the flags in a
later patch.

Wei.

> ~Andrew

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