On Thu, 7 Nov 2024 at 19:09, Torbjorn SVENSSON <torbjorn.svens...@foss.st.com> wrote: > > > > On 2024-11-07 16:33, Richard Earnshaw (lists) wrote: > > On 06/11/2024 19:50, Torbjorn SVENSSON wrote: > >> > >> > >> On 2024-11-06 19:06, Richard Earnshaw (lists) wrote: > >>> On 06/11/2024 13:50, Torbjorn SVENSSON wrote: > >>>> > >>>> > >>>> On 2024-11-06 14:04, Richard Earnshaw (lists) wrote: > >>>>> On 06/11/2024 12:23, Torbjorn SVENSSON wrote: > >>>>>> > >>>>>> > >>>>>> On 2024-11-06 12:26, Richard Earnshaw (lists) wrote: > >>>>>>> On 06/11/2024 07:44, Christophe Lyon wrote: > >>>>>>>> On Wed, 6 Nov 2024 at 07:20, Torbjörn SVENSSON > >>>>>>>> <torbjorn.svens...@foss.st.com> wrote: > >>>>>>>>> > >>>>>>>>> While the regression was reported on GCC15, I'm sure that same > >>>>>>>>> regression will be seen on GCC14 when it's tested in the > >>>>>>>>> arm-linux-gnueabihf configuration. > >>>>>>>>> > >>>>>>>>> Ok for trunk and releases/gcc-14? > >>>>>>>>> > >>>>>>>>> -- > >>>>>>>>> > >>>>>>>>> This fixes reported regression at > >>>>>>>>> https://linaro.atlassian.net/browse/GNU-1407. > >>>>>>>>> > >>>>>>>>> gcc/testsuite/ChangeLog: > >>>>>>>>> > >>>>>>>>> * gcc.target/arm/pr68620.c: Use effective-target arm_fp. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svens...@foss.st.com> > >>>>>>>>> --- > >>>>>>>>> gcc/testsuite/gcc.target/arm/pr68620.c | 4 +++- > >>>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>>>>>>>> > >>>>>>>>> diff --git a/gcc/testsuite/gcc.target/arm/pr68620.c > >>>>>>>>> b/gcc/testsuite/gcc.target/arm/pr68620.c > >>>>>>>>> index 6e38671752f..1ed84f4ac75 100644 > >>>>>>>>> --- a/gcc/testsuite/gcc.target/arm/pr68620.c > >>>>>>>>> +++ b/gcc/testsuite/gcc.target/arm/pr68620.c > >>>>>>>>> @@ -1,8 +1,10 @@ > >>>>>>>>> /* { dg-do compile } */ > >>>>>>>>> /* { dg-skip-if "-mpure-code supports M-profile without Neon > >>>>>>>>> only" { *-*-* } { "-mpure-code" } } */ > >>>>>>>>> /* { dg-require-effective-target arm_arch_v7a_ok } */ > >>>>>>>>> -/* { dg-options "-mfp16-format=ieee -mfpu=auto -mfloat-abi=softfp" > >>>>>>>>> } */ > >>>>>>>>> +/* { dg-require-effective-target arm_fp_ok } */ > >>>>>>>>> +/* { dg-options "-mfp16-format=ieee -mfpu=auto" } */ > >>>>>>>>> /* { dg-add-options arm_arch_v7a } */ > >>>>>>>>> +/* { dg-add-options arm_fp } */ > >>>>>>>>> > >>>>>>>> > >>>>>>>> So... this partially reverts your previous patch (bringing back > >>>>>>>> arm_fp). What is the problem now? > >>>>>>>> > >>>>>>> > >>>>>>> Yeah, that sounds wrong. arm_fp_ok tries to find options to add to > >>>>>>> the basic testsuite options, but it can't be combined with > >>>>>>> arm_arch_v7a as that picks a totally different set of flags for the > >>>>>>> architecture. > >>>>>> > >>>>>> The problem is that for arm-linux-gnueabihf, we cannot use > >>>>>> -mfloat-abi=softfp as there is no multilib available for that ABI, or > >>>>>> at least that's my interpretation of below error message. > >>>>>> > >>>>>> This is the output from the CI run: > >>>>>> > >>>>>> Executing on host: > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/builds/destdir/armv8l-unknown-linux-gnueabihf/bin/armv8l-unknown-linux-gnueabihf-gcc > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/snapshots/gcc.git~master/gcc/testsuite/gcc.target/arm/pr68620.c > >>>>>> -fdiagnostics-plain-output -mfp16-format=ieee -mfpu=auto > >>>>>> -mfloat-abi=softfp -mcpu=unset -march=armv7-a+fp -S -o pr68620.s > >>>>>> (timeout = 600) > >>>>>> spawn -ignore SIGHUP > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/builds/destdir/armv8l-unknown-linux-gnueabihf/bin/armv8l-unknown-linux-gnueabihf-gcc > >>>>>> > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/snapshots/gcc.git~master/gcc/testsuite/gcc.target/arm/pr68620.c > >>>>>> -fdiagnostics-plain-output -mfp16-format=ieee -mfpu=auto > >>>>>> -mfloat-abi=softfp -mcpu=unset -march=armv7-a+fp -S -o pr68620.s > >>>>>> In file included from /usr/include/features.h:510, > >>>>>> from > >>>>>> /usr/include/arm-linux-gnueabihf/bits/libc-header-start.h:33, > >>>>>> from /usr/include/stdint.h:26, > >>>>>> from > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/builds/destdir/armv8l-unknown-linux-gnueabihf/lib/gcc/armv8l-unknown-linux-gnueabihf/15.0.0/include/stdint.h:11, > >>>>>> from > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/builds/destdir/armv8l-unknown-linux-gnueabihf/lib/gcc/armv8l-unknown-linux-gnueabihf/15.0.0/include/arm_fp16.h:34, > >>>>>> from > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/builds/destdir/armv8l-unknown-linux-gnueabihf/lib/gcc/armv8l-unknown-linux-gnueabihf/15.0.0/include/arm_neon.h:41, > >>>>>> from > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/snapshots/gcc.git~master/gcc/testsuite/gcc.target/arm/pr68620.c:7: > >>>>>> /usr/include/arm-linux-gnueabihf/gnu/stubs.h:7:11: fatal error: > >>>>>> gnu/stubs-soft.h: No such file or directory > >>>>>> compilation terminated. > >>>>>> compiler exited with status 1 > >>>>>> output is: > >>>>>> In file included from /usr/include/features.h:510, > >>>>>> from > >>>>>> /usr/include/arm-linux-gnueabihf/bits/libc-header-start.h:33, > >>>>>> from /usr/include/stdint.h:26, > >>>>>> from > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/builds/destdir/armv8l-unknown-linux-gnueabihf/lib/gcc/armv8l-unknown-linux-gnueabihf/15.0.0/include/stdint.h:11, > >>>>>> from > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/builds/destdir/armv8l-unknown-linux-gnueabihf/lib/gcc/armv8l-unknown-linux-gnueabihf/15.0.0/include/arm_fp16.h:34, > >>>>>> from > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/builds/destdir/armv8l-unknown-linux-gnueabihf/lib/gcc/armv8l-unknown-linux-gnueabihf/15.0.0/include/arm_neon.h:41, > >>>>>> from > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/snapshots/gcc.git~master/gcc/testsuite/gcc.target/arm/pr68620.c:7: > >>>>>> /usr/include/arm-linux-gnueabihf/gnu/stubs.h:7:11: fatal error: > >>>>>> gnu/stubs-soft.h: No such file or directory > >>>>>> compilation terminated. > >>>>>> > >>>>>> comp_output (pruned) is: > >>>>>> In file included from /usr/include/features.h:510, > >>>>>> from > >>>>>> /usr/include/arm-linux-gnueabihf/bits/libc-header-start.h:33, > >>>>>> from /usr/include/stdint.h:26, > >>>>>> from > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/builds/destdir/armv8l-unknown-linux-gnueabihf/lib/gcc/armv8l-unknown-linux-gnueabihf/15.0.0/include/stdint.h:11, > >>>>>> from > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/builds/destdir/armv8l-unknown-linux-gnueabihf/lib/gcc/armv8l-unknown-linux-gnueabihf/15.0.0/include/arm_fp16.h:34, > >>>>>> from > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/builds/destdir/armv8l-unknown-linux-gnueabihf/lib/gcc/armv8l-unknown-linux-gnueabihf/15.0.0/include/arm_neon.h:41, > >>>>>> from > >>>>>> /home/tcwg-buildslave/workspace/tcwg_gnu_1/abe/snapshots/gcc.git~master/gcc/testsuite/gcc.target/arm/pr68620.c:7: > >>>>>> /usr/include/arm-linux-gnueabihf/gnu/stubs.h:7:11: fatal error: > >>>>>> gnu/stubs-soft.h: No such file or directory > >>>>>> compilation terminated. > >>>>>> > >>>>>> FAIL: gcc.target/arm/pr68620.c (test for excess errors) > >>>>>> > >>>>>> So, if we cannot use arm_fp, what can we use instead to check if it > >>>>>> needs -mfloat-abi=softfp or -mfloat-abi=hard? > >>>>>> > >>>>>> Kind regards, > >>>>>> Torbjörn > >>>>> > >>>>> Ah, got you. Interestingly my ubuntu armhf instalation has booth > >>>>> stubs-soft.h and stubs-hard.h even though it's a hard-float > >>>>> environment, so I wouldn't see this error. I'm not sure if that's > >>>>> something ubuntu has fixed, or whether it's because the tcwg install is > >>>>> slightly non-standard. > >>>>> > >>>>> Technically this isn't a multilib - because we aren't linking, but I > >>>>> see where you're coming from. I need to think a bit about this one. > >>>>> What we really need is some way of checking if stdint.h works in a > >>>>> soft-fp environment; we might need a new check for this in > >>>>> target-supports.exp. > >>>> > >>>> An alternative is to add an entry in the list that is > >>>> arm_arch_v7a_softfp and then do dg-require-effective-target on it, but > >>>> then it will not be tested for HF only-targets. Is this a good enough > >>>> compromise? We could also go the other way and do the "hard" way, but > >>>> either way, one of them would not be tested in that case. > >>>> > >>>> Let me know what you think. > >>>> > >>>> It's the same problem in the attr-neon* patch that I have in parallell > >>>> to this one, but I'll wait for this one to have a way forward before > >>>> working more on that one. > >>>> > >>>> Kind regards, > >>>> Torbjörn > >>> > >>> I think the attached is probably a better approach here. It tries to > >>> find just the correct ABI flags to add while remaining compatible with > >>> the platform. > >>> > >>> It's not perfect: the compiler doesn't really support -mfloat-abi=softfp > >>> correctly when generating thumb1 code, but that's a different bug. > >> > >> I think I'm missing something here. > >> > >> As far as I can tell, your approach is more or less identical to the > >> arm_fp_ok check. The only difference is that you are including stdint.h, > >> while arm_fp_ok checks that __ARM_FP is defined. From my point of view, > >> they are both a bit flawed here. > >> Wouldn't it be better to just add the include of stdint.h to arm_fp_ok? > > > > No, because we can use arm_fp_ok when we don't intend to include any > > headers, which makes it more compatible with a lot of tests. > > Ah, of course. I didn't think of that. > > > > >> > >> For easier reference, this is the implementation of arm_fp_ok in GCC15: > >> > >> proc check_effective_target_arm_fp_ok_nocache { } { > >> global et_arm_fp_flags > >> set et_arm_fp_flags "" > >> if { [check_effective_target_arm32] } { > >> foreach flags {"" "-mfloat-abi=softfp" "-mfloat-abi=hard"} { > >> if { [check_no_compiler_messages_nocache arm_fp_ok object { > >> #ifndef __ARM_FP > >> #error __ARM_FP not defined > >> #endif > >> } "$flags"] } { > >> set et_arm_fp_flags $flags > >> return 1 > >> } > >> } > >> } > >> > >> return 0 > >> } > > > > This is quite similar, but definitely not the same. Firstly, it tries not > > adding any option at all (the ""); then it tries softfp and hard. However, > > I very much doubt we ever end up with 'hard' here because either softfp > > will enable FP, or there's no FP features in the default flags anyway. > > > > My test will either select 'hard' or 'soft' but it won't ever select > > neither (well unless both flags lead to an error, but in that case we'd > > expect to skip the test). > > > >> > >> > >> When evaluating arm_libc_fp_abi_ok in the pr68620.c test case with your > >> patch, that's still in the context of what multilib flags that might have > >> been passed to dejagnu and not in the armv7-a context, right? > > > > Correct, which is why I don't look directly at __ARM_FP. I'm just trying > > to find out whether the libc headers (stdint.h specifically at present) are > > compatible with adding softfp or hard as the float abi. > > Ok. > > > > >> > >> > >> To me, it feels like the way we use dg-require-effective-target to check > >> various flags is not implemented in a way that would allow you to switch > >> target in a test case. Are we trying to achive something that is not > >> possible with the current design of the dg- comments? > > > > We could structure the testsuite so that test are only run if the base > > flags are compatible with the test and to never add additional > > (architecture-related) flags when building. But that would require us to > > run the testsuite with hundreds, if not thousands of option permutations to > > get coverage of some corner cases. So instead we try to coerce the command > > line to something that is appropriate for the test. It generally works > > wihtout too much trouble, but a small number of cases (like this) can cause > > some problems. > > > > Idealy we'd only require one effective-target test and one add-options > > command (to add the options we've detected above), but that would end up > > with many more cases to add to target-supports.exp and I'm not convinced we > > really need that here. > > I think I might have been unclear of my thoughts here. > I was not thinking that we should only run for a single target, I was > more thinking that the dg-require-effective-target needs to take into > consideration what flags has already been given via dg-options, > dg-additional-options or dg-add-options in addition to the multilib > flags provided to dejagnu. Currently, it's only the multilib flags given > to dejagnu that matters, right? > If we do more than one dg-require-effective-target + dg-add-options in a > test case, where the 1st one do a change of target, then I think that > the 2nd one would be evaluated in the wrong context and potentially give > the wrong flags to be added. > > Maybe I'm overthinking this... > > Anyway, I've sent a v2 with your suggested solution. > As I cannot see the reported failure locally, is there some way I can
Not sure what you mean? > check the status myself of the patch in the pre-commit checks done in > the linaro farm or does that require someone at Arm and/or linaro to > check that for me? Linaro precommit CI will send you a notification if it detects a regression (but it only runs native aarch64-linux-gnu and arm-linux-gnueabihf, postcommit CI has more targets, including several arm-eabi). But you can check the status on patchwork: https://patchwork.sourceware.org/ then go to "view patches" under "GNU Compiler Collection" The bots update the S/W/F column, hopefully you'll get 4 "green" from Linaro CI. Thanks, Christophe > > Kind regards, > Torbjörn > > > > > R. > >> > >> Kind regards, > >> Torbjörn > > >
