Re: [PATCH][SH] CFI directives patch

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi!

On Fri, 4 May 2012 15:03:04 +0800, Chung-Lin Tang <cltang@codesourcery.com> wrote:
> Hi Thomas, this patch adds CFI directives to epilogues in the SH port
> assembly files.

Thanks, generally looks ready to be applied!


> Some of the cancellation point calls were also modified to now use CFI
> assembler directives, instead of hard-coding the information.

I already pushed that as e1b4354e663fe7f68c96b6c6e72e55492bf38b91 --
please send such clean-up patches independently, whenever easily
possible.

Also, please indicate if/how you have tested the changes, helping your
friendly reviewer.  :-)


Some further comments:

I don't really feel strongly about this, but would it make sense to move
the cfi_remember_state instructions to the place just before the control
flow (potentially) branches?  It does not make any functional difference,
but might make things more clear conceptually.  Or, instead push it even
further forward, right to the place where the last CFI update is done,
like I had done in
<http://thread.gmane.org/gmane.comp.lib.glibc.ports/514/focus=518>, for
example?

> --- a/nptl/sysdeps/unix/sysv/linux/sh/sysdep-cancel.h
> +++ b/nptl/sysdeps/unix/sysv/linux/sh/sysdep-cancel.h
> @@ -71,6 +71,7 @@
>      CDISABLE; \
>      mov.l @r15+,r0; \
>      cfi_adjust_cfa_offset (-4); \
> +    cfi_restore (r0); \
>      lds.l @r15+,pr; \
>      cfi_adjust_cfa_offset (-4); \
>      cfi_restore (pr); \
> @@ -97,10 +98,10 @@
>  # define SAVE_ARGS_6	SAVE_ARGS_5
>  
>  # define LOAD_ARGS_0	/* Nothing.  */
> -# define LOAD_ARGS_1	LOAD_ARGS_0; mov.l @(0,r15),r4
> -# define LOAD_ARGS_2	LOAD_ARGS_1; mov.l @(4,r15),r5
> -# define LOAD_ARGS_3	LOAD_ARGS_2; mov.l @(8,r15),r6
> -# define LOAD_ARGS_4	LOAD_ARGS_3; mov.l @(12,r15),r7
> +# define LOAD_ARGS_1	LOAD_ARGS_0; mov.l @(0,r15),r4; cfi_restore (r4)
> +# define LOAD_ARGS_2	LOAD_ARGS_1; mov.l @(4,r15),r5; cfi_restore (r5)
> +# define LOAD_ARGS_3	LOAD_ARGS_2; mov.l @(8,r15),r6; cfi_restore (r6)
> +# define LOAD_ARGS_4	LOAD_ARGS_3; mov.l @(12,r15),r7; cfi_restore (r7)
>  # define LOAD_ARGS_5	LOAD_ARGS_4
>  # define LOAD_ARGS_6	LOAD_ARGS_5

Why do we need to care about these caller-save registers here?  You are
right that the current code is assymetric in that, for example,
Âcfi_rel_offset (r0, 0)Â is used but not Âcfi_restore (r0)Â -- but what
is the reason for doing the former in the first place (and likewise for
the others occurences/registers)?  And if it's not needed to preserve
these registers' content, should rather the Âcfi_rel_offsetÂs be removed?

> diff --git a/sysdeps/unix/sh/sysdep.S b/sysdeps/unix/sh/sysdep.S
> index e816575..939eb9a 100644
> --- a/sysdeps/unix/sh/sysdep.S
> +++ b/sysdeps/unix/sh/sysdep.S
> @@ -41,13 +41,15 @@ skip:
>  	sts.l	pr, @-r15
>  	cfi_adjust_cfa_offset (4)
>  	cfi_rel_offset (pr, 0)
> +	cfi_adjust_cfa_offset (4)
>  	jsr	@r1
>  	 mov.l	r0, @-r15
> -	cfi_adjust_cfa_offset (4)

Hmm?

> diff --git a/sysdeps/unix/sysv/linux/sh/sysdep.h b/sysdeps/unix/sysv/linux/sh/sysdep.h
> index 5215a84..f5e7524 100644
> --- a/sysdeps/unix/sysv/linux/sh/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/sh/sysdep.h
> @@ -120,14 +120,13 @@
>  #  endif
>  #  define SYSCALL_ERROR_HANDLER \
>  	neg r0,r1; \
> -	mov r12,r2; \
> -	mov.l 0f,r12; \
> +	mov.l 0f,r2; \
>  	mova 0f,r0; \
> -	add r0,r12; \
> +	add r0,r2; \
>  	mov.l 1f,r0; \
>  	stc gbr, r4; \
> -	mov.l @(r0,r12),r0; \
> -	mov r2,r12; \
> +	add r0,r2; \
> +	mov.l @r2,r0; \
>  	add r4,r0; \
>  	mov.l r1,@r0; \
>  	bra .Lpseudo_end; \
> @@ -139,13 +138,12 @@
>  /* Store (-r0) into errno through the GOT.  */
>  #  define SYSCALL_ERROR_HANDLER						      \
>  	neg r0,r1; \
> -	mov r12,r2; \
> -	mov.l 0f,r12; \
> +	mov.l 0f,r2; \
>  	mova 0f,r0; \
> -	add r0,r12; \
> +	add r0,r2; \
>  	mov.l 1f,r0; \
> -	mov.l @(r0,r12),r0; \
> -	mov r2,r12; \
> +	add r0,r2; \
> +	mov.l @r2,r0; \
>  	mov.l r1,@r0; \
>  	bra .Lpseudo_end; \
>  	 mov _IMM1,r0; \

Hmm.  In my understanding, r12 is the standard register to use for
accessing the GOT.  I think the linker is only able to optimize this
exact sequences.  Also, isn't there another case for RTLD_PRIVATE_ERRNO?
What about using Âcfi_register (r12, r2)Â, and later Âcfi_restore (r12)Â.


GrÃÃe,
 Thomas

Attachment: pgp00000.pgp
Description: PGP signature