Fix ____longjmp_chk for ppc(64)

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

Fix ____longjmp_chk for ppc(64)

Andreas Schwab-3
The ppc32 libc.so contains text relocations due to the non-pic load of
the string in ____longjmp_chk.  Also, backtrace from within
__fortify_fail didn't work.

Andreas.

2009-06-24  Andreas Schwab  <[hidden email]>

        * sysdeps/powerpc/powerpc32/____longjmp_chk.S (LOAD_ARG): Define.
        (CHECK_SP): Use it.  Save lr before call.
        * sysdeps/powerpc/powerpc64/____longjmp_chk.S (CHECK_SP): Save lr
        before call.

diff --git i/sysdeps/powerpc/powerpc32/____longjmp_chk.S w/sysdeps/powerpc/powerpc32/____longjmp_chk.S
index 5c1f648..b358058 100644
--- i/sysdeps/powerpc/powerpc32/____longjmp_chk.S
+++ w/sysdeps/powerpc/powerpc32/____longjmp_chk.S
@@ -26,12 +26,39 @@
 
 #define __longjmp ____longjmp_chk
 
+#ifdef PIC
+# ifdef HAVE_ASM_PPC_REL16
+#  define LOAD_ARG \
+ bcl 20,31,1f; \
+1: mflr r3; \
+ addis r3,r3,_GLOBAL_OFFSET_TABLE_-1b@ha; \
+ addi r3,r3,_GLOBAL_OFFSET_TABLE_-1b@l; \
+ lwz r3,.LC0@got(r3)
+# else
+#  define LOAD_ARG \
+ bl _GLOBAL_OFFSET_TABLE_-4@local; \
+ mflr r3; \
+ lwz r3,.LC0@got(r3)
+# endif
+#else
+# define LOAD_ARG \
+ lis r3,.LC0@ha; \
+ la r3,.LC0@l(r3)
+#endif
+
 #define CHECK_SP(reg) \
  cmplw reg, r1; \
  bge+ .Lok; \
- lis r3,.LC0@ha; \
- la r3,.LC0@l(r3); \
+ mflr r0; \
+ stwu r1,-16(r1); \
+ cfi_adjust_cfa_offset (16); \
+ stw r0,20(r1); \
+ cfi_offset (lr, 4); \
+ LOAD_ARG; \
  bl HIDDEN_JUMPTARGET (__fortify_fail); \
+ addi r1,r1,16; \
+ cfi_adjust_cfa_offset (-16); \
+ cfi_same_value (lr); \
 .Lok:
 
 #include <__longjmp-common.S>
diff --git i/sysdeps/powerpc/powerpc64/____longjmp_chk.S w/sysdeps/powerpc/powerpc64/____longjmp_chk.S
index 5654902..746717c 100644
--- i/sysdeps/powerpc/powerpc64/____longjmp_chk.S
+++ w/sysdeps/powerpc/powerpc64/____longjmp_chk.S
@@ -32,8 +32,16 @@
 #define CHECK_SP(reg) \
  cmpld reg, r1; \
  bge+ .Lok; \
+ mflr r0; \
+ std r0,16(r1); \
+ stdu r1,-112(r1); \
+ cfi_adjust_cfa_offset (112); \
+ cfi_offset (lr, 16); \
  ld r3,.LC1@toc(2); \
  bl HIDDEN_JUMPTARGET (__fortify_fail); \
+ addi r1,r1,112; \
+ cfi_adjust_cfa_offset (-112); \
+ cfi_same_value (lr); \
 .Lok:
 
 #include <__longjmp-common.S>

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Richard Henderson
On 06/24/2009 05:27 AM, Andreas Schwab wrote:

> + mflr r0; \
> + std r0,16(r1); \
> + stdu r1,-112(r1); \
> + cfi_adjust_cfa_offset (112); \
> + cfi_offset (lr, 16); \
>   ld r3,.LC1@toc(2); \
>   bl HIDDEN_JUMPTARGET (__fortify_fail); \
> + addi r1,r1,112; \
> + cfi_adjust_cfa_offset (-112); \
> + cfi_same_value (lr); \
>   .Lok:

__fortify_fail doesn't return, does it?  So, why are you
adding the stack adjustment after the return?


r~
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Andreas Schwab-3
Richard Henderson <[hidden email]> writes:

> On 06/24/2009 05:27 AM, Andreas Schwab wrote:
>> + mflr r0; \
>> + std r0,16(r1); \
>> + stdu r1,-112(r1); \
>> + cfi_adjust_cfa_offset (112); \
>> + cfi_offset (lr, 16); \
>>   ld r3,.LC1@toc(2); \
>>   bl HIDDEN_JUMPTARGET (__fortify_fail); \
>> + addi r1,r1,112; \
>> + cfi_adjust_cfa_offset (-112); \
>> + cfi_same_value (lr); \
>>   .Lok:
>
> __fortify_fail doesn't return, does it?  So, why are you
> adding the stack adjustment after the return?

Just to be able to attach the cfi to it.  It would be wrong to attach it
to the branch.

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Richard Henderson
On 06/24/2009 08:46 AM, Andreas Schwab wrote:

> Richard Henderson<[hidden email]>  writes:
>
>> On 06/24/2009 05:27 AM, Andreas Schwab wrote:
>>> + mflr r0; \
>>> + std r0,16(r1); \
>>> + stdu r1,-112(r1); \
>>> + cfi_adjust_cfa_offset (112); \
>>> + cfi_offset (lr, 16); \
>>>     ld r3,.LC1@toc(2); \
>>>     bl HIDDEN_JUMPTARGET (__fortify_fail); \
>>> + addi r1,r1,112; \
>>> + cfi_adjust_cfa_offset (-112); \
>>> + cfi_same_value (lr); \
>>>    .Lok:
>> __fortify_fail doesn't return, does it?  So, why are you
>> adding the stack adjustment after the return?
>
> Just to be able to attach the cfi to it.  It would be wrong to attach it
> to the branch.

Ah.  Then perhaps a trap insn might be more
appropriate (and slightly less confusing)?


r~
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Andreas Schwab-3
Richard Henderson <[hidden email]> writes:

> Ah.  Then perhaps a trap insn might be more
> appropriate (and slightly less confusing)?

How about this?

  bl HIDDEN_JUMPTARGET (__fortify_fail); \
+ /* __fortify_fail doesn't return, but we need \
+   an insn to attach the cfi to it.  */ \
+ nop; \
+ cfi_adjust_cfa_offset (-112); \
+ cfi_same_value (lr); \
 .Lok:

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Jakub Jelinek
On Wed, Jun 24, 2009 at 06:30:50PM +0200, Andreas Schwab wrote:

> Richard Henderson <[hidden email]> writes:
>
> > Ah.  Then perhaps a trap insn might be more
> > appropriate (and slightly less confusing)?
>
> How about this?
>
>   bl HIDDEN_JUMPTARGET (__fortify_fail); \
> + /* __fortify_fail doesn't return, but we need \
> +   an insn to attach the cfi to it.  */ \
> + nop; \
> + cfi_adjust_cfa_offset (-112); \
> + cfi_same_value (lr); \
>  .Lok:

Wouldn't be better to do:
  bl HIDDEN_JUMPTARGET (__fortify_fail); \
 .Lok: \
        mr r1, reg; \
+ cfi_adjust_cfa_offset (-112); \
+ cfi_same_value (lr)

and remove the mr r1, r14 resp. mr r1, r22 from __longjmp-common.S?
Then you don't need to add any extra insn.

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Richard Henderson
On 06/24/2009 10:15 AM, Jakub Jelinek wrote:
>   bl HIDDEN_JUMPTARGET (__fortify_fail); \
>   .Lok: \
> mr r1, reg; \
> + cfi_adjust_cfa_offset (-112); \
> + cfi_same_value (lr)
>
> and remove the mr r1, r14 resp. mr r1, r22 from __longjmp-common.S?
> Then you don't need to add any extra insn.

The cfa is incorrect for the duration of the mr r1 insn.

No, I like Andreas' nop+comment version.


r~
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Daniel Jacobowitz-2
On Wed, Jun 24, 2009 at 10:28:39AM -0700, Richard Henderson wrote:

> On 06/24/2009 10:15 AM, Jakub Jelinek wrote:
> >   bl HIDDEN_JUMPTARGET (__fortify_fail); \
> >  .Lok: \
> > mr r1, reg; \
> >+ cfi_adjust_cfa_offset (-112); \
> >+ cfi_same_value (lr)
> >
> >and remove the mr r1, r14 resp. mr r1, r22 from __longjmp-common.S?
> >Then you don't need to add any extra insn.
>
> The cfa is incorrect for the duration of the mr r1 insn.

Which mr - the one Jakub quoted above or the one in
__longjmp-common.S?  IIUC the CFI doesn't attach to the mr
in the above, but to the following instruction.  So I don't understand
why the nop is required.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Jakub Jelinek
In reply to this post by Richard Henderson
On Wed, Jun 24, 2009 at 10:28:39AM -0700, Richard Henderson wrote:

> On 06/24/2009 10:15 AM, Jakub Jelinek wrote:
>>   bl HIDDEN_JUMPTARGET (__fortify_fail); \
>>   .Lok: \
>> mr r1, reg; \
>> + cfi_adjust_cfa_offset (-112); \
>> + cfi_same_value (lr)
>>
>> and remove the mr r1, r14 resp. mr r1, r22 from __longjmp-common.S?
>> Then you don't need to add any extra insn.
>
> The cfa is incorrect for the duration of the mr r1 insn.

Ah, but in that case can't be the cfi directives be right after bl?
The unwinders subtract one when looking for which cfi directives to apply,
unless in signal frame:
  /* The comparison with the return address uses < rather than <= because
     we are only interested in the effects of code before the call; for a
     noreturn function, the return address may point to unrelated code with
     a different stack configuration that we are not interested in.  We
     assume that the call itself is unwind info-neutral; if not, or if
     there are delay instructions that adjust the stack, these must be
     reflected at the point immediately before the call insn.
     In signal frames, return address is after last completed instruction,
     so we add 1 to return address to make the comparison <=.  */
  while (insn_ptr < insn_end
         && fs->pc < context->ra + _Unwind_IsSignalFrame (context))
...

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Richard Henderson
On 06/24/2009 10:55 AM, Jakub Jelinek wrote:

> On Wed, Jun 24, 2009 at 10:28:39AM -0700, Richard Henderson wrote:
>> On 06/24/2009 10:15 AM, Jakub Jelinek wrote:
>>>     bl HIDDEN_JUMPTARGET (__fortify_fail); \
>>>    .Lok: \
>>> mr r1, reg; \
>>> + cfi_adjust_cfa_offset (-112); \
>>> + cfi_same_value (lr)
>>>
>>> and remove the mr r1, r14 resp. mr r1, r22 from __longjmp-common.S?
>>> Then you don't need to add any extra insn.
>> The cfa is incorrect for the duration of the mr r1 insn.
>
> Ah, but in that case can't be the cfi directives be right after bl?
> The unwinders subtract one when looking for which cfi directives to apply,
> unless in signal frame:

Um... Yes, I think they can.  I thought I remembered
something odd about noreturn functions, but perhaps
that was end-of-function calls interacting with the
following function.


r~
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Steve Munroe
In reply to this post by Andreas Schwab-3

[hidden email] wrote on 06/24/2009 07:27:13 AM:

> The ppc32 libc.so contains text relocations due to the non-pic load of
> the string in ____longjmp_chk.  Also, backtrace from within
> __fortify_fail didn't work.
>
> Andreas.
>
> 2009-06-24  Andreas Schwab  <[hidden email]>
>
>    * sysdeps/powerpc/powerpc32/____longjmp_chk.S (LOAD_ARG): Define.
>    (CHECK_SP): Use it.  Save lr before call.
>    * sysdeps/powerpc/powerpc64/____longjmp_chk.S (CHECK_SP): Save lr
>    before call.
>
> diff --git i/sysdeps/powerpc/powerpc32/____longjmp_chk.S
> w/sysdeps/powerpc/powerpc32/____longjmp_chk.S
> index 5c1f648..b358058 100644
> --- i/sysdeps/powerpc/powerpc32/____longjmp_chk.S
> +++ w/sysdeps/powerpc/powerpc32/____longjmp_chk.S
> @@ -26,12 +26,39 @@
>

This code is not really used by linux except for embedded. The real version
should be in sysdeps/powerpc/powerpc32/fpu/

The original fpu/__longjmp.S had the appropriate PIC implementation.

not sure where sysdeps/powerpc/powerpc32/____longjmp_chk.S can from or what
you did build it versus sysdeps/powerpc/powerpc32/fpu/____longjmp_chk.S Of
is that missing?


Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Andreas Schwab-2
Steve Munroe <[hidden email]> writes:

> not sure where sysdeps/powerpc/powerpc32/____longjmp_chk.S can from or what
> you did build it versus sysdeps/powerpc/powerpc32/fpu/____longjmp_chk.S Of
> is that missing?

There is no need for a separate file for fpu since they would be
identical.

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Andreas Schwab-2
In reply to this post by Jakub Jelinek
Jakub Jelinek <[hidden email]> writes:

> On Wed, Jun 24, 2009 at 10:28:39AM -0700, Richard Henderson wrote:
>> On 06/24/2009 10:15 AM, Jakub Jelinek wrote:
>>>   bl HIDDEN_JUMPTARGET (__fortify_fail); \
>>>   .Lok: \
>>> mr r1, reg; \
>>> + cfi_adjust_cfa_offset (-112); \
>>> + cfi_same_value (lr)
>>>
>>> and remove the mr r1, r14 resp. mr r1, r22 from __longjmp-common.S?
>>> Then you don't need to add any extra insn.
>>
>> The cfa is incorrect for the duration of the mr r1 insn.
>
> Ah, but in that case can't be the cfi directives be right after bl?

I tried that, but then gdb cannot backtrace through ____longjmp_chk any
more (it says "Backtrace stopped: frame did not save the PC").

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Steve Munroe
In reply to this post by Andreas Schwab-2


Andreas Schwab <[hidden email]> wrote on 06/24/2009 04:23:31 PM:

> Steve Munroe <[hidden email]> writes:
>
> > not sure where sysdeps/powerpc/powerpc32/____longjmp_chk.S can from or
what
> > you did build it versus sysdeps/powerpc/powerpc32/fpu/____longjmp_chk.S
Of
> > is that missing?
>
> There is no need for a separate file for fpu since they would be
> identical.
>
Only if it is including powerpc32/fpu/__longjmp-common.S

The generated binary will not be identical (includes non-volatile FPRs and
VRs in the jumpbuf) There is a reason for this separation including the
versioning of the interface for the VMX add.

Steven J. Munroe
Linux on Power Toolchain Architect
IBM Corporation, Linux Technology Center

Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Andreas Schwab-2
Steve Munroe <[hidden email]> writes:

> The generated binary will not be identical (includes non-volatile FPRs and
> VRs in the jumpbuf) There is a reason for this separation including the
> versioning of the interface for the VMX add.

Since the __longjmp_chk function is new, there is no need for it to
support the old jumpbuf.

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Jakub Jelinek
In reply to this post by Andreas Schwab-2
On Wed, Jun 24, 2009 at 11:56:41PM +0200, Andreas Schwab wrote:

> Jakub Jelinek <[hidden email]> writes:
>
> > On Wed, Jun 24, 2009 at 10:28:39AM -0700, Richard Henderson wrote:
> >> On 06/24/2009 10:15 AM, Jakub Jelinek wrote:
> >>>   bl HIDDEN_JUMPTARGET (__fortify_fail); \
> >>>   .Lok: \
> >>> mr r1, reg; \
> >>> + cfi_adjust_cfa_offset (-112); \
> >>> + cfi_same_value (lr)
> >>>
> >>> and remove the mr r1, r14 resp. mr r1, r22 from __longjmp-common.S?
> >>> Then you don't need to add any extra insn.
> >>
> >> The cfa is incorrect for the duration of the mr r1 insn.
> >
> > Ah, but in that case can't be the cfi directives be right after bl?
>
> I tried that, but then gdb cannot backtrace through ____longjmp_chk any
> more (it says "Backtrace stopped: frame did not save the PC").

For noreturn calls which can't be tail-called GCC also emits .cfi_*
directives right after the call (whether it is .cfi_endproc or e.g.
.cfi_restore_state), so I'd say if gdb doesn't handle it, we should fix gdb.
Also, cfi_remember_state/cfi_restore_state pair would be more compact
than cfi_adjust_cfa_offset (-112); cfi_same_value (lr).

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Joseph Myers
In reply to this post by Steve Munroe
On Wed, 24 Jun 2009, Steve Munroe wrote:

> Andreas Schwab <[hidden email]> wrote on 06/24/2009 04:23:31 PM:
>
> > Steve Munroe <[hidden email]> writes:
> >
> > > not sure where sysdeps/powerpc/powerpc32/____longjmp_chk.S can from or
> what
> > > you did build it versus sysdeps/powerpc/powerpc32/fpu/____longjmp_chk.S
> Of
> > > is that missing?
> >
> > There is no need for a separate file for fpu since they would be
> > identical.
> >
> Only if it is including powerpc32/fpu/__longjmp-common.S

It is using "#include <__longjmp-common.S>" which means it will find the
first __longjmp-common.S in the sysdeps search path, which is the fpu/
version for configurations using that directory.  Other targets with
multiple __longjmp implementations (e.g. SH) do the same thing with a
single ____longjmp_chk implementation.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Daniel Jacobowitz-2
In reply to this post by Andreas Schwab-2
On Thu, Jun 25, 2009 at 12:44:45AM +0200, Andreas Schwab wrote:
> Steve Munroe <[hidden email]> writes:
>
> > The generated binary will not be identical (includes non-volatile FPRs and
> > VRs in the jumpbuf) There is a reason for this separation including the
> > versioning of the interface for the VMX add.
>
> Since the __longjmp_chk function is new, there is no need for it to
> support the old jumpbuf.

I believe that if you build for a non-FPU processor, you get a
different sized jmpbuf...

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: Fix ____longjmp_chk for ppc(64)

Andreas Schwab-3
Daniel Jacobowitz <[hidden email]> writes:

> On Thu, Jun 25, 2009 at 12:44:45AM +0200, Andreas Schwab wrote:
>> Steve Munroe <[hidden email]> writes:
>>
>> > The generated binary will not be identical (includes non-volatile FPRs and
>> > VRs in the jumpbuf) There is a reason for this separation including the
>> > versioning of the interface for the VMX add.
>>
>> Since the __longjmp_chk function is new, there is no need for it to
>> support the old jumpbuf.
>
> I believe that if you build for a non-FPU processor, you get a
> different sized jmpbuf...

In which way does that contradict my statement?

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."