[Mesa-dev] [PATCH] configure.ac: Save user {C, CXX}FLAGS and append them at end.

Mon Dec 31 06:03:23 PST 2012

On Sun, Dec 23, 2012 at 3:41 AM, Johannes Obermayr
<johannesobermayr at gmx.de> wrote:
> Am Samstag, 22. Dezember 2012, 16:34:48 schrieben Sie:
>> On Sat, Dec 22, 2012 at 10:47 AM, Johannes Obermayr
>> <johannesobermayr at gmx.de> wrote:
>> > Am Samstag, 22. Dezember 2012, 09:21:33 schrieb Matt Turner:
>> >> On Sat, Dec 22, 2012 at 9:16 AM, Johannes Obermayr
>> >> <johannesobermayr at gmx.de> wrote:
>> >> > This way the user has the privilege of last decision and so the option to build an optimized debug build again.
>> >> > ---
>> >>
>> >> You can just do CFLAGS="-g -O2" ./configure can't you?
>> >
>> > Nope.
>> >
>> > CXXFLAGS='-fmessage-length=0 -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector -funwind-tables -fasynchronous-unwind-tables -g' CFLAGS='-fmessage-length=0 -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector -funwind-tables -fasynchronous-unwind-tables -g' ./autogen.sh --host=x86_64-suse-linux-gnu --build=x86_64-suse-linux-gnu --program-prefix= --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64 --libexecdir=/usr/lib --localstatedir=/var --sharedstatedir=/usr/com --mandir=/usr/share/man --infodir=/usr/share/info --disable-dependency-tracking --enable-xvmc --enable-vdpau --enable-texture-float --enable-debug --with-dri-drivers=i915,i965,nouveau,r200,radeon,swrast --with-gallium-drivers=i915,nouveau,r300,r600,svga,swrast --enable-dri --enable-glx --enable-osmesa --enable-gles1 --enable-gles2 --enable-openvg --enable-shared-glapi --enable-shared-gallium --enable-gbm --enable-xa --ena
>> >  ble-gallium-egl --enable-gallium-llvm --enable-gallium-gbm --enable-opencl --enable-r600-llvm-compiler --enable-gallium-g3dvl --enable-glx-tls
>> >
>> > Resulting Makefile:
>> > before:
>> > CFLAGS = -fmessage-length=0 -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector -funwind-tables -fasynchronous-unwind-tables -g -Wall -std=c99 -Werror=implicit-function-declaration -Werror=missing-prototypes -fno-strict-aliasing -fno-builtin-memcmp -g -O0
>> >
>> > after:
>> > CFLAGS =  -Wall -std=c99 -Werror=implicit-function-declaration -Werror=missing-prototypes -fno-strict-aliasing -fno-builtin-memcmp -g -O0 -fmessage-length=0 -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector -funwind-tables -fasynchronous-unwind-tables -g
>> >
>> > You see configure CFLAGS and initial CFLAGS are vice versa.
>>
>> The real fix here is not to have configure.ac touch CFLAGS at all like
>> any reasonable autotools project. That was an unfortunate shortcut
>> during the initial work. That would be a lot more effort, though.
>
> Initial work after ~ 5 years?
> http://cgit.freedesktop.org/mesa/mesa/commit/?id=122345876479cf5cf553e38162ab105658614ab7

Not sure what you mean by this. When I wrote configure.ac however many
years ago, I didn't cleanly split off CFLAGS set by mesa from the
user's CFLAGS as a shortcut. Looking back now, I should have just bit
the bullet and made something like MESA_CFLAGS/MESA_LDFLAGS and made
all the Makefiles use that.

> Btw:
> http://cgit.freedesktop.org/mesa/mesa/commit/?id=ff2efdf5997d20b41f7a82b77118366e6fbd23bc
> http://cgit.freedesktop.org/mesa/mesa/commit/?id=33ae29c93b8f70a86dcedc495dd658a5d5679db3
>
> This patch covers the motivation for commit 33ae29c.
>
>>
>> However, the typical convention is that the user's CFLAGS come first
>> (see any automake COMPILE or LTCOMPILE definition), mostly so that any
>> user specified -I paths take precedence. That's unfortunate when -O
>> options to gcc are last one wins, but that's how things have typically
>> been. I would make this change very cautiously.
>>
>> --
>> Dan
>
> And here is the real reason for my request:
>
> /usr/lib64/mesa-demos/egl/opengl/xeglgears
>
> AMD Fusion       (fps)                  Nvidia ION (fps)
> -O0     -O2                     -O0     -O2
> 562,7   750,778 0,33            641,851 921,464 0,44
> 701,756 662,877 -0,06           693,752 1000,196        0,44
> 571,566 665,052 0,16            692,196 936,147 0,35
> 542,715 664,658 0,22            693,733 995,499 0,43
> 542,935 664,78  0,22            692,702 992,634 0,43
> 542,574 665,398 0,23            692,835 998,665 0,44
> 542,945 663,087 0,22            691,353 998,319 0,44
> 541,732 664,253 0,23            656,32  995,91  0,52
> 543,392 665,078 0,22            691,084 994,037 0,44
> 542,901 663,406 0,22            693,464 996,858 0,44
> 5635,216        6729,367        0,19            6839,29 9829,729        0,44 (sum)
>
> -> Performance + 19 % on AMD Fusion, + 44 % on Nvidia ION on debug build with -O2. :-)
>
> Usual users should build Mesa with default FLAGS. Advanced users should be able to build it with their own FLAGS without modifying source.

Right, --enable-debug is forcing you to have -O0. That's the problem.
Mesa should not be overriding user defined flags such as optimization
unless it's necessary to get Mesa to build. There are two issues here:

1. Adding -O0 in --enable-debug is a convenience, but as you see it
can conflicts with the user's flags. This is why I left -O out of
--enable-debug from the beginning.

http://cgit.freedesktop.org/mesa/mesa/commit/?id=23656c47

That said, the whole option is just a helper. If you know what you're
doing, you can easily build with CFLAGS='-g -O2 -DDEBUG' and have all
the control you want. --enable-32-bit/--enable-64-bit are the same
way. They're convenience options I added that fiddle with your CFLAGS
so you don't have to, but they clearly have to make compromises about
"the right thing". All three options should have their help text
updated to indicate that they're just passing flags to the compiler
and not doing anything special to the code. That way people that know
what they're doing can set things up the way they want.

An alternate fix would be to check if CFLAGS contains -O already and
leave out -O0 in that case.

2. More generally, Mesa should be not be touching the user's CFLAGS at
all. All the flags Mesa needs to pass to the compiler/linker should be
split off into separate variables so the user can be free to do
whatever they want with CFLAGS/CPPFLAGS/LDFLAGS/whatever. Look at any
cleanly constructed autotools project and you will see it does not
touch the user's environment variables at all. And, in fact,
automake/libtool does exactly what you're requesting by putting
CFLAGS/LDFLAG at the end of the command. From one of my other
projects:

COMPILE = $(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) \
        $(CPPFLAGS) $(AM_CFLAGS) $(CFLAGS)
LTCOMPILE = $(LIBTOOL) $(AM_V_lt) --tag=CC $(AM_LIBTOOLFLAGS) \
        $(LIBTOOLFLAGS) --mode=compile $(CC) $(DEFS) \
        $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) \
        $(AM_CFLAGS) $(CFLAGS)
LINK = $(LIBTOOL) $(AM_V_lt) --tag=CC $(AM_LIBTOOLFLAGS) \
        $(LIBTOOLFLAGS) --mode=link $(CCLD) $(AM_CFLAGS) $(CFLAGS) \
        $(AM_LDFLAGS) $(LDFLAGS) -o $@

I'm pretty sure the right way to fix this bug is to drop the whole
USER_*FLAGS thing, bite the bullet, and spray
MESA_CFLAGS/MESA_CPPFLAGS/MESA_LDFLAGS all over the tree. Or cheat and
AC_SUBST AM_CFLAGS etc. from configure.ac and check that no one
overrides it in Makefile.am.

Looking back at my previous email, I was wrong that automake/libtool
put CFLAGS at the beginning of the command line. This patch has a
glaring bug in it, though. By setting CFLAGS/CXXFLAGS to "" during
configure, you lose the user's settings while doing the feature tests.
This is bad since then the feature tests have been performed with
different compiler settings the rest of the build. It's trivial to
think up ways (like missing -I while doing AC_CHECK_HEADER) that the
build could break in subtle ways.

I'm afraid this issue needs a more invasive fix. Either that larger
fix I discussed above, or just make sure that CFLAGS/CXXFLAGS are
always prepended to in configure as a short term fix.

--
Dan