diff mbox

[rs6000] Pass --secure-plt to the linker

Message ID 5626A650.7080704@arm.com
State New
Headers show

Commit Message

Szabolcs Nagy Oct. 20, 2015, 8:38 p.m. UTC
On 20/10/15 06:16, Alan Modra wrote:
> On Mon, Oct 19, 2015 at 08:10:32PM +0100, Szabolcs Nagy wrote:
>> On 19/10/15 14:04, Szabolcs Nagy wrote:
>>> On 19/10/15 12:12, Alan Modra wrote:
>>>> On Thu, Oct 15, 2015 at 06:50:50PM +0100, Szabolcs Nagy wrote:
>>>>> A powerpc toolchain built with (or without) --enable-secureplt
>>>>> currently creates a binary that uses bss plt if
>>>>>
>>>>> (1) any of the linked PIC objects have bss plt relocs
>>>>> (2) or all the linked objects are non-PIC or have no relocs,
>>>>>
>>>>> because this is the binutils linker behaviour.
>>>>>
>>>>> This patch passes --secure-plt to the linker which makes the linker
>>>>> warn in case (1) and produce a binary with secure plt in case (2).
>>>>
>>>> The idea is OK I think, but
>>>>
>>>>> @@ -574,6 +577,7 @@ ENDIAN_SELECT(" -mbig", " -mlittle", DEFAULT_ASM_ENDIAN)
>>>>>   %{R*} \
>>>>>   %(link_shlib) \
>>>>>   %{!T*: %(link_start) } \
>>>>> +%{!static: %(link_secure_plt_default)} \
>>>>>   %(link_os)"
>>>>
>>>> this change needs to be conditional on !mbss-plt too.
>>>>
>>>
>>> OK, will change that.
>>>
>>> if -msecure-plt and -mbss-plt are supposed to affect
>>> linking too (not just code gen) then shall i add
>>> %{msecure-plt: --secure-plt} too?
>>>
>>
>> I added !mbss-plt only for now as a mix of -msecure-plt
>> and -mbss-plt options do not cancel each other in gcc,
>
> They do for code-gen since they share the same variable (see
> sysv4.opt), but I guess you meant as far as spec parsing goes.  In
> hindsight, it might have been better if I'd spelled -mbss-plt as
> -mno-secure-plt.
>
>> the patch only changes behaviour for a secureplt toolchain.
>>
>> OK to commit?
>
> Apologies for not thinking of this before when I first reviewed the
> patch, but have you bootstrapped this patch on powerpc64-linux?  I'm
> guessing not, because it occurs to me that --secure-plt is not a
> powerpc64-linux-ld option.  So if you try to build a biarch compiler
> with --enable-secure-plt then ld will complain when attempting to link
> 64-bit binaries.
>
> You'll want this on top of your patch:
>

Added, thanks. (and now built a powerpc64-none-linux-gnu cross
toolchain as well and checked if the correct flags are passed to
the linker with various -msecure-plt/-mbss-plt/-m32/-m64 flags.)

OK for trunk?

2015-10-20  Gregor Richards  <gregor.richards@uwaterloo.ca>
	    Szabolcs Nagy  <szabolcs.nagy@arm.com>

	* config/rs6000/secureplt.h (LINK_SECURE_PLT_DEFAULT_SPEC): Define.
	* config/rs6000/sysv4.h (LINK_SECURE_PLT_SPEC): Define.
	(LINK_SPEC): Add %(link_secure_plt).
	(SUBTARGET_EXTRA_SPECS): Add "link_secure_plt".
	* config/rs6000/linux64.h (LINK_SECURE_PLT_SPEC): Redefine.

Comments

David Edelsohn Oct. 20, 2015, 8:40 p.m. UTC | #1
On Tue, Oct 20, 2015 at 3:38 PM, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> On 20/10/15 06:16, Alan Modra wrote:
>>
>> On Mon, Oct 19, 2015 at 08:10:32PM +0100, Szabolcs Nagy wrote:
>>>
>>> On 19/10/15 14:04, Szabolcs Nagy wrote:
>>>>
>>>> On 19/10/15 12:12, Alan Modra wrote:
>>>>>
>>>>> On Thu, Oct 15, 2015 at 06:50:50PM +0100, Szabolcs Nagy wrote:
>>>>>>
>>>>>> A powerpc toolchain built with (or without) --enable-secureplt
>>>>>> currently creates a binary that uses bss plt if
>>>>>>
>>>>>> (1) any of the linked PIC objects have bss plt relocs
>>>>>> (2) or all the linked objects are non-PIC or have no relocs,
>>>>>>
>>>>>> because this is the binutils linker behaviour.
>>>>>>
>>>>>> This patch passes --secure-plt to the linker which makes the linker
>>>>>> warn in case (1) and produce a binary with secure plt in case (2).
>>>>>
>>>>>
>>>>> The idea is OK I think, but
>>>>>
>>>>>> @@ -574,6 +577,7 @@ ENDIAN_SELECT(" -mbig", " -mlittle",
>>>>>> DEFAULT_ASM_ENDIAN)
>>>>>>   %{R*} \
>>>>>>   %(link_shlib) \
>>>>>>   %{!T*: %(link_start) } \
>>>>>> +%{!static: %(link_secure_plt_default)} \
>>>>>>   %(link_os)"
>>>>>
>>>>>
>>>>> this change needs to be conditional on !mbss-plt too.
>>>>>
>>>>
>>>> OK, will change that.
>>>>
>>>> if -msecure-plt and -mbss-plt are supposed to affect
>>>> linking too (not just code gen) then shall i add
>>>> %{msecure-plt: --secure-plt} too?
>>>>
>>>
>>> I added !mbss-plt only for now as a mix of -msecure-plt
>>> and -mbss-plt options do not cancel each other in gcc,
>>
>>
>> They do for code-gen since they share the same variable (see
>> sysv4.opt), but I guess you meant as far as spec parsing goes.  In
>> hindsight, it might have been better if I'd spelled -mbss-plt as
>> -mno-secure-plt.
>>
>>> the patch only changes behaviour for a secureplt toolchain.
>>>
>>> OK to commit?
>>
>>
>> Apologies for not thinking of this before when I first reviewed the
>> patch, but have you bootstrapped this patch on powerpc64-linux?  I'm
>> guessing not, because it occurs to me that --secure-plt is not a
>> powerpc64-linux-ld option.  So if you try to build a biarch compiler
>> with --enable-secure-plt then ld will complain when attempting to link
>> 64-bit binaries.
>>
>> You'll want this on top of your patch:
>>
>
> Added, thanks. (and now built a powerpc64-none-linux-gnu cross
> toolchain as well and checked if the correct flags are passed to
> the linker with various -msecure-plt/-mbss-plt/-m32/-m64 flags.)
>
> OK for trunk?
>
> 2015-10-20  Gregor Richards  <gregor.richards@uwaterloo.ca>
>             Szabolcs Nagy  <szabolcs.nagy@arm.com>
>
>         * config/rs6000/secureplt.h (LINK_SECURE_PLT_DEFAULT_SPEC): Define.
>         * config/rs6000/sysv4.h (LINK_SECURE_PLT_SPEC): Define.
>         (LINK_SPEC): Add %(link_secure_plt).
>         (SUBTARGET_EXTRA_SPECS): Add "link_secure_plt".
>         * config/rs6000/linux64.h (LINK_SECURE_PLT_SPEC): Redefine.
>

I'm okay if Alan is satisfied.

Thanks, David
Alan Modra Oct. 20, 2015, 11:43 p.m. UTC | #2
On Tue, Oct 20, 2015 at 03:40:14PM -0500, David Edelsohn wrote:
> On Tue, Oct 20, 2015 at 3:38 PM, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > 2015-10-20  Gregor Richards  <gregor.richards@uwaterloo.ca>
> >             Szabolcs Nagy  <szabolcs.nagy@arm.com>
> >
> >         * config/rs6000/secureplt.h (LINK_SECURE_PLT_DEFAULT_SPEC): Define.
> >         * config/rs6000/sysv4.h (LINK_SECURE_PLT_SPEC): Define.
> >         (LINK_SPEC): Add %(link_secure_plt).
> >         (SUBTARGET_EXTRA_SPECS): Add "link_secure_plt".
> >         * config/rs6000/linux64.h (LINK_SECURE_PLT_SPEC): Redefine.
> >
> 
> I'm okay if Alan is satisfied.

Committed revision 229102.
diff mbox

Patch

diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h
index 9599735..01fb880 100644
--- a/gcc/config/rs6000/linux64.h
+++ b/gcc/config/rs6000/linux64.h
@@ -174,20 +174,24 @@  extern int dot_symbols;
 #undef	ASM_DEFAULT_SPEC
 #undef	ASM_SPEC
 #undef	LINK_OS_LINUX_SPEC
+#undef	LINK_SECURE_PLT_SPEC
 
 #ifndef	RS6000_BI_ARCH
 #define	ASM_DEFAULT_SPEC "-mppc64"
 #define	ASM_SPEC	 "%(asm_spec64) %(asm_spec_common)"
 #define	LINK_OS_LINUX_SPEC "%(link_os_linux_spec64)"
+#define	LINK_SECURE_PLT_SPEC ""
 #else
 #if DEFAULT_ARCH64_P
 #define	ASM_DEFAULT_SPEC "-mppc%{!m32:64}"
 #define	ASM_SPEC	 "%{m32:%(asm_spec32)}%{!m32:%(asm_spec64)} %(asm_spec_common)"
 #define	LINK_OS_LINUX_SPEC "%{m32:%(link_os_linux_spec32)}%{!m32:%(link_os_linux_spec64)}"
+#define	LINK_SECURE_PLT_SPEC "%{m32: " LINK_SECURE_PLT_DEFAULT_SPEC "}"
 #else
 #define	ASM_DEFAULT_SPEC "-mppc%{m64:64}"
 #define	ASM_SPEC	 "%{!m64:%(asm_spec32)}%{m64:%(asm_spec64)} %(asm_spec_common)"
 #define	LINK_OS_LINUX_SPEC "%{!m64:%(link_os_linux_spec32)}%{m64:%(link_os_linux_spec64)}"
+#define	LINK_SECURE_PLT_SPEC "%{!m64: " LINK_SECURE_PLT_DEFAULT_SPEC "}"
 #endif
 #endif
 
diff --git a/gcc/config/rs6000/secureplt.h b/gcc/config/rs6000/secureplt.h
index b463463..77edf2a 100644
--- a/gcc/config/rs6000/secureplt.h
+++ b/gcc/config/rs6000/secureplt.h
@@ -18,3 +18,4 @@  along with GCC; see the file COPYING3.  If not see
 <http://www.gnu.org/licenses/>.  */
 
 #define CC1_SECURE_PLT_DEFAULT_SPEC "-msecure-plt"
+#define LINK_SECURE_PLT_DEFAULT_SPEC "--secure-plt"
diff --git a/gcc/config/rs6000/sysv4.h b/gcc/config/rs6000/sysv4.h
index 7b2f9bd..1bb400f 100644
--- a/gcc/config/rs6000/sysv4.h
+++ b/gcc/config/rs6000/sysv4.h
@@ -537,6 +537,9 @@  ENDIAN_SELECT(" -mbig", " -mlittle", DEFAULT_ASM_ENDIAN)
 #ifndef CC1_SECURE_PLT_DEFAULT_SPEC
 #define CC1_SECURE_PLT_DEFAULT_SPEC ""
 #endif
+#ifndef LINK_SECURE_PLT_DEFAULT_SPEC
+#define LINK_SECURE_PLT_DEFAULT_SPEC ""
+#endif
 
 /* Pass -G xxx to the compiler.  */
 #undef CC1_SPEC
@@ -567,6 +570,7 @@  ENDIAN_SELECT(" -mbig", " -mlittle", DEFAULT_ASM_ENDIAN)
                : %(link_start_default)     }"
 
 #define LINK_START_DEFAULT_SPEC ""
+#define LINK_SECURE_PLT_SPEC LINK_SECURE_PLT_DEFAULT_SPEC
 
 #undef	LINK_SPEC
 #define	LINK_SPEC "\
@@ -574,6 +578,7 @@  ENDIAN_SELECT(" -mbig", " -mlittle", DEFAULT_ASM_ENDIAN)
 %{R*} \
 %(link_shlib) \
 %{!T*: %(link_start) } \
+%{!static: %{!mbss-plt: %(link_secure_plt)}} \
 %(link_os)"
 
 /* Shared libraries are not default.  */
@@ -889,6 +894,7 @@  ncrtn.o%s"
   { "link_os_openbsd",		LINK_OS_OPENBSD_SPEC },			\
   { "link_os_default",		LINK_OS_DEFAULT_SPEC },			\
   { "cc1_secure_plt_default",	CC1_SECURE_PLT_DEFAULT_SPEC },		\
+  { "link_secure_plt",		LINK_SECURE_PLT_SPEC },			\
   { "cpp_os_ads",		CPP_OS_ADS_SPEC },			\
   { "cpp_os_yellowknife",	CPP_OS_YELLOWKNIFE_SPEC },		\
   { "cpp_os_mvme",		CPP_OS_MVME_SPEC },			\