Re: [PATCH] rs6000: Compute rop_hash_save_offset for non-Altivec compiles [PR115389]

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Peter,

on 2024/6/8 12:06, Peter Bergner wrote:
> We currently only compute the offset for the ROP hash save location in
> the stack frame for Altivec compiles.  For non-Altivec compiles when we
> emit ROP mitigation instructions, we use a default offset of zero which
> corresponds to the backchain save location which will get clobbered on
> any call.  The fix is to compute the ROP hash save location for all
> compiles.

Thanks for fixing this.

> 
> This passed bootstrap and regtesting on powerpc64le-linux.
> Ok for trunk and backports after some burn-in time?
> 
> Peter
> 
> 
> gcc/
> 	PR target/115389
> 	* config/rs6000/rs6000-logue.cc (rs6000_stack_info): Compute
> 	rop_hash_save_offset for non-Altivec compiles.
> 
> gcc/testsuite/
> 	PR target/115389
> 	* gcc.target/powerpc/pr115389.c: New test.
> 
> diff --git a/gcc/config/rs6000/rs6000-logue.cc b/gcc/config/rs6000/rs6000-logue.cc
> index d61a25a5126..cfa8a67a5f3 100644
> --- a/gcc/config/rs6000/rs6000-logue.cc
> +++ b/gcc/config/rs6000/rs6000-logue.cc
> @@ -826,7 +826,14 @@ rs6000_stack_info (void)
>  	  info->ehrd_offset -= info->rop_hash_size;
>  	}
>        else
> -	info->ehrd_offset = info->gp_save_offset - ehrd_size;
> +	{
> +	  info->ehrd_offset = info->gp_save_offset - ehrd_size;
> +
> +	  /* Adjust for ROP protection.  */
> +	  info->rop_hash_save_offset
> +	    = info->gp_save_offset - info->rop_hash_size;
> +	  info->ehrd_offset -= info->rop_hash_size;
> +	}

I understand this is just copied from the if arm, but if I read this right, it can be
simplified as:

diff --git a/gcc/config/rs6000/rs6000-logue.cc b/gcc/config/rs6000/rs6000-logue.cc
index bd5d56ba002..2559c974c6e 100644
--- a/gcc/config/rs6000/rs6000-logue.cc
+++ b/gcc/config/rs6000/rs6000-logue.cc
@@ -817,17 +817,14 @@ rs6000_stack_info (void)
 	  gcc_assert (info->altivec_size == 0
 		      || info->altivec_save_offset % 16 == 0);

-	  /* Adjust for AltiVec case.  */
-	  info->ehrd_offset = info->altivec_save_offset - ehrd_size;
-
- 	  /* Adjust for ROP protection.  */
 	  info->rop_hash_save_offset
 	    = info->altivec_save_offset - info->rop_hash_size;
-	  info->ehrd_offset -= info->rop_hash_size;
 	}
       else
-	info->ehrd_offset = info->gp_save_offset - ehrd_size;
+	info->rop_hash_save_offset = info->gp_save_offset - info->rop_hash_size;

+      info->ehrd_offset = info->rop_hash_save_offset - ehrd_size;
       info->ehcr_offset = info->ehrd_offset - ehcr_size;
       info->cr_save_offset = reg_size; /* first word when 64-bit.  */
       info->lr_save_offset = 2*reg_size;

, both if and else have info->rop_hash_save_offset as the base for offset computation further.

>  
>        info->ehcr_offset = info->ehrd_offset - ehcr_size;
>        info->cr_save_offset = reg_size; /* first word when 64-bit.  */
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr115389.c b/gcc/testsuite/gcc.target/powerpc/pr115389.c
> new file mode 100644
> index 00000000000..a091ee8a1be
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr115389.c
> @@ -0,0 +1,17 @@
> +/* PR target/115389 */
> +/* { dg-do assemble } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect -mno-vsx -mno-altivec -mabi=no-altivec -save-temps" } */

I'd expect -mabi=no-altivec is default for -mno-altivec, but specifying it explicitly
looks fine to me. :)

BR,
Kewen

> +/* { dg-require-effective-target rop_ok } */
> +
> +/* Verify we do not emit invalid offsets for our ROP insns.  */
> +
> +extern void foo (void);
> +long
> +bar (void)
> +{
> +  foo ();
> +  return 0;
> +}
> +
> +/* { dg-final { scan-assembler-times {\mhashst\M} 1 } } */
> +/* { dg-final { scan-assembler-times {\mhashchk\M} 1 } } */