[1/4] rs6000: Add -mrop-protect and -mprivileged flags

2021-03-25  Bill Schmidt  <wschmidt@linux.ibm.com>

gcc/
	* config/rs6000/rs6000.c (rs6000_option_override_internal):
	Disable shrink wrap when inserting ROP-protect instructions.
	* config/rs6000/rs6000.opt (mrop-protect): New option.
	(mprivileged): Likewise.
	* doc/invoke.texi: Document mrop-protect and mprivileged.
---
 gcc/config/rs6000/rs6000.c   |  7 +++++++
 gcc/config/rs6000/rs6000.opt |  6 ++++++
 gcc/doc/invoke.texi          | 19 +++++++++++++++++--
 3 files changed, 30 insertions(+), 2 deletions(-)

On Sun, 2021-04-25 at 20:50 -0500, Bill Schmidt via Gcc-patches wrote:
> 2021-03-25  Bill Schmidt  <wschmidt@linux.ibm.com>
> 
> gcc/
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal):
> 	Disable shrink wrap when inserting ROP-protect instructions.
> 	* config/rs6000/rs6000.opt (mrop-protect): New option.
> 	(mprivileged): Likewise.
> 	* doc/invoke.texi: Document mrop-protect and mprivileged.

Hi, 


> ---
>  gcc/config/rs6000/rs6000.c   |  7 +++++++
>  gcc/config/rs6000/rs6000.opt |  6 ++++++
>  gcc/doc/invoke.texi          | 19 +++++++++++++++++--
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index 844fee88cf3..d13ed6e7ff4 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -4036,6 +4036,13 @@ rs6000_option_override_internal (bool global_init_p)
>        && ((rs6000_isa_flags_explicit & OPTION_MASK_QUAD_MEMORY_ATOMIC) == 0))
>      rs6000_isa_flags |= OPTION_MASK_QUAD_MEMORY_ATOMIC;
> 
> +  /* If we are inserting ROP-protect instructions, disable shrink wrap.  */
> +  if (rs6000_rop_protect)
> +    {
> +      flag_shrink_wrap = 0;
> +      flag_shrink_wrap_separate = 0;
> +    }

Does this (shrink-wrap is disabled if/when ROP-protect is enabled) need
additional commentary somewhere?  


> +
>    /* If we can shrink-wrap the TOC register save separately, then use
>       -msave-toc-indirect unless explicitly disabled.  */
>    if ((rs6000_isa_flags_explicit & OPTION_MASK_SAVE_TOC_INDIRECT) == 0
> diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
> index 0dbdf753673..d116fd12f7e 100644
> --- a/gcc/config/rs6000/rs6000.opt
> +++ b/gcc/config/rs6000/rs6000.opt
> @@ -619,3 +619,9 @@ Generate (do not generate) MMA instructions.
> 
>  mrelative-jumptables
>  Target Undocumented Var(rs6000_relative_jumptables) Init(1) Save
> +
> +mrop-protect
> +Target Var(rs6000_rop_protect) Init(0)
> +
> +mprivileged
> +Target Var(rs6000_privileged) Init(0)

Most but not all of the entries in rs6000.opt have an additional
description line.  I'd wonder about updating this to be stl

> +mrop-protect
> +Target Var(rs6000_rop_protect) Init(0)

Enable ROP protection 

> +
> +mprivileged
> +Target Var(rs6000_privileged) Init(0)

Enable privileged instructions for ROP protection.


OK with me either way.  :-)




> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index e98b0962b9f..36bd0bf9b3b 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -1238,7 +1238,8 @@ See RS/6000 and PowerPC Options.
>  -mgnu-attribute  -mno-gnu-attribute @gol
>  -mstack-protector-guard=@var{guard} -mstack-protector-guard-reg=@var{reg} @gol
>  -mstack-protector-guard-offset=@var{offset} -mprefixed -mno-prefixed @gol
> --mpcrel -mno-pcrel -mmma -mno-mmma}
> +-mpcrel -mno-pcrel -mmma -mno-mmma -mrop-protect -mno-rop-protect @gol
> +-mprivileged -mno-privileged}
> 
>  @emph{RX Options}
>  @gccoptlist{-m64bit-doubles  -m32bit-doubles  -fpu  -nofpu@gol
> @@ -27019,7 +27020,8 @@ following options:
>  -mmulhw  -mdlmzb  -mmfpgpr  -mvsx @gol
>  -mcrypto  -mhtm  -mpower8-fusion  -mpower8-vector @gol
>  -mquad-memory  -mquad-memory-atomic  -mfloat128 @gol
> --mfloat128-hardware -mprefixed -mpcrel -mmma}
> +-mfloat128-hardware -mprefixed -mpcrel -mmma @gol
> +-mrop-protect -mprivileged}
> 
>  The particular options set for any particular CPU varies between
>  compiler versions, depending on what setting seems to produce optimal
> @@ -28024,6 +28026,19 @@ store instructions when the option @option{-mcpu=future} is used.
>  Generate (do not generate) the MMA instructions when the option
>  @option{-mcpu=future} is used.
> 
> +@item -mrop-protect
> +@itemx -mno-rop-protect
> +@opindex mrop-protect
> +@opindex mno-rop-protect
> +Generate (do not generate) ROP protection instructions when the option
> +@option{-mcpu=power10} is used.

Is the option on by default?  if so, may want another testcase to
verify ROP instructions are generated with just -mcpu=power10.
if not,
perhaps the "-mcpu=power10" reference here instead be "-mrop-protect".


> +
> +@item -mprivileged
> +@itemx -mno-privileged
> +@opindex mprivileged
> +@opindex mno-privileged
> +Generate (do not generate) instructions for privileged state.
> +
>  @item -mblock-ops-unaligned-vsx
>  @itemx -mno-block-ops-unaligned-vsx
>  @opindex block-ops-unaligned-vsx


lgtm
thanks,
-Will

On Sun, Apr 25, 2021 at 08:50:15PM -0500, Bill Schmidt wrote:
> +  /* If we are inserting ROP-protect instructions, disable shrink wrap.  */
> +  if (rs6000_rop_protect)
> +    {
> +      flag_shrink_wrap = 0;
> +      flag_shrink_wrap_separate = 0;
> +    }

Separate shrink-wrapping requires flag_shrink_wrap, so remove that
second assignment please?

===
void
try_shrink_wrapping (edge *entry_edge, rtx_insn *prologue_seq)
{
  /* If we cannot shrink-wrap, are told not to shrink-wrap, or it makes
     no sense to shrink-wrap: then do not shrink-wrap!  */

  if (!SHRINK_WRAPPING_ENABLED)
    return;
===

===
void
try_shrink_wrapping_separate (basic_block first_bb)
{
  if (!(SHRINK_WRAPPING_ENABLED
        && flag_shrink_wrap_separate
        && optimize_function_for_speed_p (cfun)
        && targetm.shrink_wrap.get_separate_components))
    return;
===

and for completeness

===
#define SHRINK_WRAPPING_ENABLED \
  (flag_shrink_wrap && targetm.have_simple_return ())
===

> +-mrop-protect -mprivileged}

-mprivileged should not be CPU-specific, the concept applies on all CPUs
(but of course is only used on p10 so far).  So please document it in
the right location :-)

Okay for trunk and 11 with those changes.  Thanks!


Segher

On Mon, Apr 26, 2021 at 11:02:53AM -0500, will schmidt wrote:
> On Sun, 2021-04-25 at 20:50 -0500, Bill Schmidt via Gcc-patches wrote:
> > +  /* If we are inserting ROP-protect instructions, disable shrink wrap.  */
> > +  if (rs6000_rop_protect)
> > +    {
> > +      flag_shrink_wrap = 0;
> > +      flag_shrink_wrap_separate = 0;
> > +    }
> 
> Does this (shrink-wrap is disabled if/when ROP-protect is enabled) need
> additional commentary somewhere?  

This should be documented in the manual somewhere, probably where
-mrop-protect is described.

It would be nice if we could do both ROP protection and shrink-wrapping
at once, but we aren't there yet (if we ever can do that).

> > +mrop-protect
> > +Target Var(rs6000_rop_protect) Init(0)
> > +
> > +mprivileged
> > +Target Var(rs6000_privileged) Init(0)
> 
> Most but not all of the entries in rs6000.opt have an additional
> description line.  I'd wonder about updating this to be stl

Yeah, that is used for the help text etc.

> > +mrop-protect
> > +Target Var(rs6000_rop_protect) Init(0)
> 
> Enable ROP protection 

This is a bit brief.  Also, these lines are sentences, need to end in a
full stop.

> > +mprivileged
> > +Target Var(rs6000_privileged) Init(0)
> 
> Enable privileged instructions for ROP protection.

-mprivileged is not only for ROP protection: it simply indicates we are
compiling code that will run in a privileged mode (i.e., not in problem
state).

> > +@item -mrop-protect
> > +@itemx -mno-rop-protect
> > +@opindex mrop-protect
> > +@opindex mno-rop-protect
> > +Generate (do not generate) ROP protection instructions when the option
> > +@option{-mcpu=power10} is used.
> 
> Is the option on by default?

Nope.

> if so, may want another testcase to
> verify ROP instructions are generated with just -mcpu=power10.
> if not,
> perhaps the "-mcpu=power10" reference here instead be "-mrop-protect".

This is the help text for -mrop-protect :-)

Probably the help text could be more future-proof though?


Segher