Xen devel - Feb 2012 - [PATCH] tools/qemu-xen: remove CFLAGS for qemu build

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1330015545 -3600
# Node ID 5bdbdcb03d60a7b58f41306ef39cb988100efbe4
# Parent  56214b978466914c1b9f8adb1158a3217a823e42
tools/qemu-xen: remove CFLAGS for qemu build

Currently qemu-xen gets build with CFLAGS only if CFLAGS was already in
the environment during make invocation. If CFLAGS is in environment then
make will append all of the various flags specified in xen Makefiles,
which is then passed to qemu configure. If CFLAGS is not set, then
configure will use just "-O2 -g" because make does not export its own
CFLAGS variable.

To make qemu-xen build consistent this change removes CFLAGS from the
environment so that only the CFLAGS from qemu configure script will be
used. This matches what is done in kvm.rpm and qemu.rpm where for
example RPM_OPT_FLAGS is not passes as CFLAGS. Otherwise those packages
would not build as well.

Passing makes CFLAGS to configure will lead to build errors:
- xen Makefiles append -std=gnu99, this breaks qemu build due to a bug
  in header file:
fpu/softfloat-specialize.h:107: error: initializer element is not constant
- in 32bit builds, qemus configure script will append -mcpu=i486 in an
  odd way, which leads to unknown gcc cmdline options due to a missing
  space
- xen Makefiles will append -Wall which will expose all sorts of style
  issues in the qemu code
- in one case some of the asm() blocks will not compile with gcc 4.6 in
  openSuSE 12.1

Until upstream qemu has fixed all these issues use no extra CFLAGS to
configure qemu-xen.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 56214b978466 -r 5bdbdcb03d60 tools/Makefile
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -146,6 +146,7 @@ subdir-all-qemu-xen-dir subdir-install-q
 		source=.; \
 	fi; \
 	cd qemu-xen-dir; \
+	env -u CFLAGS \
 	$$source/configure --enable-xen --target-list=i386-softmmu \
 		--source-path=$$source \
 		--extra-cflags="-I$(XEN_ROOT)/tools/include \

Olaf Hering writes ("[Xen-devel] [PATCH] tools/qemu-xen: remove CFLAGS for
qemu build"):
> tools/qemu-xen: remove CFLAGS for qemu build
> 
> Currently qemu-xen gets build with CFLAGS only if CFLAGS was already in
> the environment during make invocation. If CFLAGS is in environment then
> make will append all of the various flags specified in xen Makefiles,
> which is then passed to qemu configure. If CFLAGS is not set, then
> configure will use just "-O2 -g" because make does not export its
own
> CFLAGS variable.
> 
> To make qemu-xen build consistent this change removes CFLAGS from the
> environment so that only the CFLAGS from qemu configure script will be
> used. This matches what is done in kvm.rpm and qemu.rpm where for
> example RPM_OPT_FLAGS is not passes as CFLAGS. Otherwise those packages
> would not build as well.
I think it is a bug to try to invoke the Xen build system with CFLAGS
set.  So I don''t think it''s a good idea to try to fix it up.

Ian.

On Thu, Mar 01, Ian Jackson wrote:
> Olaf Hering writes ("[Xen-devel] [PATCH] tools/qemu-xen: remove CFLAGS
for qemu build"):
> > tools/qemu-xen: remove CFLAGS for qemu build
> > 
> > Currently qemu-xen gets build with CFLAGS only if CFLAGS was already
in
> > the environment during make invocation. If CFLAGS is in environment
then
> > make will append all of the various flags specified in xen Makefiles,
> > which is then passed to qemu configure. If CFLAGS is not set, then
> > configure will use just "-O2 -g" because make does not
export its own
> > CFLAGS variable.
> > 
> > To make qemu-xen build consistent this change removes CFLAGS from the
> > environment so that only the CFLAGS from qemu configure script will be
> > used. This matches what is done in kvm.rpm and qemu.rpm where for
> > example RPM_OPT_FLAGS is not passes as CFLAGS. Otherwise those
packages
> > would not build as well.
> 
> I think it is a bug to try to invoke the Xen build system with CFLAGS
> set.  So I don''t think it''s a good idea to try to fix it
up.
Its a way to pass RPM_OPT_FLAGS to build the tools. This includes things
like fortify-source (whatever the exact spelling is) and other global
flags to build a distro.
What other ways exist to pass that to the buildsystem for the tools?

Olaf

Olaf Hering writes ("Re: [Xen-devel] [PATCH] tools/qemu-xen: remove CFLAGS
for qemu build"):
> Its a way to pass RPM_OPT_FLAGS to build the tools. This includes things
> like fortify-source (whatever the exact spelling is) and other global
> flags to build a distro.
> What other ways exist to pass that to the buildsystem for the tools?
This is what EXTRA_CFLAGS and PREPEND_INCLUDES and so forth are for.

Ian.

On Thu, Mar 01, Ian Jackson wrote:
> Olaf Hering writes ("Re: [Xen-devel] [PATCH] tools/qemu-xen: remove
CFLAGS for qemu build"):
> > Its a way to pass RPM_OPT_FLAGS to build the tools. This includes
things
> > like fortify-source (whatever the exact spelling is) and other global
> > flags to build a distro.
> > What other ways exist to pass that to the buildsystem for the tools?
> 
> This is what EXTRA_CFLAGS and PREPEND_INCLUDES and so forth are for.
With EXTRA_CFLAGS there is even more breakage:

/usr/src/packages/BUILD/xen-4.2.24911/non-dbg/tools/firmware/etherboot/ipxe/src/arch/i386/interface/pcbios/int13.c:1729:
undefined reference to `__stack_chk_fail''

I will have a look tomorrow.

Olaf

On Thu, Mar 01, Ian Jackson wrote:
> Olaf Hering writes ("Re: [Xen-devel] [PATCH] tools/qemu-xen: remove
CFLAGS for qemu build"):
> > Its a way to pass RPM_OPT_FLAGS to build the tools. This includes
things
> > like fortify-source (whatever the exact spelling is) and other global
> > flags to build a distro.
> > What other ways exist to pass that to the buildsystem for the tools?
> 
> This is what EXTRA_CFLAGS and PREPEND_INCLUDES and so forth are for.
EXTRA_CFLAGS is only used by ipxe.


I think that there should be a way to pass individual external CFLAGS to
the tools, ipxe, qemu-traditional, qemu-xen, etc builds.
From a distro perspective, its required to build libraries and binaries
with certain global cflags. Up to the point when qemu-upstream was
imported it worked as expected by exporting CFLAGS before ''make
tools''.
Now qemu-upstream reuses these CFLAGS, but it cant deal with the result.

How about something like this:
env \
        EXTRA_CFLAGS_XEN_TOOLS="$RPM_OPT_FLAGS" \
        EXTRA_CFLAGS_QEMU_UPSTREAM="" \
        EXTRA_CFLAGS_IPXE="" \
        ./configure <more options>

Then configure can pass these to the various Makefiles. In case of qemu
its configure could be invoked like 
env \
        CFLAGS="$(EXTRA_CFLAGS_QEMU_UPSTREAM)" \
        $source/configure <qemu options>

In case of ipxe it would be "EXTRA_CFLAGS=$(EXTRA_CFLAGS_IPXE)".
I think you get the idea.

Right now I see the need for external CFLAGS only for tools itself, but
if the options are there one can use them also for the other targets if
there is a demand for it.

Olaf

Olaf Hering writes ("Re: [Xen-devel] [PATCH] tools/qemu-xen: remove CFLAGS
for qemu build"):
> I think that there should be a way to pass individual external CFLAGS to
> the tools, ipxe, qemu-traditional, qemu-xen, etc builds.
Yes, but I think that''s already supposed to exist.  PREPEND_CFLAGS and
APPEND_CFLAGS.  Look in tools/Rules.mk.
> From a distro perspective, its required to build libraries and binaries
> with certain global cflags. Up to the point when qemu-upstream was
> imported it worked as expected by exporting CFLAGS before ''make
tools''.
> Now qemu-upstream reuses these CFLAGS, but it cant deal with the result.
> 
> How about something like this:
> env \
>         EXTRA_CFLAGS_XEN_TOOLS="$RPM_OPT_FLAGS" \
>         EXTRA_CFLAGS_QEMU_UPSTREAM="" \
>         EXTRA_CFLAGS_IPXE="" \
>         ./configure <more options>
It doesn''t need to be plumbed through configure; it can just be used
directly by make, although configure should use it for its tests I
guess and probably doesn''t right now.

Ian.

On Wed, Mar 14, Ian Jackson wrote:
> Olaf Hering writes ("Re: [Xen-devel] [PATCH] tools/qemu-xen: remove
CFLAGS for qemu build"):
> > I think that there should be a way to pass individual external CFLAGS
to
> > the tools, ipxe, qemu-traditional, qemu-xen, etc builds.
> 
> Yes, but I think that''s already supposed to exist.  PREPEND_CFLAGS
and
> APPEND_CFLAGS.  Look in tools/Rules.mk.
PREPEND_CFLAGS sets just CFLAGS inside configure. APPEND_CFLAGS is the
wrong place, the Makefiles must be able to override whats already in
CFLAGS.
> > From a distro perspective, its required to build libraries and
binaries
> > with certain global cflags. Up to the point when qemu-upstream was
> > imported it worked as expected by exporting CFLAGS before
''make tools''.
> > Now qemu-upstream reuses these CFLAGS, but it cant deal with the
result.
> > 
> > How about something like this:
> > env \
> >         EXTRA_CFLAGS_XEN_TOOLS="$RPM_OPT_FLAGS" \
> >         EXTRA_CFLAGS_QEMU_UPSTREAM="" \
> >         EXTRA_CFLAGS_IPXE="" \
> >         ./configure <more options>
> 
> It doesn''t need to be plumbed through configure; it can just be
used
> directly by make, although configure should use it for its tests I
> guess and probably doesn''t right now.
My idea is to use configure to write the desired strings into the
Makefiles. The change below mostly works.
I''m using it like this in xen.spec:

 env \
 EXTRA_CFLAGS_XEN_TOOLS="${RPM_OPT_FLAGS} -DEXTRA_CFLAGS_XEN_TOOLS" \
 EXTRA_CFLAGS_QEMU_TRADITIONAL="${RPM_OPT_FLAGS}
-DEXTRA_CFLAGS_QEMU_TRADITIONAL" \
 EXTRA_CFLAGS_QEMU_XEN="${RPM_OPT_FLAGS} -DEXTRA_CFLAGS_QEMU_XEN" \
 ./configure \
        --libdir=%{_libdir} \
        --prefix=/usr

However, there are a few issues with this approach:

Now that CFLAGS starts as empty variable in config/Tools.mk, this file
has to be included first. Therefore the order of include in
tools/Rules.mk is changed. I will send patches to fix affected
Makefiles, so far its only tools/libfsimage/zfs/Makefile. I will prepare
a patch to move the include tools/libfsimage/Rules.mk in all libfsimage
Makefiles.

I noticed that @F is empty now, leading to ..d files in all directories.
No idea how to fix that.
tools/Rules.mk:CFLAGS += -MMD -MF .$(@F).d

Unrelated to this change: in config/Tools.mk debug is forced to
''y''. I
think the logic in tools/m4/{enable,disable}_feature.m4 is flipped.

What do you think about the general approach of this patch?

Olaf


# HG changeset patch
# Parent 773d0367087212c43faf8cdcc21cf443b1ea0046

diff -r 773d03670872 config/Tools.mk.in
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -23,6 +23,10 @@ PREPEND_LIB         := @PREPEND_LIB@
 APPEND_INCLUDES     := @APPEND_INCLUDES@
 APPEND_LIB          := @APPEND_LIB@
 
+CFLAGS                        := @EXTRA_CFLAGS_XEN_TOOLS@
+EXTRA_CFLAGS_QEMU_TRADITIONAL := @EXTRA_CFLAGS_QEMU_TRADITIONAL@
+EXTRA_CFLAGS_QEMU_XEN         := @EXTRA_CFLAGS_QEMU_XEN@
+
 # Download GIT repositories via HTTP or GIT''s own protocol?
 # GIT''s protocol is faster and more robust, when it works at all
(firewalls
 # may block it). We make it the default, but if your GIT repository downloads
diff -r 773d03670872 tools/Makefile
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -122,6 +122,8 @@ subdir-all-qemu-xen-traditional-dir subd
 	set -e; \
 		$(buildmakevars2shellvars); \
 		cd qemu-xen-traditional-dir; \
+		env CFLAGS="$(EXTRA_CFLAGS_QEMU_TRADITIONAL)" \
+		PREFIX="$(PREFIX)" \
 		$(QEMU_ROOT)/xen-setup $(IOEMU_CONFIGURE_CROSS); \
 		$(MAKE) install
 
@@ -147,6 +149,7 @@ subdir-all-qemu-xen-dir subdir-install-q
 		source=.; \
 	fi; \
 	cd qemu-xen-dir; \
+	env CFLAGS="$(EXTRA_CFLAGS_QEMU_XEN)" \
 	$$source/configure --enable-xen --target-list=i386-softmmu \
 		--source-path=$$source \
 		--extra-cflags="-I$(XEN_ROOT)/tools/include \
diff -r 773d03670872 tools/Rules.mk
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -3,8 +3,8 @@
 # `all'' is the default target
 all:
 
+include $(XEN_ROOT)/config/Tools.mk
 include $(XEN_ROOT)/Config.mk
-include $(XEN_ROOT)/config/Tools.mk
 
 export _INSTALL := $(INSTALL)
 INSTALL = $(XEN_ROOT)/tools/cross-install
diff -r 773d03670872 tools/configure.ac
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -54,6 +54,12 @@ AC_ARG_VAR([APPEND_INCLUDES],
     [List of include folders to append to CFLAGS (without -I)])
 AC_ARG_VAR([APPEND_LIB],
     [List of library folders to append to LDFLAGS (without -L)])
+AC_ARG_VAR([EXTRA_CFLAGS_XEN_TOOLS],
+    [Extra CFLAGS to build tools])
+AC_ARG_VAR([EXTRA_CFLAGS_QEMU_TRADITIONAL],
+    [Extra CFLAGS to build qemu-traditional])
+AC_ARG_VAR([EXTRA_CFLAGS_QEMU_XEN],
+    [Extra CFLAGS to build qemu-xen])
 
 AX_SET_FLAGS

On Wed, Mar 14, Olaf Hering wrote:
> I noticed that @F is empty now, leading to ..d files in all directories.
> No idea how to fix that.
> tools/Rules.mk:CFLAGS += -MMD -MF .$(@F).d
> diff -r 773d03670872 config/Tools.mk.in
> --- a/config/Tools.mk.in
> +++ b/config/Tools.mk.in
> @@ -23,6 +23,10 @@ PREPEND_LIB         := @PREPEND_LIB@
>  APPEND_INCLUDES     := @APPEND_INCLUDES@
>  APPEND_LIB          := @APPEND_LIB@
>  
> +CFLAGS                        := @EXTRA_CFLAGS_XEN_TOOLS@
> +EXTRA_CFLAGS_QEMU_TRADITIONAL := @EXTRA_CFLAGS_QEMU_TRADITIONAL@
> +EXTRA_CFLAGS_QEMU_XEN         := @EXTRA_CFLAGS_QEMU_XEN@
The empty @F is fixed by using ''+='' instead of
'':='' above. With this
small change my attempt works for me, CFLAGS get passed properly to qemu
and tools itself.

Olaf