Re: [PATCH 10/11] aarch64: Fix branch-protection error message tests

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 10/13/2023 11:29, Richard Earnshaw (lists) wrote:
> On 05/09/2023 16:00, Richard Sandiford via Gcc-patches wrote:
> > Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> >> @@ -4,19 +4,19 @@ void __attribute__ ((target("branch-protection=leaf")))
> >>  foo1 ()
> >>  {
> >>  }
> >> -/* { dg-error {invalid protection type 'leaf' in 'target\("branch-protection="\)' pragma or attribute} "" { target *-*-* } 5 } */
> >> +/* { dg-error {invalid argument 'leaf' for 'target\("branch-protection="\)'} "" { target *-*-* } 5 } */
> >>  /* { dg-error {pragma or attribute 'target\("branch-protection=leaf"\)' is not valid} "" { target *-*-* } 5 } */
> 
> 'leaf' is really a modifier for the other branch protection strategies; perhaps it would be better to describe it as that.

this error message is used for arbitrary strings, e.g.
branch-protection=foobar or branch-protection=bti+foo.

with further processing we can figure out that 'leaf'
is a valid modifier for pac-ret and change the error to

invalid placement of modifier 'leaf' in 'target("branch-protection=")'

otherwise fall back to

invalid argument 'foobar' for 'target("branch-protection=")'.

does that help?

(currently 'leaf' and 'b-key' are the only modifiers.)

> But this brings up another issue/question.  If the compiler has been configured with, say, '--enable-branch-protection=standard' or some other variety, is there (or do we want) a way to extend that to leaf functions without changing the underlying strategy?

there are several limitations in branch-protection handling,
i'm only fixing bugs and assumptions that don't work when arm
and aarch64 has different set of branch-protection options.

i think it can be useful to add/remove branch-protection options
incrementally in cflags instead of having one string, but it's
not obvious to me how to get there.

> >>  void __attribute__ ((target("branch-protection=none+pac-ret")))
> >>  foo2 ()
> >>  {
> >>  }
> >> -/* { dg-error "unexpected 'pac-ret' after 'none'" "" { target *-*-* } 12 } */
> >> +/* { dg-error {argument 'none' can only appear alone in 'target\("branch-protection="\)'} "" { target *-*-* } 12 } */
> 
> Or maybe better still: "branch protection strategies 'none' and 'pac-ret' are incompatible".

i can make this change, but e.g.

in case of branch-protection=standard+bti+foo it would
say "'standard' and 'bti' are incompatible" which can be
surprising given that standard includes bti, meanwhile
"'standard' can only appear alone" explains the problem.

> But this is all a matter of taste.
> 
> However, this patch should be merged with the patch that changes the error messages.  Or has that already gone in?

i can do that merge.