Re: [PATCH][SH] SH CFI directives patch, revised

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi!

On Thu, 17 May 2012 18:33:08 +0800, Chung-Lin Tang <cltang@codesourcery.com> wrote:
> Here's an updated set of patches for SH CFI annotations, asides from the
> parts already committed by Thomas (the CFI modifications for
> cancellation point routines).

Thanks, I committed most of it.


> 3) The SYSCALL_ERROR_HANDLER macro in
> sysdeps/unix/sysv/linux/sh/sysdep.h has been changed to use CFI
> directives to mark r12's save/restore.  The RTLD_PRIVATE_ERRNO version
> is not dealt with, because simply, r12 there is never saved.

Hmm, I don't quite understand how that code can get away with not
preserving r12 -- Kaz, do you have an explanation?  The code in question:

    #ifndef PIC
    # define SYSCALL_ERROR_HANDLER  \
            mov.l 0f,r1; \
            jmp @r1; \
             mov r0,r4; \
            .align 2; \
         0: .long __syscall_error
    #else
    # if RTLD_PRIVATE_ERRNO
    #  define SYSCALL_ERROR_HANDLER \
            neg r0,r1; \
            mov.l 0f,r12; \
            mova 0f,r0; \
            add r0,r12; \
            mov.l 1f,r0; \
            mov.l r1,@(r0,r12); \
            bra .Lpseudo_end; \
             mov _IMM1,r0; \
            .align 2; \
         0: .long _GLOBAL_OFFSET_TABLE_; \
         1: .long rtld_errno@GOTOFF
    
    # elif defined _LIBC_REENTRANT
    
    #  ifndef NOT_IN_libc
    #   define SYSCALL_ERROR_ERRNO __libc_errno
    #  else
    #   define SYSCALL_ERROR_ERRNO errno
    #  endif
    #  define SYSCALL_ERROR_HANDLER [...]
    # else [...]

> diff --git a/sysdeps/unix/sysv/linux/sh/sysdep.h b/sysdeps/unix/sysv/linux/sh/sysdep.h
> index 5215a84..a492d1f 100644
> --- a/sysdeps/unix/sysv/linux/sh/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/sh/sysdep.h
> @@ -121,6 +121,8 @@
>  #  define SYSCALL_ERROR_HANDLER \
>  	neg r0,r1; \
>  	mov r12,r2; \
> +	cfi_remember_state; \
> +	cfi_register (r12, r2); \
>  	mov.l 0f,r12; \
>  	mova 0f,r0; \
>  	add r0,r12; \
> @@ -128,6 +130,7 @@
>  	stc gbr, r4; \
>  	mov.l @(r0,r12),r0; \
>  	mov r2,r12; \
> +	cfi_restore_state; \
>  	add r4,r0; \
>  	mov.l r1,@r0; \
>  	bra .Lpseudo_end; \

(I have not yet committed that.)  Wouldn't it be yet simpler to just use
Âcfi_register (r12, r2)Â and later Âcfi_restore (r12)Â, and avoid the
remember/restore cycle?  (No need to send a patch if you agree.)


Two additional changes that I have done:

diff --git a/nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.S b/nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.S
index dbdf6a8..0177209 100644
--- a/nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.S
+++ b/nptl/sysdeps/unix/sysv/linux/sh/lowlevellock.S
@@ -313,6 +313,8 @@ __lll_timedlock_wait:
 	cmp/hs	r0, r1
 	bt	3f
 
+	cfi_remember_state
+
 	mov.l	r11, @-r15
 	cfi_adjust_cfa_offset(4)
 	cfi_rel_offset (r11, 0)
@@ -393,7 +395,6 @@ __lll_timedlock_wait:
 2:	mov	#ETIMEDOUT, r3
 
 6:
-	cfi_remember_state
 	add	#8, r15
 	cfi_adjust_cfa_offset (-8)
 	mov.l	@r15+, r8

Otherwise the CFI state is wrong when jump label 3 is reached.


diff --git a/nptl/sysdeps/unix/sysv/linux/sh/lowlevelrobustlock.S b/nptl/sysdeps/unix/sysv/linux/sh/lowlevelrobustlock.S
index e3aaeee..7a192a9 100644
--- a/nptl/sysdeps/unix/sysv/linux/sh/lowlevelrobustlock.S
+++ b/nptl/sysdeps/unix/sysv/linux/sh/lowlevelrobustlock.S
@@ -263,6 +263,7 @@ __lll_robust_timedlock_wait:
 	cfi_restore (r8)
 	cfi_restore (r9)
 	cfi_restore (r10)
+	cfi_restore (r11)
 	cfi_def_cfa_offset (0)
 	rts
 	 mov	#EINVAL, r0

Should be obvious.

But, in this case, wouldn't it be simpler to use a nested
Â.cfi_remember_state [...] .cfi_remember_state [...] .cfi_restore_state
[...]  .cfi_restore_state instead of restoring the state manually (as
quoted)?  My reading of GDB's dwarf2-frame seems to confirm that this is
permissible.  (No need to send a patch if you agree.  I'd also prepare a
patch to improve the binutils documentation.)


GrÃÃe,
 Thomas

Attachment: pgp00000.pgp
Description: PGP signature