Hi Richard,
> On 20 Aug 2021, at 09:39, Richard Sandiford <richard.sandif...@arm.com> wrote:
>
> Iain Sandoe <i...@sandoe.co.uk> writes:
>> This concerns the settings of flags (using the host makefile fragment) for
>> tools that will run on the host.
>>
>> At present the (no)PIE flags are computed in gcc/configure but it is not
>> possible to override them (either from higher level Makefile or from the
>> command line). Secondly the ordering of placement of flags assumes
>> ELF semantics are OK for ordering of -fno-PIE and -fPIC.
>>
>> For Darwin, this introduces problems if fno-PIE causes PIC to be switched
>> off and the bootstrap compiler does not support mdynamic-no-pic (which
>> is the case when we bootstrap a 32b toolchain with clang). This causes
>> the host files to be built '-static' which is not legal for user-space
>> code, and the build terminates with illegal relocations (so that bootstrap
>> fails).
>>
>> This patch:
>>
>> 1. Allows the computed PIE flags to be overridden by the top level
>> configure.
>>
>> 2. Allows a host fragment to set values on the basis of the configured
>> host platform/version.
>>
>> 3. Sets suitable values for the Darwin cases that currently fail.
>>
>> tested on i686,powerpc,x86-64-darwin, x86_64, powerpc64-linux,
>> OK for master?
>>
>> thanks
>> Iain
>>
>> Signed-off-by: Iain Sandoe <i...@sandoe.co.uk>
>>
>> ChangeLog:
>>
>> * Makefile.in: Regenerated.
>> * Makefile.tpl: Add PIE flags to HOST_EXPORTS and
>> POST_STAGE1_HOST_EXPORTS as specified by the host makefile
>> fragment.
>>
>> config/ChangeLog:
>>
>> * mh-darwin: Specify suitable PIE/PIC flags for 32b Darwin
>> hosts.
>>
>> gcc/ChangeLog:
>>
>> * configure: Regenerate.
>> * configure.ac: Allow top level configure to override assumed
>> PIE flags.
>> ---
>> Makefile.in | 14 ++++++++++++++
>> Makefile.tpl | 14 ++++++++++++++
>> config/mh-darwin | 20 ++++++++++++++++----
>> gcc/configure | 4 ++--
>> gcc/configure.ac | 4 ++--
>> 5 files changed, 48 insertions(+), 8 deletions(-)
>>
>> diff --git a/Makefile.tpl b/Makefile.tpl
>> index 9adf4f94728..9523be5a761 100644
>> --- a/Makefile.tpl
>> +++ b/Makefile.tpl
>> @@ -576,6 +576,20 @@ all:
>> @host_makefile_frag@
>> ###
>>
>> +# Allow host makefile fragment to override PIE settings.
>> +ifneq ($(STAGE1_NO_PIE_CFLAGS),)
>> + HOST_EXPORTS += export NO_PIE_CFLAGS="$(STAGE1_NO_PIE_CFLAGS)";
>> +endif
>> +ifneq ($(STAGE1_NO_PIE_FLAG),)
>> + HOST_EXPORTS += export NO_PIE_FLAG="$(STAGE1_NO_PIE_FLAG)";
>> +endif
>> +ifneq ($(BOOT_NO_PIE_CFLAGS),)
>> + POSTSTAGE1_HOST_EXPORTS += export NO_PIE_CFLAGS="$(BOOT_NO_PIE_CFLAGS)";
>> +endif
>> +ifneq ($(BOOT_NO_PIE_FLAG),)
>> + POSTSTAGE1_HOST_EXPORTS += export NO_PIE_FLAG="$(BOOT_NO_PIE_FLAG)";
>> +endif
>
> Not really my area, but:
thanks for a very helpful review :)
> Is the NO_PIE_FLAG just for completeness?
Well, the NO_PIE_FLAG is used (the flags that are passed down lose the
distinction of stage1/post-stage1),
it’s the BOOT_NO_PIE_FLAG that is not used in this initial patch - however, it
is used when we consider
the enable_host_shared in a later patch.
The idea was to produce a general facility even if the first user of it did not
need all elements.
> It didn't look like the
> patch overrode that. Might be easier to leave it out if so, to avoid
> any risk of accidentally having an empty variable.
Yeah, perhaps trying to be general without a use-case, fails to test adequately.
… since the host shared stuff is now applied I’ll revise this consider that too
so that both the
stage1 and post-stage1 cases should be exercised.
> Maybe it would be easier to have the makefile fragments determine
> something like CODE_MODEL_CFLAGS, which can be "-fPIC", "-mdynamic-no-pic",
> etc., and use:
>
> COMPILER += $(NO_PIE_CFLAGS) $(CODE_MODEL_CFLAGS)
OK. I have misgivings about this - the problem is that:
-fPIC -fno-PIE != -fno-PIE -fPIC, which is not obvious to many folks - who
expect that
the “last edition of a flag will be the one in force”.
So the PIE-ness and the PIC-ness are decoupled in the configury but they need
to be
ordered specifically for targets that want PIC code by default (FWIW, I don’t
think Darwin
is the only default-PIC case here, from discussions on irc).
Thus the mechanism I adopted was to ensure that the no-PIE setting could be
adjusted
to make sure that PIC was enforced where needed (the probably non-obvious
aspect of
this is that mdynamic-no-pic only happens to work because it doesn’t exist for
ELF and
thus doesn’t get switched off by the no-PIE logic). All very fragile, sadly.
In fact, (on Darwin) PIE implies PIC - it is one case where ELF semantics are
at odds with
Mach-O in a non-easy manner (I have caught most cases in
darwin_override_options, but
that obv. doesn’t work at configure-time since the bootstrap complier is
outside our control).
I would be concerned that any decoupled flag could become reordered w.r.t the
no-PIE
and break things again, but I guess we could assume that testing would catch
that.
Trying to couple the PIC and PIE configs might be possible but I suspect it’s a
deeper
rabbit hole than I have cycles to explore at the present.
> This avoids mixing the autoconf-derived variable with the host override.
>
> I think we need to stick to the traditional sh
>
> foo=X; export foo
>
> style used elsewhere. "export foo=X" is a bash extension.
good catch; something that doesn’t trigger on darwin and linux testing but
would be
reported the day after the patch was applied ;)
>
> It also might be better to have:
>
> STAGE1_CODE_MODEL_CFLAGS =
> BOOT_CODE_MODEL_CFLAGS =
> CODE_MODEL_CFLAGS =
>
> in Makefile.tpl (just before the config/ includes) and then override
> them in mh-darwin. We can then include the exports unconditionally,
> like with other variables.
that’s a good idea it should make things more readable...
>
> I think we should export them for the host too (via HOST_EXPORTS).
… that is done, in fact, it’s probably just not the most readable config
fragment.
>
> We could add {STAGE1,BOOT}_CODE_MODEL_CFLAGS to the main
> {STAGE1,BOOT}_{C,CXX}FLAGS in Makefile.tpl. It looks like
> that might simplify some of the existing mh-darwin code a bit.
>
> Like I say, not my area, so feel free to ignore :-)
I’ll make a revision to implement the changes to avoid bash-isms and the
simplification of the exports;
for switching to use of CODE_MODEL_CFLAGS, I’m open to persuasion
that it can be done in a non-fragile manner.
thanks for the review,
Iain
>
> Thanks,
> Richard
>
>> +
>> # This is the list of directories that may be needed in RPATH_ENVVAR
>> # so that programs built for the target machine work.
>> TARGET_LIB_PATH = [+ FOR target_modules +][+
>> diff --git a/config/mh-darwin b/config/mh-darwin
>> index b72835ae953..d3c357a0574 100644
>> --- a/config/mh-darwin
>> +++ b/config/mh-darwin
>> @@ -7,6 +7,13 @@
>> # We use Werror, since some versions of clang report unknown command line
>> flags
>> # as a warning only.
>>
>> +# In addition, all versions of clang released to date treat -fno-PIE in -m32
>> +# compilations as switching PIC code off too, which means that -fno-PIE,
>> +# without -mdynamic-no-pic produces broken relocations (and we cannot enable
>> +# -mdynamic-no-pic because the inverse setting doesn't work). To work
>> around
>> +# this, we need to ensure that the no-PIE option is followed by -fPIE, when
>> +# the compiler does not support mdynamic-no-pic.
>> +
>> # We only need to determine this for the host tool used to build stage1 (or a
>> # non-bootstrapped compiler), later stages will be built by GCC which
>> supports
>> # the required flags.
>> @@ -24,23 +31,28 @@ endif
>> @if gcc-bootstrap
>> ifeq (${BOOTSTRAP_TOOL_CAN_USE_MDYNAMIC_NO_PIC},true)
>> STAGE1_CFLAGS += -mdynamic-no-pic
>> +STAGE1_NO_PIE_CFLAGS = -fno-PIE
>> else
>> STAGE1_CFLAGS += -fPIC
>> +STAGE1_NO_PIE_CFLAGS = -fno-PIE -fPIC
>> endif
>> ifeq (${host_shared},no)
>> # Add -mdynamic-no-pic to later stages when we know it is built with GCC.
>> BOOT_CFLAGS += -mdynamic-no-pic
>> +BOOT_NO_PIE_CFLAGS = -fno-PIE
>> +else
>> +BOOT_NO_PIE_CFLAGS = -fno-PIE -fPIC
>> endif
>> @endif gcc-bootstrap
>>
>> @unless gcc-bootstrap
>> ifeq (${BOOTSTRAP_TOOL_CAN_USE_MDYNAMIC_NO_PIC},true)
>> -# FIXME: we should also enable this for cross and non-bootstrap builds but
>> -# that needs amendment to libcc1.
>> -# CFLAGS += -mdynamic-no-pic
>> -# CXXFLAGS += -mdynamic-no-pic
>> +CFLAGS += -mdynamic-no-pic
>> +CXXFLAGS += -mdynamic-no-pic
>> +STAGE1_NO_PIE_CFLAGS = -fno-PIE
>> else
>> CFLAGS += -fPIC
>> CXXFLAGS += -fPIC
>> +STAGE1_NO_PIE_CFLAGS = -fno-PIE -fPIC
>> endif
>> @endunless gcc-bootstrap
>>
>> diff --git a/gcc/configure.ac b/gcc/configure.ac
>> index ad8fa5a4604..fad9b27879e 100644
>> --- a/gcc/configure.ac
>> +++ b/gcc/configure.ac
>> @@ -7580,7 +7580,7 @@ AC_CACHE_CHECK([for -fno-PIE option],
>> [gcc_cv_c_no_fpie=no])
>> CXXFLAGS="$saved_CXXFLAGS"])
>> if test "$gcc_cv_c_no_fpie" = "yes"; then
>> - NO_PIE_CFLAGS="-fno-PIE"
>> + NO_PIE_CFLAGS=${NO_PIE_CFLAGS-"-fno-PIE"}
>> fi
>> AC_SUBST([NO_PIE_CFLAGS])
>>
>> @@ -7594,7 +7594,7 @@ AC_CACHE_CHECK([for -no-pie option],
>> [gcc_cv_no_pie=no])
>> LDFLAGS="$saved_LDFLAGS"])
>> if test "$gcc_cv_no_pie" = "yes"; then
>> - NO_PIE_FLAG="-no-pie"
>> + NO_PIE_FLAG=${NO_PIE_FLAG-"-no-pie"}
>> fi
>> AC_SUBST([NO_PIE_FLAG])
