Re: [PATCH] Fix CFLAGS overwrite by Makefile

On Tuesday, May 24, 2011 5:30:37 pm Arnaud Lacombe wrote:
> Hi,
> 
> On Tue, May 24, 2011 at 4:41 PM, Dimitry Andric <dim_at_freebsd.org> wrote:
> > On 2011-05-24 22:09, Arnaud Lacombe wrote:
> >>
> >> Many Makefile (espectially under sys/boot/) overwrite the value of 
CFLAGS.
> >> This is an issue if you want to generate code for a specific CPU as 
before
> >> the
> >> Makefile is interpreted, CFLAGS might already have been set with CPU
> >> specific
> >> settings by<bsd.cpu.mk>, which is source from sys.mk.
> >
> > ...
> >>
> >> --- a/sys/boot/i386/boot2/Makefile
> >> +++ b/sys/boot/i386/boot2/Makefile
> >
> > ...
> >
> > The problem with this patch is that for some of the things you fixed,
> > stuff like boot-time programs, you NEVER want any CPU specific settings!
> > You must use the default, lowest common denominator setting instead, or
> > there is no guarantee the boot program will be correct.
> >
> To use your argument against you: with the default, the boot program
> is not correct (see below).

Thousands of machines succesfully booting FreeBSD would seem to contradict
your assertion.

> > So that is why these Makefiles purposefully overwrite CFLAGS. it is not
> > by accident.
> You just might be right, but unless the code say the overwrite is
> _on_purpose_, I would not assume the state of mind of the author, one
> way or another.

The boot code is certainly intended to be something that works across the
board.  Also, I doubt you will see any user-visible performance difference
from changing the optimization options for the boot code.

> > Besides, for space-constrained things like boot2, you
> > might not even be able to compile it when using non-standard settings,
> > since the code might grow too large.
> >
> or can shrink by using more optimized instructions.

Well, your test in a later e-mail is a bit flawed.  GCC tends to insert
a lot of padding for newer CPUs to align things on more optimal boundaries.
We run 'sed' over the assembly version of boot2 to strip all that out.
However, the more important point for the boot code is that it needs to
just work.

> The original trouble I met, is that building for an i586 target in a
> 32bits jail, on top of an amd64 system[0] (I do not have control over
> that setup) produces incorrect binaries. The current fix I've got is
> to define MACHINE_ARCH=i386 and CPUTYPE=i586. This enforces
> `-march=i586' to be passed to the compiler, for all except the
> bootloader (because it overwrites CFLAGS). With this, binaries
> produced works fine (ie. /bin/sh no longer SIGILL when bringing up the
> system). So I suspect that gcc default to i686 in this setup and
> corrupt all the binaries, thus the attached patch.

Wait.  You must have something wrong in your jail if you can't do a buildworld 
with CPUTYPE set to none and have it do the right thing.  You need to find 
your root problem.  Forcing CPUCFLAGS for the boot code is a band-aid, it's 
not the right solution to your problem.

It may be that your jail is not a pure 32-bit jail (some things like a 32-bit 
ps won't really work in with a 64-bit host for example).  Also, until 
recently, the machine_arch sysctl reported amd64 for 32-bit binaries.  That 
was recently changed and might also help your world build to be more correct.
You could also try doing 'make buildworld TARGET=i386' in a 64-bit jail.  
However, one question is what problem are you trying to solve?

-- 
John Baldwin