| Message ID | CAKdteOZhhnaJ2GB5VwN_QB+sfy3mLfsNASimibeWRyYfaga4sA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [testsuite,arm] Fix cmse-15.c expected output | expand |
On 06/04/2020 16:12, Christophe Lyon via Gcc-patches wrote: > Hi, > > While checking Martin's fix for PR ipa/94445, he made me realize that > the cmse-15.c testcase still fails at -Os because ICF means that we > generate > nonsecure2: > b nonsecure0 > > which is OK, but does not match the currently expected > nonsecure2: > ... > bl __gnu_cmse_nonsecure_call > > (see https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543190.html) > > The test has already different expectations for v8-M and v8.1-M. > > I've decided to try to use check-function-bodies to account for the > different possibilities: > - v8-M vs v8.1-M via two different prefixes > - code generation variants (-0?) via multiple regexps > > I've tested that the test now passes with --target-board=-march=armv8-m.main > and --target-board=-march=armv8.1-m.main. > > I feel this a bit too much of a burden for the purpose, maybe there's > a better way of handling all these alternatives (in particular, > there's a lot of duplication since the expected code for the secure* > functions is the same for v8-M and v8.1-M). > > OK? > > Thanks, > > Christophe Hi Christophe, This check-function-bodies functionality is pretty sweet, I assume the ( A | B ) checks for either of them? If so that looks like a good improvement. Ideally we'd also check the clearing for the v8.1-M cases, but that wasn't there before either and they would need again splitting for -mfloat-abi=soft+softfp and -mfloat-abi=hard. So yeah this LGTM but you need approval from a port/global maintainer. Cheers, Andre
On Tue, 7 Apr 2020 at 12:31, Andre Vieira (lists) <andre.simoesdiasvieira@arm.com> wrote: > > On 06/04/2020 16:12, Christophe Lyon via Gcc-patches wrote: > > Hi, > > > > While checking Martin's fix for PR ipa/94445, he made me realize that > > the cmse-15.c testcase still fails at -Os because ICF means that we > > generate > > nonsecure2: > > b nonsecure0 > > > > which is OK, but does not match the currently expected > > nonsecure2: > > ... > > bl __gnu_cmse_nonsecure_call > > > > (see https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543190.html) > > > > The test has already different expectations for v8-M and v8.1-M. > > > > I've decided to try to use check-function-bodies to account for the > > different possibilities: > > - v8-M vs v8.1-M via two different prefixes > > - code generation variants (-0?) via multiple regexps > > > > I've tested that the test now passes with --target-board=-march=armv8-m.main > > and --target-board=-march=armv8.1-m.main. > > > > I feel this a bit too much of a burden for the purpose, maybe there's > > a better way of handling all these alternatives (in particular, > > there's a lot of duplication since the expected code for the secure* > > functions is the same for v8-M and v8.1-M). > > > > OK? > > > > Thanks, > > > > Christophe > Hi Christophe, > > This check-function-bodies functionality is pretty sweet, I assume the ( > A | B ) checks for either of them? Yes. > If so that looks like a good improvement. Ideally we'd also check the > clearing for the v8.1-M cases, but that wasn't there before either and > they would need again splitting for -mfloat-abi=soft+softfp and > -mfloat-abi=hard. > Not sure what you mean? The only nonsecure test with the (A|B) construct is: +*Clear nonsecure2: +*Clear ... +*Clear ( +*Clear blxns r[0-3] +*Clear | +*Clear b nonsecure0 +*Clear ) So it does check the clearing (blxns), and 'b nonsecure0' is as valid as the result of the test for nonsecure0. > > So yeah this LGTM but you need approval from a port/global maintainer. > Thanks > Cheers, > Andre
Christophe Lyon via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > Hi, > > While checking Martin's fix for PR ipa/94445, he made me realize that > the cmse-15.c testcase still fails at -Os because ICF means that we > generate > nonsecure2: > b nonsecure0 > > which is OK, but does not match the currently expected > nonsecure2: > ... > bl __gnu_cmse_nonsecure_call > > (see https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543190.html) > > The test has already different expectations for v8-M and v8.1-M. > > I've decided to try to use check-function-bodies to account for the > different possibilities: > - v8-M vs v8.1-M via two different prefixes > - code generation variants (-0?) via multiple regexps > > I've tested that the test now passes with --target-board=-march=armv8-m.main > and --target-board=-march=armv8.1-m.main. > > I feel this a bit too much of a burden for the purpose, maybe there's > a better way of handling all these alternatives (in particular, > there's a lot of duplication since the expected code for the secure* > functions is the same for v8-M and v8.1-M). FWIW, an alternative is to give multiple versions with the same prefix and use { target ... } to select between them. E.g.: /* ** foo: { target a } ** ... */ /* ** foo: { target { ! a } } ** ... */ Thanks, Richard
On Wed, 8 Apr 2020 at 11:48, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Christophe Lyon via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > > Hi, > > > > While checking Martin's fix for PR ipa/94445, he made me realize that > > the cmse-15.c testcase still fails at -Os because ICF means that we > > generate > > nonsecure2: > > b nonsecure0 > > > > which is OK, but does not match the currently expected > > nonsecure2: > > ... > > bl __gnu_cmse_nonsecure_call > > > > (see https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543190.html) > > > > The test has already different expectations for v8-M and v8.1-M. > > > > I've decided to try to use check-function-bodies to account for the > > different possibilities: > > - v8-M vs v8.1-M via two different prefixes > > - code generation variants (-0?) via multiple regexps > > > > I've tested that the test now passes with --target-board=-march=armv8-m.main > > and --target-board=-march=armv8.1-m.main. > > > > I feel this a bit too much of a burden for the purpose, maybe there's > > a better way of handling all these alternatives (in particular, > > there's a lot of duplication since the expected code for the secure* > > functions is the same for v8-M and v8.1-M). > > FWIW, an alternative is to give multiple versions with the same prefix > and use { target ... } to select between them. E.g.: > > /* > ** foo: { target a } > ** ... > */ > /* > ** foo: { target { ! a } } > ** ... > */ > Ha indeed, that makes it simpler. Thanks for the example, I hadn't fully understand how to use that scheme: I tried to add a third alternative (different prefix) with no selector for cases where no distinction was needed, but I realized that all alternatives need their full matching description. However, {target { ! a } } does not work as is, so the attached patch uses a non-greedy regexp to avoid trying "! a }" instead of "target { ! a }" If OK, maybe I should commit that as two separate patches? Thanks Christophe > Thanks, > Richard diff --git a/gcc/testsuite/gcc.target/arm/cmse/cmse-15.c b/gcc/testsuite/gcc.target/arm/cmse/cmse-15.c index 0e37b50..b0fefe5 100644 --- a/gcc/testsuite/gcc.target/arm/cmse/cmse-15.c +++ b/gcc/testsuite/gcc.target/arm/cmse/cmse-15.c @@ -1,5 +1,8 @@ /* { dg-do compile } */ /* { dg-options "-mcmse" } */ +/* ARMv8-M expectation with target { ! arm_cmse_clear_ok }. */ +/* ARMv8.1-M expectation with target arm_cmse_clear_ok. */ +/* { dg-final { check-function-bodies "**" "" "" } } */ int __attribute__ ((cmse_nonsecure_call)) (*ns_foo) (void); int (*s_bar) (void); @@ -11,67 +14,204 @@ typedef int s_bar_t (void); typedef int __attribute__ ((cmse_nonsecure_call)) (* ns_foo_ptr) (void); typedef int (*s_bar_ptr) (void); +/* +** nonsecure0: { target arm_cmse_clear_ok } +** ... +** blxns r[0-3] +** ... +*/ +/* +** nonsecure0: { target { ! arm_cmse_clear_ok } } +** ... +** bl __gnu_cmse_nonsecure_call +** ... +*/ int nonsecure0 (ns_foo_t * ns_foo_p) { return ns_foo_p (); } +/* +** nonsecure1: { target arm_cmse_clear_ok } +** ... +** blxns r[0-3] +** ... +*/ +/* +** nonsecure1: { target { ! arm_cmse_clear_ok } } +** ... +** bl __gnu_cmse_nonsecure_call +** ... +*/ int nonsecure1 (ns_foo_t ** ns_foo_p) { return (*ns_foo_p) (); } +/* +** nonsecure2: { target arm_cmse_clear_ok } +** ... +** ( +** blxns r[0-3] +** | +** b nonsecure0 +** ) +** ... +*/ +/* +** nonsecure2: { target { ! arm_cmse_clear_ok } } +** ... +** ( +** bl __gnu_cmse_nonsecure_call +** | +** b nonsecure0 +** ) +** ... +*/ int nonsecure2 (ns_foo_ptr ns_foo_p) { return ns_foo_p (); } + +/* +** nonsecure3: { target arm_cmse_clear_ok } +** ... +** blxns r[0-3] +** ... +*/ +/* +** nonsecure3: { target { ! arm_cmse_clear_ok } } +** ... +** bl __gnu_cmse_nonsecure_call +** ... +*/ int nonsecure3 (ns_foo_ptr * ns_foo_p) { return (*ns_foo_p) (); } +/* +** secure0: +** ... +** ( +** bx r[0-3] +** | +** blx r[0-3] +** ) +** ... +*/ int secure0 (s_bar_t * s_bar_p) { return s_bar_p (); } +/* +** secure1: +** ... +** ( +** bx r[0-3] +** | +** blx r[0-3] +** ) +** ... +*/ int secure1 (s_bar_t ** s_bar_p) { return (*s_bar_p) (); } +/* +** secure2: +** ... +** ( +** bx r[0-3] +** | +** blx r[0-3] +** | +** b secure0 +** ) +** ... +*/ int secure2 (s_bar_ptr s_bar_p) { return s_bar_p (); } +/* +** secure3: +** ... +** ( +** bx r[0-3] +** | +** blx r[0-3] +** ) +** ... +*/ int secure3 (s_bar_ptr * s_bar_p) { return (*s_bar_p) (); } +/* +** nonsecure4: { target arm_cmse_clear_ok } +** ... +** blxns r[0-3] +** ... +*/ +/* +** nonsecure4: { target { ! arm_cmse_clear_ok } } +** ... +** bl __gnu_cmse_nonsecure_call +** ... +*/ int nonsecure4 (void) { return ns_foo (); } +/* +** nonsecure5: { target arm_cmse_clear_ok } +** ... +** blxns r[0-3] +** ... +*/ +/* +** nonsecure5: { target { ! arm_cmse_clear_ok } } +** ... +** bl __gnu_cmse_nonsecure_call +** ... +*/ int nonsecure5 (void) { return (*ns_foo2) (); } +/* +** secure4: +** ... +** ( +** bx r[0-3] +** | +** blx r[0-3] +** ) +** ... +*/ int secure4 (void) { return s_bar (); } +/* +** secure5: +** ... +** ( +** bx r[0-3] +** | +** blx r[0-3] +** ) +** ... +*/ int secure5 (void) { return (*s_bar2) (); } - -/* ARMv8-M expectation. */ -/* { dg-final { scan-assembler-times "bl\\s+__gnu_cmse_nonsecure_call" 6 { target { ! arm_cmse_clear_ok } } } } */ - -/* ARMv8.1-M expectation. */ -/* { dg-final { scan-assembler-times "blxns" 6 { target arm_cmse_clear_ok } } } */ diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp index f7d2773..0098c3d 100644 --- a/gcc/testsuite/lib/scanasm.exp +++ b/gcc/testsuite/lib/scanasm.exp @@ -679,7 +679,7 @@ proc check-function-bodies { args } { if { [string equal -length $prefix_len $line $prefix] } { set line [string trim [string range $line $prefix_len end]] if { !$in_function } { - if { [regexp "^(.*\\S)\\s+{(.*)}\$" $line dummy \ + if { [regexp "^(.*?\\S)\\s+{(.*)}\$" $line dummy \ line selector] } { set selector [dg-process-target $selector] } else { gcc/testsuite/ChangeLog: 2020-04-08 Christophe Lyon <christophe.lyon@linaro.org> * gcc.target/arm/cmse/cmse-15.c: Use check-function-bodies. * lib/scanasm.exp (check-function-bodies): Use non-greedy regexp when extracting the target selector.
Christophe Lyon <christophe.lyon@linaro.org> writes: > On Wed, 8 Apr 2020 at 11:48, Richard Sandiford > <richard.sandiford@arm.com> wrote: >> >> Christophe Lyon via Gcc-patches <gcc-patches@gcc.gnu.org> writes: >> > Hi, >> > >> > While checking Martin's fix for PR ipa/94445, he made me realize that >> > the cmse-15.c testcase still fails at -Os because ICF means that we >> > generate >> > nonsecure2: >> > b nonsecure0 >> > >> > which is OK, but does not match the currently expected >> > nonsecure2: >> > ... >> > bl __gnu_cmse_nonsecure_call >> > >> > (see https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543190.html) >> > >> > The test has already different expectations for v8-M and v8.1-M. >> > >> > I've decided to try to use check-function-bodies to account for the >> > different possibilities: >> > - v8-M vs v8.1-M via two different prefixes >> > - code generation variants (-0?) via multiple regexps >> > >> > I've tested that the test now passes with --target-board=-march=armv8-m.main >> > and --target-board=-march=armv8.1-m.main. >> > >> > I feel this a bit too much of a burden for the purpose, maybe there's >> > a better way of handling all these alternatives (in particular, >> > there's a lot of duplication since the expected code for the secure* >> > functions is the same for v8-M and v8.1-M). >> >> FWIW, an alternative is to give multiple versions with the same prefix >> and use { target ... } to select between them. E.g.: >> >> /* >> ** foo: { target a } >> ** ... >> */ >> /* >> ** foo: { target { ! a } } >> ** ... >> */ >> > > Ha indeed, that makes it simpler. Thanks for the example, I hadn't > fully understand how to use that scheme: I tried to add a third > alternative (different prefix) with no selector for cases where no > distinction was needed, but I realized that all alternatives need > their full matching description. > > However, {target { ! a } } does not work as is, so the attached patch > uses a non-greedy regexp to avoid trying "! a }" instead of "target { > ! a }" Oops. This part is definitely OK, thanks. > If OK, maybe I should commit that as two separate patches? I don't really feel qualified to review the substance of the arm part of the patch, but given Andre's LGTM for that in the earlier version, a rubber-stamp OK for that too. I agree separate commits might be better. Thanks, Richard
diff --git a/gcc/testsuite/gcc.target/arm/cmse/cmse-15.c b/gcc/testsuite/gcc.target/arm/cmse/cmse-15.c index 0e37b50..603c456 100644 --- a/gcc/testsuite/gcc.target/arm/cmse/cmse-15.c +++ b/gcc/testsuite/gcc.target/arm/cmse/cmse-15.c @@ -1,5 +1,9 @@ /* { dg-do compile } */ /* { dg-options "-mcmse" } */ +/* ARMv8-M expectation. */ +/* { dg-final { check-function-bodies "*Noclear" "" "" { target { ! arm_cmse_clear_ok } } } } */ +/* ARMv8.1-M expectation. */ +/* { dg-final { check-function-bodies "*Clear" "" "" { target arm_cmse_clear_ok } } } */ int __attribute__ ((cmse_nonsecure_call)) (*ns_foo) (void); int (*s_bar) (void); @@ -11,67 +15,242 @@ typedef int s_bar_t (void); typedef int __attribute__ ((cmse_nonsecure_call)) (* ns_foo_ptr) (void); typedef int (*s_bar_ptr) (void); +/* +*Clear nonsecure0: +*Clear ... +*Clear blxns r[0-3] +*Clear ... +*Noclear nonsecure0: +*Noclear ... +*Noclear bl __gnu_cmse_nonsecure_call +*Noclear ... +*/ int nonsecure0 (ns_foo_t * ns_foo_p) { return ns_foo_p (); } +/* +*Clear nonsecure1: +*Clear ... +*Clear blxns r[0-3] +*Clear ... +*Noclear nonsecure1: +*Noclear ... +*Noclear bl __gnu_cmse_nonsecure_call +*Noclear ... +*/ int nonsecure1 (ns_foo_t ** ns_foo_p) { return (*ns_foo_p) (); } +/* +*Clear nonsecure2: +*Clear ... +*Clear ( +*Clear blxns r[0-3] +*Clear | +*Clear b nonsecure0 +*Clear ) +*Clear ... +*Noclear nonsecure2: +*Noclear ... +*Noclear ( +*Noclear bl __gnu_cmse_nonsecure_call +*Noclear | +*Noclear b nonsecure0 +*Noclear ) +*Noclear ... +*/ int nonsecure2 (ns_foo_ptr ns_foo_p) { return ns_foo_p (); } + +/* +*Clear nonsecure3: +*Clear ... +*Clear blxns r[0-3] +*Clear ... +*Noclear nonsecure3: +*Noclear ... +*Noclear bl __gnu_cmse_nonsecure_call +*Noclear ... +*/ int nonsecure3 (ns_foo_ptr * ns_foo_p) { return (*ns_foo_p) (); } +/* +*Clear secure0: +*Clear ... +*Clear ( +*Clear bx r[0-3] +*Clear | +*Clear blx r[0-3] +*Clear ) +*Clear ... +*Noclear secure0: +*Noclear ... +*Noclear ( +*Noclear bx r[0-3] +*Noclear | +*Noclear blx r[0-3] +*Noclear ) +*Noclear ... +*/ int secure0 (s_bar_t * s_bar_p) { return s_bar_p (); } +/* +*Clear secure1: +*Clear ... +*Clear ( +*Clear bx r[0-3] +*Clear | +*Clear blx r[0-3] +*Clear ) +*Clear ... +*Noclear secure1: +*Noclear ... +*Noclear ( +*Noclear bx r[0-3] +*Noclear | +*Noclear blx r[0-3] +*Noclear ) +*Noclear ... +*/ int secure1 (s_bar_t ** s_bar_p) { return (*s_bar_p) (); } +/* +*Clear secure2: +*Clear ... +*Clear ( +*Clear bx r[0-3] +*Clear | +*Clear blx r[0-3] +*Clear | +*Clear b secure0 +*Clear ) +*Clear ... +*Noclear secure2: +*Noclear ... +*Noclear ( +*Noclear bx r[0-3] +*Noclear | +*Noclear blx r[0-3] +*Noclear | +*Noclear b secure0 +*Noclear ) +*Noclear ... +*/ int secure2 (s_bar_ptr s_bar_p) { return s_bar_p (); } +/* +*Clear secure3: +*Clear ... +*Clear ( +*Clear bx r[0-3] +*Clear | +*Clear blx r[0-3] +*Clear ) +*Clear ... +*Noclear secure3: +*Noclear ... +*Noclear ( +*Noclear bx r[0-3] +*Noclear | +*Noclear blx r[0-3] +*Noclear ) +*Noclear ... +*/ int secure3 (s_bar_ptr * s_bar_p) { return (*s_bar_p) (); } +/* +*Clear nonsecure4: +*Clear ... +*Clear blxns r[0-3] +*Clear ... +*Noclear nonsecure4: +*Noclear ... +*Noclear bl __gnu_cmse_nonsecure_call +*Noclear ... +*/ int nonsecure4 (void) { return ns_foo (); } +/* +*Clear nonsecure5: +*Clear ... +*Clear blxns r[0-3] +*Clear ... +*Noclear nonsecure5: +*Noclear ... +*Noclear bl __gnu_cmse_nonsecure_call +*Noclear ... +*/ int nonsecure5 (void) { return (*ns_foo2) (); } +/* +*Clear secure4: +*Clear ... +*Clear ( +*Clear bx r[0-3] +*Clear | +*Clear blx r[0-3] +*Clear ) +*Clear ... +*Noclear secure4: +*Noclear ... +*Noclear ( +*Noclear bx r[0-3] +*Noclear | +*Noclear blx r[0-3] +*Noclear ) +*Noclear ... +*/ int secure4 (void) { return s_bar (); } +/* +*Clear secure5: +*Clear ... +*Clear ( +*Clear bx r[0-3] +*Clear | +*Clear blx r[0-3] +*Clear ) +*Clear ... +*Noclear secure5: +*Noclear ... +*Noclear ( +*Noclear bx r[0-3] +*Noclear | +*Noclear blx r[0-3] +*Noclear ) +*Noclear ... +*/ int secure5 (void) { return (*s_bar2) (); } - -/* ARMv8-M expectation. */ -/* { dg-final { scan-assembler-times "bl\\s+__gnu_cmse_nonsecure_call" 6 { target { ! arm_cmse_clear_ok } } } } */ - -/* ARMv8.1-M expectation. */ -/* { dg-final { scan-assembler-times "blxns" 6 { target arm_cmse_clear_ok } } } */
Hi, While checking Martin's fix for PR ipa/94445, he made me realize that the cmse-15.c testcase still fails at -Os because ICF means that we generate nonsecure2: b nonsecure0 which is OK, but does not match the currently expected nonsecure2: ... bl __gnu_cmse_nonsecure_call (see https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543190.html) The test has already different expectations for v8-M and v8.1-M. I've decided to try to use check-function-bodies to account for the different possibilities: - v8-M vs v8.1-M via two different prefixes - code generation variants (-0?) via multiple regexps I've tested that the test now passes with --target-board=-march=armv8-m.main and --target-board=-march=armv8.1-m.main. I feel this a bit too much of a burden for the purpose, maybe there's a better way of handling all these alternatives (in particular, there's a lot of duplication since the expected code for the secure* functions is the same for v8-M and v8.1-M). OK? Thanks, Christophe 2020-04-06 Christophe Lyon <christophe.lyon@linaro.org> gcc/testsuite/ * gcc.target/arm/cmse/cmse-15.c: Use check-function-bodies.