| Message ID | 711459e210437af7580296f5bff2ef72b6039e7c.1692699125.git.szabolcs.nagy@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64 GCS preliminary patches | expand |
Szabolcs Nagy <szabolcs.nagy@arm.com> 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 } */
On 05/09/2023 16:00, Richard Sandiford via Gcc-patches wrote: > Szabolcs Nagy <szabolcs.nagy@arm.com> 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
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.
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 } */