RE: [PATCH 12/15 V2] arm: implement bti injection

[Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>]

> -----Original Message-----
> From: Andrea Corallo <andrea.corallo@arm.com>
> Sent: Thursday, September 29, 2022 4:46 PM
> To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> Cc: Andrea Corallo via Gcc-patches <gcc-patches@gcc.gnu.org>; Richard
> Earnshaw <Richard.Earnshaw@arm.com>; nd <nd@arm.com>
> Subject: [PATCH 12/15 V2] arm: implement bti injection
> 
> Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> writes:
> 
> > Hi Andrea,
> 
> [...]
> 
> > diff --git a/gcc/config/arm/aarch-bti-insert.cc b/gcc/config/arm/aarch-bti-
> insert.cc
> > index 2d1d2e334a9..8f045c247bf 100644
> > --- a/gcc/config/arm/aarch-bti-insert.cc
> > +++ b/gcc/config/arm/aarch-bti-insert.cc
> > @@ -41,6 +41,7 @@
> >  #include "cfgrtl.h"
> >  #include "tree-pass.h"
> >  #include "cgraph.h"
> > +#include "diagnostic-core.h"
> >
> > This change doesn't seem to match what's in the ChangeLog and doesn't
> make sense to me.
> 
> Change removed thanks.
> 
> > @@ -32985,6 +32979,58 @@ arm_current_function_pac_enabled_p (void)
> >  	&& !crtl->is_leaf);
> >  }
> >
> > +/* Return TRUE if Branch Target Identification Mechanism is enabled.  */
> > +bool
> > +aarch_bti_enabled (void)
> > +{
> > +  return aarch_enable_bti == 1;
> > +}
> > +
> > +/* Check if INSN is a BTI J insn.  */
> > +bool
> > +aarch_bti_j_insn_p (rtx_insn *insn)
> > +{
> > +  if (!insn || !INSN_P (insn))
> > +    return false;
> > +
> > +  rtx pat = PATTERN (insn);
> > +  return GET_CODE (pat) == UNSPEC_VOLATILE && XINT (pat, 1) ==
> UNSPEC_BTI_NOP;
> > +}
> > +
> > +/* Check if X (or any sub-rtx of X) is a PACIASP/PACIBSP instruction.  */
> >
> > The arm instructions are not PACIASP/PACIBSP.
> > This comment should be rewritten.
> 
> This hunk belongs to aarch64.cc so it's aarch64 specific.
> 
> > +bool
> > +aarch_pac_insn_p (rtx x)
> > +{
> >
> > ..........
> >
> > +rtx
> > +aarch_gen_bti_c (void)
> > +{
> > +  return gen_bti_nop ();
> > +}
> > +
> > +rtx
> > +aarch_gen_bti_j (void)
> > +{
> > +  return gen_bti_nop ();
> > +}
> > +
> >
> > A reader may be confused for why we have a bti_c and bti_j function that
> have identical functionality.
> > Please add function comments explaining the situation.
> 
> Done
> 
> > diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> > index 92269a7819a..90c8c1d66f5 100644
> > --- a/gcc/config/arm/arm.md
> > +++ b/gcc/config/arm/arm.md
> > @@ -12913,6 +12913,13 @@
> >    "aut\t%|ip, %|lr, %|sp"
> >    [(set_attr "length" "4")])
> >
> > +(define_insn "bti_nop"
> > +  [(unspec_volatile [(const_int 0)] UNSPEC_BTI_NOP)]
> > +  "arm_arch7 && arm_arch_cmse"
> >
> > That seems like a copy-paste mistake. CMSE has nothing to do with this
> functionality?
> 
> This is because we don't have arm_arch8m_main, but this is equivalent to
> arm_arch7 && arm_arch_cmse.  IIUC it wasn't added becasue armv8-m is
> basically just armv7-m + cmse.
> 
> Any other preferred way to express this?

I think I'd prefer if we added an explicit arm_arch8m_main. It would help readability

> 
> > +  "bti"
> > +  [(set_attr "length" "4")
> >
> > The length of instructions in the arm backend is 4 by default, this set_attr
> can be omitted
> >
> > +   (set_attr "type" "mov_reg")])
> > +
> > Probably better to use the "nop" attribute here?
> 
> Done

Thanks, and as in patch 10/12 I think we'll want to set the "conds" attribute here to "unconditional".
Looks good to me otherwise!
Kyrill

> 
> Thanks for reviewing, please find attached the updated version.
> 
>   Andrea