| Message ID | 20161101134720.GZ30231@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > > ---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. > > 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. > > For stubdom, it is a bit special because it unilaterally sets debug all > the time, and it also inherits CFLAGS from the source package it tries > to build. It should be fine to not inherit any flags from Xen build > system because they will be overridden by source packages anyway. > > Specifically there are some considerations on how the flags are picked: > > 1. we don't need -fno-optimize-sibling-calls anymore because gcc doc > indicates that it is not enabled for -O1, which we already set in the > debug build. > 2. for tools we use -O0 -g3 in Rules.mk because they already take > precedence over the flags set in config/*.mk. > 3. for hypervisor we don't add -fno-omit-frame-pointer to debug build > because that's controlled by CONFIG_FRAME_POINTER. > > The debug option in Config.mk will only affect tools components after > this patch is applied. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > config/StdGNU.mk | 8 -------- > config/SunOS.mk | 7 ------- > tools/Rules.mk | 4 +++- > xen/Rules.mk | 6 ++++++ > 4 files changed, 9 insertions(+), 16 deletions(-) > > diff --git a/config/StdGNU.mk b/config/StdGNU.mk > index 39d36b2..6be8233 100644 > --- a/config/StdGNU.mk > +++ b/config/StdGNU.mk > @@ -35,14 +35,6 @@ UTIL_LIBS = -lutil > SONAME_LDFLAG = -soname > SHLIB_LDFLAGS = -shared > > -ifneq ($(debug),y) > -CFLAGS += -O2 -fomit-frame-pointer > -else > -# Less than -O1 produces bad code and large stack frames > -CFLAGS += -O1 -fno-omit-frame-pointer > -CFLAGS-$(gcc) += -fno-optimize-sibling-calls > -endif > - > ifeq ($(lto),y) > CFLAGS += -flto > LDFLAGS-$(clang) += -plugin LLVMgold.so > diff --git a/config/SunOS.mk b/config/SunOS.mk > index 86a384d..0fe5f45 100644 > --- a/config/SunOS.mk > +++ b/config/SunOS.mk > @@ -31,12 +31,5 @@ UTIL_LIBS = > SONAME_LDFLAG = -h > SHLIB_LDFLAGS = -R $(SunOS_LIBDIR) -shared > > -ifneq ($(debug),y) > -CFLAGS += -O2 -fno-omit-frame-pointer > -else > -# Less than -O1 produces bad code and large stack frames > -CFLAGS += -O1 -fno-omit-frame-pointer > -endif > - > CFLAGS += -Wa,--divide -D_POSIX_C_SOURCE=200112L -D__EXTENSIONS__ > > 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. ~Andrew
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
>>> Wei Liu <wei.liu2@citrix.com> 11/01/16 2:48 PM >>> >config/StdGNU.mk | 8 -------- >config/SunOS.mk | 7 ------- >tools/Rules.mk | 4 +++- >xen/Rules.mk | 6 ++++++ >4 files changed, 9 insertions(+), 16 deletions(-) Considering this diffstat - did the original global settings not get inherited by any of the other subtrees, namely stubdom/ and/or extras/ ? Jan
On Tue, Nov 01, 2016 at 10:18:46AM -0600, Jan Beulich wrote: > >>> Wei Liu <wei.liu2@citrix.com> 11/01/16 2:48 PM >>> > >config/StdGNU.mk | 8 -------- > >config/SunOS.mk | 7 ------- > >tools/Rules.mk | 4 +++- > >xen/Rules.mk | 6 ++++++ > >4 files changed, 9 insertions(+), 16 deletions(-) > > Considering this diffstat - did the original global settings not get inherited > by any of the other subtrees, namely stubdom/ and/or extras/ ? > I mentioned this in commit message and deliberately made the decision to not inherit these flags for stubdom build. Wei. > Jan >
>>> Wei Liu <wei.liu2@citrix.com> 11/01/16 5:20 PM >>> >On Tue, Nov 01, 2016 at 10:18:46AM -0600, Jan Beulich wrote: >> >>> Wei Liu <wei.liu2@citrix.com> 11/01/16 2:48 PM >>> >> >config/StdGNU.mk | 8 -------- >> >config/SunOS.mk | 7 ------- >> >tools/Rules.mk | 4 +++- >> >xen/Rules.mk | 6 ++++++ >> >4 files changed, 9 insertions(+), 16 deletions(-) >> >> Considering this diffstat - did the original global settings not get inherited >> by any of the other subtrees, namely stubdom/ and/or extras/ ? > >I mentioned this in commit message and deliberately made the decision to >not inherit these flags for stubdom build. Indeed, I've overlooked that part, but I'm not fully convinced. And then extras/ and possible other subtrees don't get mentioned (at the very least I'd expect a sentence clarifying that no other subtrees are affected, but as said at least for extras/ I'm not sure: That's where mini-os gets linked in, and I don't recall whether its build machinery got fully separated). Jan
On Tue, Nov 01, 2016 at 10:33:54AM -0600, Jan Beulich wrote: > >>> Wei Liu <wei.liu2@citrix.com> 11/01/16 5:20 PM >>> > >On Tue, Nov 01, 2016 at 10:18:46AM -0600, Jan Beulich wrote: > >> >>> Wei Liu <wei.liu2@citrix.com> 11/01/16 2:48 PM >>> > >> >config/StdGNU.mk | 8 -------- > >> >config/SunOS.mk | 7 ------- > >> >tools/Rules.mk | 4 +++- > >> >xen/Rules.mk | 6 ++++++ > >> >4 files changed, 9 insertions(+), 16 deletions(-) > >> > >> Considering this diffstat - did the original global settings not get inherited > >> by any of the other subtrees, namely stubdom/ and/or extras/ ? > > > >I mentioned this in commit message and deliberately made the decision to > >not inherit these flags for stubdom build. > > Indeed, I've overlooked that part, but I'm not fully convinced. And then Not fully convinced because? Can you be more specific? > extras/ and possible other subtrees don't get mentioned (at the very > least I'd expect a sentence clarifying that no other subtrees are > affected, but as said at least for extras/ I'm not sure: That's where > mini-os gets linked in, and I don't recall whether its build machinery > got fully separated). > Mini-os got its own build system after I separated it out from Xen.git. It should be dealt with separately if necessary. I can mention that in commit message as well. Wei. > Jan >
>>> Wei Liu <wei.liu2@citrix.com> 11/01/16 5:38 PM >>> >On Tue, Nov 01, 2016 at 10:33:54AM -0600, Jan Beulich wrote: >> >>> Wei Liu <wei.liu2@citrix.com> 11/01/16 5:20 PM >>> >> >On Tue, Nov 01, 2016 at 10:18:46AM -0600, Jan Beulich wrote: >> >> >>> Wei Liu <wei.liu2@citrix.com> 11/01/16 2:48 PM >>> >> >> >config/StdGNU.mk | 8 -------- >> >> >config/SunOS.mk | 7 ------- >> >> >tools/Rules.mk | 4 +++- >> >> >xen/Rules.mk | 6 ++++++ >> >> >4 files changed, 9 insertions(+), 16 deletions(-) >> >> >> >> Considering this diffstat - did the original global settings not get inherited >> >> by any of the other subtrees, namely stubdom/ and/or extras/ ? >> > >> >I mentioned this in commit message and deliberately made the decision to >> >not inherit these flags for stubdom build. >> >> Indeed, I've overlooked that part, but I'm not fully convinced. And then > >Not fully convinced because? Can you be more specific? I don't know very much about the stubdom build machinery, but I could easily imaging some top level setting to have made it through to some of the sub-components, in which case this change would have a functional impact. Jan
On Tue, Nov 01, 2016 at 10:42:37AM -0600, Jan Beulich wrote: > >>> Wei Liu <wei.liu2@citrix.com> 11/01/16 5:38 PM >>> > >On Tue, Nov 01, 2016 at 10:33:54AM -0600, Jan Beulich wrote: > >> >>> Wei Liu <wei.liu2@citrix.com> 11/01/16 5:20 PM >>> > >> >On Tue, Nov 01, 2016 at 10:18:46AM -0600, Jan Beulich wrote: > >> >> >>> Wei Liu <wei.liu2@citrix.com> 11/01/16 2:48 PM >>> > >> >> >config/StdGNU.mk | 8 -------- > >> >> >config/SunOS.mk | 7 ------- > >> >> >tools/Rules.mk | 4 +++- > >> >> >xen/Rules.mk | 6 ++++++ > >> >> >4 files changed, 9 insertions(+), 16 deletions(-) > >> >> > >> >> Considering this diffstat - did the original global settings not get inherited > >> >> by any of the other subtrees, namely stubdom/ and/or extras/ ? > >> > > >> >I mentioned this in commit message and deliberately made the decision to > >> >not inherit these flags for stubdom build. > >> > >> Indeed, I've overlooked that part, but I'm not fully convinced. And then > > > >Not fully convinced because? Can you be more specific? > > I don't know very much about the stubdom build machinery, but I could > easily imaging some top level setting to have made it through to some > of the sub-components, in which case this change would have a > functional impact. > I don't feel strongly about this. I can move some flags into stubdom build system as well, just to err on the safe side. Wei. > Jan >
Wei Liu writes ("Re: [PATCH for-4.8 2/2] Config.mk: fix comment for debug option"):
> On Mon, Oct 31, 2016 at 05:09:46PM +0000, Wei Liu wrote:
> > -# 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.
...
> For hypervisor, the flags previously added by debug option is now
> controlled by CONFIG_DEBUG.
So there will no longer be one place where debug is enabled. That's a
shame. It would need to come with an osstest patch...
Ian.
On 01/11/16 16:56, Ian Jackson wrote: > Wei Liu writes ("Re: [PATCH for-4.8 2/2] Config.mk: fix comment for debug option"): >> On Mon, Oct 31, 2016 at 05:09:46PM +0000, Wei Liu wrote: >>> -# 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. > ... >> For hypervisor, the flags previously added by debug option is now >> controlled by CONFIG_DEBUG. > So there will no longer be one place where debug is enabled. That's a > shame. It would need to come with an osstest patch... It hasn't been the case for almost all of 4.8 Attempting to build Xen with an explicit debug= set yields "You must use 'make menuconfig' to enable/disable debug now." I personally would prefer if debug= still worked, but apparently that is prohibitively difficult to do. ~Andrew
diff --git a/config/StdGNU.mk b/config/StdGNU.mk index 39d36b2..6be8233 100644 --- a/config/StdGNU.mk +++ b/config/StdGNU.mk @@ -35,14 +35,6 @@ UTIL_LIBS = -lutil SONAME_LDFLAG = -soname SHLIB_LDFLAGS = -shared -ifneq ($(debug),y) -CFLAGS += -O2 -fomit-frame-pointer -else -# Less than -O1 produces bad code and large stack frames -CFLAGS += -O1 -fno-omit-frame-pointer -CFLAGS-$(gcc) += -fno-optimize-sibling-calls -endif - ifeq ($(lto),y) CFLAGS += -flto LDFLAGS-$(clang) += -plugin LLVMgold.so diff --git a/config/SunOS.mk b/config/SunOS.mk index 86a384d..0fe5f45 100644 --- a/config/SunOS.mk +++ b/config/SunOS.mk @@ -31,12 +31,5 @@ UTIL_LIBS = SONAME_LDFLAG = -h SHLIB_LDFLAGS = -R $(SunOS_LIBDIR) -shared -ifneq ($(debug),y) -CFLAGS += -O2 -fno-omit-frame-pointer -else -# Less than -O1 produces bad code and large stack frames -CFLAGS += -O1 -fno-omit-frame-pointer -endif - CFLAGS += -Wa,--divide -D_POSIX_C_SOURCE=200112L -D__EXTENSIONS__ 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 # 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 +endif + CFLAGS += -nostdinc -fno-builtin -fno-common CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith CFLAGS += -pipe -g -D__XEN__ -include $(BASEDIR)/include/xen/config.h