Re: [PATCH 10/11] aarch64: Fix branch-protection error message tests
The 10/13/2023 11:29, Richard Earnshaw (lists) wrote:
> On 05/09/2023 16:00, Richard Sandiford via Gcc-patches wrote:
> > Szabolcs Nagy 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.
Re: [PATCH 10/11] aarch64: Fix branch-protection error message tests
On 05/09/2023 16:00, Richard Sandiford via Gcc-patches wrote:
> Szabolcs Nagy writes:
>> Update tests for the new branch-protection parser errors.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * gcc.target/aarch64/branch-protection-attr.c: Update.
>> * gcc.target/aarch64/branch-protection-option.c: Update.
>
> OK, thanks. (And I agree these are better messages. :))
>
> I think that's the last of the AArch64-specific ones. The others
> will need to be reviewed by Kyrill or Richard.
>
> Richard
>
>> ---
>> gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c | 6 +++---
>> gcc/testsuite/gcc.target/aarch64/branch-protection-option.c | 2 +-
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
>> b/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
>> index 272000c2747..dae2a758a56 100644
>> --- a/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
>> +++ b/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
>> @@ -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.
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?
>>
>> 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".
>> /* { dg-error {pragma or attribute
>> 'target\("branch-protection=none\+pac-ret"\)' is not valid} "" { target
>> *-*-* } 12 } */
>>
>> void __attribute__ ((target("branch-protection=")))
>> foo3 ()
>> {
>> }
>> -/* { dg-error {missing argument to 'target\("branch-protection="\)' pragma
>> or attribute} "" { target *-*-* } 19 } */
>> +/* { dg-error {invalid argument '' for 'target\("branch-protection="\)'} ""
>> { target *-*-* } 19 } */
>> /* { dg-error {pragma or attribute 'target\("branch-protection="\)' is not
>> valid} "" { target *-*-* } 19 } */
>> diff --git a/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
>> b/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
>> index 1b3bf4ee2b8..e2f847a31c4 100644
>> --- a/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
>> +++ b/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
>> @@ -1,4 +1,4 @@
>> /* { dg-do "compile" } */
>> /* { dg-options "-mbranch-protection=leaf -mbranch-protection=none+pac-ret"
>> } */
>>
>> -/* { dg-error "unexpected 'pac-ret' after 'none'" "" { target *-*-* } 0 }
>> */
>> +/* { dg-error "argument 'none' can only appear alone in
>> '-mbranch-protection='" "" { target *-*-* } 0 } */
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?
R
Re: [PATCH 10/11] aarch64: Fix branch-protection error message tests
Szabolcs Nagy writes:
> Update tests for the new branch-protection parser errors.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/aarch64/branch-protection-attr.c: Update.
> * gcc.target/aarch64/branch-protection-option.c: Update.
OK, thanks. (And I agree these are better messages. :))
I think that's the last of the AArch64-specific ones. The others
will need to be reviewed by Kyrill or Richard.
Richard
> ---
> gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c | 6 +++---
> gcc/testsuite/gcc.target/aarch64/branch-protection-option.c | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
> b/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
> index 272000c2747..dae2a758a56 100644
> --- a/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
> +++ b/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
> @@ -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 } */
>
> 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 } */
> /* { dg-error {pragma or attribute
> 'target\("branch-protection=none\+pac-ret"\)' is not valid} "" { target *-*-*
> } 12 } */
>
> void __attribute__ ((target("branch-protection=")))
> foo3 ()
> {
> }
> -/* { dg-error {missing argument to 'target\("branch-protection="\)' pragma
> or attribute} "" { target *-*-* } 19 } */
> +/* { dg-error {invalid argument '' for 'target\("branch-protection="\)'} ""
> { target *-*-* } 19 } */
> /* { dg-error {pragma or attribute 'target\("branch-protection="\)' is not
> valid} "" { target *-*-* } 19 } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
> b/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
> index 1b3bf4ee2b8..e2f847a31c4 100644
> --- a/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
> +++ b/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
> @@ -1,4 +1,4 @@
> /* { dg-do "compile" } */
> /* { dg-options "-mbranch-protection=leaf -mbranch-protection=none+pac-ret"
> } */
>
> -/* { dg-error "unexpected 'pac-ret' after 'none'" "" { target *-*-* } 0 } */
> +/* { dg-error "argument 'none' can only appear alone in
> '-mbranch-protection='" "" { target *-*-* } 0 } */
[PATCH 10/11] aarch64: Fix branch-protection error message tests
Update tests for the new branch-protection parser errors.
gcc/testsuite/ChangeLog:
* gcc.target/aarch64/branch-protection-attr.c: Update.
* gcc.target/aarch64/branch-protection-option.c: Update.
---
gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c | 6 +++---
gcc/testsuite/gcc.target/aarch64/branch-protection-option.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
b/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
index 272000c2747..dae2a758a56 100644
--- a/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
+++ b/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
@@ -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 } */
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 } */
/* { dg-error {pragma or attribute
'target\("branch-protection=none\+pac-ret"\)' is not valid} "" { target *-*-* }
12 } */
void __attribute__ ((target("branch-protection=")))
foo3 ()
{
}
-/* { dg-error {missing argument to 'target\("branch-protection="\)' pragma or
attribute} "" { target *-*-* } 19 } */
+/* { dg-error {invalid argument '' for 'target\("branch-protection="\)'} "" {
target *-*-* } 19 } */
/* { dg-error {pragma or attribute 'target\("branch-protection="\)' is not
valid} "" { target *-*-* } 19 } */
diff --git a/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
b/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
index 1b3bf4ee2b8..e2f847a31c4 100644
--- a/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
+++ b/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
@@ -1,4 +1,4 @@
/* { dg-do "compile" } */
/* { dg-options "-mbranch-protection=leaf -mbranch-protection=none+pac-ret" }
*/
-/* { dg-error "unexpected 'pac-ret' after 'none'" "" { target *-*-* } 0 } */
+/* { dg-error "argument 'none' can only appear alone in
'-mbranch-protection='" "" { target *-*-* } 0 } */
--
2.25.1
