Re: [PATCH] PR ld/20283: gold: Add a linker configure option --enable-relro

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, Jun 22, 2016 at 7:09 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 22 Jun 2016 06:33, H.J. Lu wrote:
>> On Wed, Jun 22, 2016 at 6:16 AM, Mike Frysinger <vapier@gentoo.org> wrote:
>> > On 22 Jun 2016 05:41, H.J. Lu wrote:
>> >> On Wed, Jun 22, 2016 at 3:43 AM, Mike Frysinger <vapier@gentoo.org> wrote:
>> >> > On 21 Jun 2016 19:45, H.J. Lu wrote:
>> >> >> On Tue, Jun 21, 2016 at 6:06 PM, Mike Frysinger wrote:
>> >> >> > On 21 Jun 2016 15:11, H.J. Lu wrote:
>> >> >> >> +# Decide if -z relro should be enabled in ELF linker by default.
>> >> >> >> +ac_default_ld_z_relro=unset
>> >> >> >> +# Provide a configure time option to override our default.
>> >> >> >> +AC_ARG_ENABLE(relro,
>> >> >> >> +           AS_HELP_STRING([--enable-relro],
>> >> >> >> +           [enable -z relro in ELF linker by default]),
>> >> >> >> +[case "${enableval}" in
>> >> >> >> +  no)  ac_default_ld_z_relro=0 ;;
>> >> >> >> +esac])dnl
>> >> >> >> +if test ${ac_default_ld_z_relro} = unset; then
>> >> >> >> +  ac_default_ld_z_relro=1
>> >> >> >> +fi
>> >> >> >
>> >> >> > any reason to not just write it like:
>> >> >> > AC_ARG_ENABLE(relro,
>> >> >> >         AS_HELP_STRING([--enable-relro],
>> >> >> >                 [enable -z relro in ELF linker by default]))
>> >> >> > if test "${enable_relro}" = "yes"; then
>> >> >> >   ac_default_ld_z_relro=1
>> >> >> > else
>> >> >> >   ac_default_ld_z_relro=0
>> >> >> > fi
>> >> >> >
>> >> >> > it's a bit simpler that way.
>> >> >>
>> >> >> I copied it from ld where ac_default_ld_z_relro is set to 1 unless
>> >> >> --disable-relro is used or not a Linux target. For gold, it becomes
>> >> >> unless --disable-relro is used.  It is easier for me to keep both ld
>> >> >> and gold similar.
>> >> >
>> >> > except that patch isn't merged yet, so you have time to change it too
>> >>
>> >> This is what I checked in.
>> >
>> > there was still no reason to not write it the way i suggested.
>> > it's shorter and simpler.
>>
>> It is better to make ld and gold consistent just in case
>> we may need to change.
>
> that is entirely irrelevant.  neither ld nor gold had this change
> merged which meant you could fix both at the same time.  in fact,

The ld case is done such a way on purpose since default depends
on configure.tgt:

# Decide if -z relro should be enabled in ELF linker by default.
ac_default_ld_z_relro=unset
# Provide a configure time option to override our default.
AC_ARG_ENABLE(relro,
              AS_HELP_STRING([--enable-relro],
              [enable -z relro in ELF linker by default]),
[case "${enableval}" in
  yes)  ac_default_ld_z_relro=1 ;;
  no)  ac_default_ld_z_relro=0 ;;
esac])dnl
...
---configure.tgt---
*-*-linux*)
  if test ${ac_default_ld_z_relro} = unset; then
    ac_default_ld_z_relro=1
  fi
..
if test "${ac_default_ld_z_relro}" = unset; then
  ac_default_ld_z_relro=0
fi

Your suggestion doesn't work for ld even if the typo is fixed:

AC_ARG_ENABLE(relro,
        AS_HELP_STRING([--enable-relro],
                [enable -z relro in ELF linker by default]))
if test "${enable_relro}" = "yes"; then  <<<< Default is no.
  ac_default_ld_z_relro=1
else
  ac_default_ld_z_relro=0
fi

It is better to make ld and gold consistent if gold decides to
change the default depending on target like ld.

> Nick asked you to fix bugs in the ld version which my suggestion
> above already had fixed.
> -mike



-- 
H.J.