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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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

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