| Message ID | orwmoyc6iz.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | [testsuite,arm] require arm_v8_1m_main for pacbti tests | expand |
On 16/04/2024 04:48, Alexandre Oliva wrote: > > arm pac and bti tests that use -march=armv8.1-m.main get an implicit > -mthumb, that is incompatible with vxworks kernel mode. Declaring the > requirement for a 8.1-m.main-compatible toolchain is enough to avoid > those fails, because the toolchain feature test fails in kernel mode. > > Regstrapped on x86_64-linux-gnu. Also tested with gcc-13 on arm-, > aarch64-, x86- and x86_64-vxworks7r2. Ok to install? > > > for gcc/testsuite/ChangeLog > > * g++.target/arm/pac-1.C: Require arm_arch_v8_1m_main. > * gcc.target/arm/acle/pacbti-m-predef-11.c: Likewise. > * gcc.target/arm/acle/pacbti-m-predef-12.c: Likewise. > * gcc.target/arm/acle/pacbti-m-predef-7.c: Likewise. > * gcc.target/arm/bti-1.c: Likewise. > * gcc.target/arm/bti-2.c: Likewise. > --- > gcc/testsuite/g++.target/arm/pac-1.C | 1 + > .../gcc.target/arm/acle/pacbti-m-predef-11.c | 1 + > .../gcc.target/arm/acle/pacbti-m-predef-12.c | 1 + > .../gcc.target/arm/acle/pacbti-m-predef-7.c | 1 + > gcc/testsuite/gcc.target/arm/bti-1.c | 1 + > gcc/testsuite/gcc.target/arm/bti-2.c | 1 + > 6 files changed, 6 insertions(+) > > diff --git a/gcc/testsuite/g++.target/arm/pac-1.C b/gcc/testsuite/g++.target/arm/pac-1.C > index f671a27b048c6..f48ad6cc5cb65 100644 > --- a/gcc/testsuite/g++.target/arm/pac-1.C > +++ b/gcc/testsuite/g++.target/arm/pac-1.C > @@ -2,6 +2,7 @@ > /* { dg-do compile } */ > /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */ > /* { dg-options "-march=armv8.1-m.main+mve+pacbti -mbranch-protection=pac-ret -mthumb -mfloat-abi=hard -g -O0" } */ > +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */ The require-effective-target flags test whether a specific set of flags will make the compilation work, so they need to be used in conjunction with the corresponding dg-add-options flags that then apply those options. It isn't safe to just add a different architecture flag instead. So if you're going to use this effective target, you should use it along with "dg-add-options arm_arch_v8_1m_main" (ie the effective-target name minus the trailing '_ok'), and then replace dg-options with dg-additional-options adding the remaining flags. You can then remove the dg-skip-if as well because that's what the require-effective-target flag is doing. So something like dg-do compile dg-require-effective-target arm_arch_v8_1m_main_ok dg-add-options arm_arch_v8_1m_main dg-additional-options "-mbranch-protection=pac-ret -g -O0" But this test is also adding pacbti to the architecture flags, so it would probably be better to use v8_1m_main_pacbti_ok as the effective target. It's not identical to the options above, but it's probably sufficient for this test. Each test below will need checking for the exact flags that are needed for the test in question. > > __attribute__((noinline)) void > fn1 (int a, int b, int c) > diff --git a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-11.c b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-11.c > index 6a5ae92c567f3..dba4f491cfea7 100644 > --- a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-11.c > +++ b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-11.c > @@ -1,6 +1,7 @@ > /* { dg-do compile } */ > /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" "-mfloat-abi=*" } } */ > /* { dg-options "-march=armv8.1-m.main+fp+pacbti" } */ > +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */ > > #if (__ARM_FEATURE_BTI != 1) > #error "Feature test macro __ARM_FEATURE_BTI_DEFAULT should be defined to 1." > diff --git a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-12.c b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-12.c > index db40b17c3b030..308a41eb4ba4c 100644 > --- a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-12.c > +++ b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-12.c > @@ -1,6 +1,7 @@ > /* { dg-do compile } */ > /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */ > /* { dg-options "-march=armv8-m.main+fp -mfloat-abi=softfp" } */ > +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */ > > #if defined (__ARM_FEATURE_BTI) > #error "Feature test macro __ARM_FEATURE_BTI should not be defined." > diff --git a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-7.c b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-7.c > index 1b25907635e24..10836a84bde56 100644 > --- a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-7.c > +++ b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-7.c > @@ -1,6 +1,7 @@ > /* { dg-do compile } */ > /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */ > /* { dg-additional-options "-march=armv8.1-m.main+pacbti+fp --save-temps -mfloat-abi=hard" } */ > +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */ > > #if defined (__ARM_FEATURE_BTI_DEFAULT) > #error "Feature test macro __ARM_FEATURE_BTI_DEFAULT should be undefined." > diff --git a/gcc/testsuite/gcc.target/arm/bti-1.c b/gcc/testsuite/gcc.target/arm/bti-1.c > index 79dd8010d2dab..34655387b55f5 100644 > --- a/gcc/testsuite/gcc.target/arm/bti-1.c > +++ b/gcc/testsuite/gcc.target/arm/bti-1.c > @@ -2,6 +2,7 @@ > /* { dg-do compile } */ > /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */ > /* { dg-options "-march=armv8.1-m.main -mthumb -mfloat-abi=softfp -mbranch-protection=bti --save-temps" } */ > +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */ > > int > main (void) > diff --git a/gcc/testsuite/gcc.target/arm/bti-2.c b/gcc/testsuite/gcc.target/arm/bti-2.c > index 33910563849a4..3284aba3020cc 100644 > --- a/gcc/testsuite/gcc.target/arm/bti-2.c > +++ b/gcc/testsuite/gcc.target/arm/bti-2.c > @@ -3,6 +3,7 @@ > /* { dg-options "-Os" } */ > /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */ > /* { dg-options "-march=armv8.1-m.main -mthumb -mfloat-abi=softfp -mbranch-protection=bti --save-temps" } */ > +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */ > > extern int f1 (void); > extern int f2 (void); > R.
On Apr 16, 2024, "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com> wrote: > The require-effective-target flags test whether a specific set of > flags will make the compilation work, so they need to be used in > conjunction with the corresponding dg-add-options flags that then > apply those options. *nod*, that's the theory. Problem is the architectures suported by [add_options_for_]arm_arch_*[_ok] do not match exactly those expected by the tests, and I can't quite tell whether the subtle changes they would introduce would change what they intend to test, or even whether the differences are irrelevant, or would be sensible to add as variants to the dg machinery. I think it would take someone more familiar than I am with all of the ARM variants to do this correctly. I don't even know how these changes would need to be tested to be sure they remain correct. Would you be willing to take it from here, or would you accept the patch as an incremental yet imperfect improvement, or would you prefer to guide me in making it correct, and in verifying it (there are questions below)? I don't have a lot of cycles to put into this (we've already worked around the testsuite bugs we ran into), but it would be desirable to get a fix into GCC as well, if we can converge on one without unreasonably burdening anyone. v8_1m_main "-march=armv8.1-m.main+fp -mthumb" __ARM_ARCH_8M_MAIN__ v8_1m_main_pacbti "-march=armv8.1-m.main+pacbti+fp -mthumb" "__ARM_ARCH_8M_MAIN__ && __ARM_FEATURE_BTI && __ARM_FEATURE_PAUTH Why do these have +fp in -march but not in the v8_1m* arch name? gcc/testsuite/g++.target/arm/pac-1.C: /* { dg-options "-march=armv8.1-m.main+mve+pacbti -mbranch-protection=pac-ret -mthumb -mfloat-abi=hard -g -O0" } */ v8_1m_main_pacbti plus +mve minus +fp. Do we need a dg arch for that? gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-7.c: /* { dg-additional-options "-march=armv8.1-m.main+pacbti+fp --save-temps -mfloat-abi=hard" } */ gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-11.c: /* { dg-options "-march=armv8.1-m.main+fp+pacbti" } */ v8_1m_main_pacbti minus -mthumb. AFAICT the -mthumb is redundant. gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-12.c: /* { dg-options "-march=armv8-m.main+fp -mfloat-abi=softfp" } */ v8_1m_main minus -mthumb. AFAICT the -mthumb is redundant. gcc/testsuite/gcc.target/arm/bti-1.c: /* { dg-options "-march=armv8.1-m.main -mthumb -mfloat-abi=softfp -mbranch-protection=bti --save-temps" } */ gcc/testsuite/gcc.target/arm/bti-2.c: /* { dg-options "-march=armv8.1-m.main -mthumb -mfloat-abi=softfp -mbranch-protection=bti --save-temps" } */ v8_1m_main minus +fp. Can these be bumped to +fp, or do we need an extra dg arch? Are these missing +pacbti? Thanks,
On 19/04/2024 13:45, Alexandre Oliva wrote: > On Apr 16, 2024, "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com> wrote: > >> The require-effective-target flags test whether a specific set of >> flags will make the compilation work, so they need to be used in >> conjunction with the corresponding dg-add-options flags that then >> apply those options. > > *nod*, that's the theory. Problem is the architectures suported by > [add_options_for_]arm_arch_*[_ok] do not match exactly those expected by > the tests, and I can't quite tell whether the subtle changes they would > introduce would change what they intend to test, or even whether the > differences are irrelevant, or would be sensible to add as variants to > the dg machinery. I think it would take someone more familiar than I am > with all of the ARM variants to do this correctly. I don't even know > how these changes would need to be tested to be sure they remain > correct. It's ok to add additional variations to the table of variants in target-supports.exp, but we should avoid writing new specific run-time functions unless we really want an executable test. I started doing some cleanup of the Arm tests infrastructure during phase 3, but stopped during phase 4 as I wanted to minimise the changes being made now. I plan to go back and work on it some more once stage 1 re-opens. > > Would you be willing to take it from here, or would you accept the patch > as an incremental yet imperfect improvement, or would you prefer to > guide me in making it correct, and in verifying it (there are questions > below)? I don't have a lot of cycles to put into this (we've already > worked around the testsuite bugs we ran into), but it would be desirable > to get a fix into GCC as well, if we can converge on one without > unreasonably burdening anyone. > > > v8_1m_main "-march=armv8.1-m.main+fp -mthumb" __ARM_ARCH_8M_MAIN__ > v8_1m_main_pacbti "-march=armv8.1-m.main+pacbti+fp -mthumb" > "__ARM_ARCH_8M_MAIN__ && __ARM_FEATURE_BTI && __ARM_FEATURE_PAUTH > > Why do these have +fp in -march but not in the v8_1m* arch name? It's ... complicated :) The +fp is there because, with the move to having -mfpu=auto as the default, we need to avoid problems when the compiler has been configured with --with-float=hard, which requires the extension register set (fp or vector support) even if the test code itself doesn't care. The best way to handle this in most cases is to give the architecture strings a default FPU specification (ie +fp). > > > gcc/testsuite/g++.target/arm/pac-1.C: > /* { dg-options "-march=armv8.1-m.main+mve+pacbti -mbranch-protection=pac-ret -mthumb -mfloat-abi=hard -g -O0" } */ > > v8_1m_main_pacbti plus +mve minus +fp. > Do we need a dg arch for that? I'd be inclined to drop +mve from this one; there's nothing I can see in the test that would generate mve instructions, so I think it's irrelevant. We can use the existing v8_1m_main_pacbti operations. > > > gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-7.c: > /* { dg-additional-options "-march=armv8.1-m.main+pacbti+fp --save-temps -mfloat-abi=hard" } */ > gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-11.c: > /* { dg-options "-march=armv8.1-m.main+fp+pacbti" } */ > > v8_1m_main_pacbti minus -mthumb. > AFAICT the -mthumb is redundant. Nearly, but not quite. Although the gcc driver knows that m-profile architectures require thumb, that's not enough to override an explicit -marm from a testsuite configuration run, so if your site.exp file adds -marm in a test configuration we need to override that or the test will fail. But the table based list of options will do that for you. > > > gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-12.c: > /* { dg-options "-march=armv8-m.main+fp -mfloat-abi=softfp" } */ > > v8_1m_main minus -mthumb. > AFAICT the -mthumb is redundant. As above > > > gcc/testsuite/gcc.target/arm/bti-1.c: > /* { dg-options "-march=armv8.1-m.main -mthumb -mfloat-abi=softfp -mbranch-protection=bti --save-temps" } */ > gcc/testsuite/gcc.target/arm/bti-2.c: > /* { dg-options "-march=armv8.1-m.main -mthumb -mfloat-abi=softfp -mbranch-protection=bti --save-temps" } */ > > v8_1m_main minus +fp.> > Can these be bumped to +fp, or do we need an extra dg arch? > > Are these missing +pacbti? The tests themselves do not require fp, but if we use the effective-target rules (arm_arch_v8_1m_main), we can remove the -march, -mthumb and -mfloat-abi flags from these tests. These tests for BTI should NOT have +pacbti: they're testing that the compiler generates the right nop-based implementation that is backwards compatible with CPUs that do not have this feature.[1] R. > > > Thanks, > [1] The PAC and BTI features define the behaviour of some architectural NOP instructions: on older CPUs these instructions have no effect (they are NOPs), but on newer CPUs these NOPs take on a new behaviour that implements the feature (PAC or BTI). R.
diff --git a/gcc/testsuite/g++.target/arm/pac-1.C b/gcc/testsuite/g++.target/arm/pac-1.C index f671a27b048c6..f48ad6cc5cb65 100644 --- a/gcc/testsuite/g++.target/arm/pac-1.C +++ b/gcc/testsuite/g++.target/arm/pac-1.C @@ -2,6 +2,7 @@ /* { dg-do compile } */ /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */ /* { dg-options "-march=armv8.1-m.main+mve+pacbti -mbranch-protection=pac-ret -mthumb -mfloat-abi=hard -g -O0" } */ +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */ __attribute__((noinline)) void fn1 (int a, int b, int c) diff --git a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-11.c b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-11.c index 6a5ae92c567f3..dba4f491cfea7 100644 --- a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-11.c +++ b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-11.c @@ -1,6 +1,7 @@ /* { dg-do compile } */ /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" "-mfloat-abi=*" } } */ /* { dg-options "-march=armv8.1-m.main+fp+pacbti" } */ +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */ #if (__ARM_FEATURE_BTI != 1) #error "Feature test macro __ARM_FEATURE_BTI_DEFAULT should be defined to 1." diff --git a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-12.c b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-12.c index db40b17c3b030..308a41eb4ba4c 100644 --- a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-12.c +++ b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-12.c @@ -1,6 +1,7 @@ /* { dg-do compile } */ /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */ /* { dg-options "-march=armv8-m.main+fp -mfloat-abi=softfp" } */ +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */ #if defined (__ARM_FEATURE_BTI) #error "Feature test macro __ARM_FEATURE_BTI should not be defined." diff --git a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-7.c b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-7.c index 1b25907635e24..10836a84bde56 100644 --- a/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-7.c +++ b/gcc/testsuite/gcc.target/arm/acle/pacbti-m-predef-7.c @@ -1,6 +1,7 @@ /* { dg-do compile } */ /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */ /* { dg-additional-options "-march=armv8.1-m.main+pacbti+fp --save-temps -mfloat-abi=hard" } */ +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */ #if defined (__ARM_FEATURE_BTI_DEFAULT) #error "Feature test macro __ARM_FEATURE_BTI_DEFAULT should be undefined." diff --git a/gcc/testsuite/gcc.target/arm/bti-1.c b/gcc/testsuite/gcc.target/arm/bti-1.c index 79dd8010d2dab..34655387b55f5 100644 --- a/gcc/testsuite/gcc.target/arm/bti-1.c +++ b/gcc/testsuite/gcc.target/arm/bti-1.c @@ -2,6 +2,7 @@ /* { dg-do compile } */ /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */ /* { dg-options "-march=armv8.1-m.main -mthumb -mfloat-abi=softfp -mbranch-protection=bti --save-temps" } */ +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */ int main (void) diff --git a/gcc/testsuite/gcc.target/arm/bti-2.c b/gcc/testsuite/gcc.target/arm/bti-2.c index 33910563849a4..3284aba3020cc 100644 --- a/gcc/testsuite/gcc.target/arm/bti-2.c +++ b/gcc/testsuite/gcc.target/arm/bti-2.c @@ -3,6 +3,7 @@ /* { dg-options "-Os" } */ /* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" "-mcpu=*" } } */ /* { dg-options "-march=armv8.1-m.main -mthumb -mfloat-abi=softfp -mbranch-protection=bti --save-temps" } */ +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */ extern int f1 (void); extern int f2 (void);