Re: [PATCH] PowerPC: Add systemtap static probe points in setjmp/longjmp

[Will Schmidt <will_schmidt at vnet dot ibm dot com>]

On Thu, 2013-10-31 at 21:42 -0200, Adhemerval Zanella wrote:
> Hi all,
> 
> This patch add static probes for setjmp/longjmp in the way gdb expects. Along with
> another fix (discussed at https://sourceware.org/ml/gdb/2013-10/msg00191.html) this
> fixes the gdb.base/longjmp.exp gdb testcases.
> 
> I have to change the symbol_name and use macros to change them to avoid change the
> probe names and ending up adding more logic on GCC (since with the expected name
> GDB work seamlessly). They symbols name will be the same as before.
> 
> Tested on PPC64, PPC32, and PPC32 soft-fp.
> 
> ---
> 
> 2013-10-31  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
> 
> 	* sysdeps/powerpc/powerpc32/__longjmp-common.S: Add "longjmp" and
> 	"longjmp_target" static probes.
> 	(__longjmp): Rename to __longjmp_symbol.
> 	* sysdeps/powerpc/powerpc32/fpu/__longjmp-common.S: Likewise.
> 	* sysdeps/powerpc/powerpc32/__longjmp.S: Define __longjmp_symbol based
> 	on which longjmp to generate.
> 	* sysdeps/powerpc/powerpc32/fpu/__longjmp.S: Likewise.
> 	* sysdeps/powerpc/powerpc32/fpu/setjmp-common.S: Add "setjmp" static
> 	probe.
> 	(__sigsetjmp): Rename to __sigsetjmp_symbol.
> 	(__sigjmp_save): Rename to __sigjmp_save_symbol.
> 	* sysdeps/powerpc/powerpc32/setjmp-common.S: Likewise.
> 	* sysdeps/powerpc/powerpc32/fpu/setjmp.S: Define __sigsetjmp_symbol
> 	and __sigjmp_save_symbol based on which sigsetjmp to generated.
> 	* sysdeps/powerpc/powerpc32/setjmp.S: Likewise
> 	* sysdeps/unix/sysv/linux/powerpc/powerpc32/____longjmp_chk.S: Define
> 	__longjmp_symbol based on which __longjmp to generate.
> 	* sysdeps/powerpc/powerpc64/setjmp-common.S: Add "setjmp" static
> 	probe.
> 	(setjmp): Rename to setjmp_symbol.
> 	(__sigsetjmp): Rename to __sigsetjmp_symbol.
> 	(_setjmp): Rename to _setjmp_symbol.
> 	(__sigsetjmp): Rename to __sigsetjmp_symbol.
> 	* sysdeps/powerpc/powerpc64/setjmp.S: Define setjmp_symbol,
> 	_setjmp_symbol, __sigsetjmp_symbol, and __sigjmp_save_symbol based on
> 	which setjmp to generate.
> 	* sysdeps/powerpc/powerpc64/__longjmp-common.S: Add "longjmp" and
> 	"longjmp_target" static probes.
> 
> --

For ease of review, this could probably have been broken apart into two
patches.  1 - cosmetic/renaming from __longjmp to __longjmp_symbol, and
2 - the actual addition of the probes.  

Overall, I think this patch is OK.  Two clarification questions below.

Thanks, 
-Will


> 
> diff --git a/sysdeps/powerpc/powerpc32/__longjmp-common.S b/sysdeps/powerpc/powerpc32/__longjmp-common.S
> index df1d519..08eff37 100644
> --- a/sysdeps/powerpc/powerpc32/__longjmp-common.S
> +++ b/sysdeps/powerpc/powerpc32/__longjmp-common.S
> @@ -17,6 +17,7 @@
>     <http://www.gnu.org/licenses/>.  */
> 
>  #include <sysdep.h>
> +#include <stap-probe.h>
>  #define _ASM
>  #ifdef __NO_VMX__
>  # include <novmxsetjmp.h>
> @@ -30,7 +31,7 @@
>  # define LOAD_GP(N)	lwz r##N,((JB_GPRS+(N)-14)*4)(r3)
>  #endif
> 
> -ENTRY (__longjmp)
> +ENTRY (__longjmp_symbol)
> 
>  #if defined PTR_DEMANGLE || defined CHECK_SP
>  	lwz r24,(JB_GPR1*4)(r3)
> @@ -58,20 +59,22 @@ ENTRY (__longjmp)
>  # endif
>  	PTR_DEMANGLE2 (r0, r25)
>  #endif
> +	LIBC_PROBE (longjmp, 3, 4@3, -4@4, 4@0)
>  	mtlr r0
>  	LOAD_GP (21)
>  	LOAD_GP (22)
> -	lwz r0,(JB_CR*4)(r3)
> +	lwz r5,(JB_CR*4)(r3)
>  	LOAD_GP (23)
>  	LOAD_GP (24)
>  	LOAD_GP (25)
> -	mtcrf 0xFF,r0
> +	mtcrf 0xFF,r5
>  	LOAD_GP (26)
>  	LOAD_GP (27)
>  	LOAD_GP (28)
>  	LOAD_GP (29)
>  	LOAD_GP (30)
>  	LOAD_GP (31)
> +	LIBC_PROBE (longjmp_target, 3, 8@3, -4@4, 8@0)

Two questions here. 
Can you explain the use of r5 instead of r0 in the code above?  I'd
guess the probe wants something specific in R5, but want to confirm
that.
What are the 8@3, -4@4, 8@0 mapping to?  The values vary slightly
throughout, and may be worth a comment on the LIBC_PROBE call to clarify
the intent.    from a peek at LIBC_PROBE(), there is some _VA_ARGS_
activity on the other side of the macro.					

thanks, 
-Will