| Message ID | CAD57uCcwLCJttGUoLDJcfJF16di+w5F9Hp2B78fywnpaMD32UQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Apr 09, 2015 at 01:10:41PM +0200, Yvan Roux wrote: > 2105-04-09 Yvan Roux <yvan.roux@linaro.org> > > PR target/65648 > * gcc.c-torture/execute/pr65648.c: New test. This part is definitely ok for trunk. > * gcc.target/arm/pr65648.c: New test. This part should better be reviewed by some ARM maintainer, I'm really lost in all the incompatible ARM options. Jakub
On 09/04/15 12:10, Yvan Roux wrote: > diff --git a/gcc/testsuite/gcc.target/arm/pr65648.c b/gcc/testsuite/gcc.target/arm/pr65648.c > new file mode 100644 > index 0000000..e075546 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/arm/pr65648.c > @@ -0,0 +1,9 @@ > +/* { dg-do run } */ > +/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-march=*" } { "-march=armv6" } } */ > +/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" } { "" } } */ > +/* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { "*" } { "" } } */ > +/* { dg-options "-mthumb -Os -mfloat-abi=soft" } */ > +/* { dg-add-options arm_arch_v6 } */ > + > +#include "../../gcc.c-torture/execute/pr65648.c" > + Hi Yvan, These are always tough to get right. How about: /* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { "*" } { "" } } */ /* { dg-options "-Os -mthumb -mfloat-abi=soft" } */ /* { dg-add-options arm_arch_v6 } */ /* { dg-require-effective-target arm_arch_v6_ok } */ ? I think the dg-skip-if will avoid the error when testing arm-none-linux-gnueabihf: "error: ./pr65648.exe uses VFP register arguments, /tmp/ccXpRQ41.o does not" The dg-require-effective-target should remove the need for the first dg-skip-if in your options. I don't think it's worth skipping the test when the user explicitly asks for -marm. It won't test the behaviour of the bug but then again, the user overrode the options, so presumably knows best. Is there any case where this fails? Kyrill
On 13 April 2015 at 15:42, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: > > On 09/04/15 12:10, Yvan Roux wrote: >> >> diff --git a/gcc/testsuite/gcc.target/arm/pr65648.c >> b/gcc/testsuite/gcc.target/arm/pr65648.c >> new file mode 100644 >> index 0000000..e075546 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/arm/pr65648.c >> @@ -0,0 +1,9 @@ >> +/* { dg-do run } */ >> +/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { >> "-march=*" } { "-march=armv6" } } */ >> +/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" >> } { "" } } */ >> +/* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { >> "*" } { "" } } */ >> +/* { dg-options "-mthumb -Os -mfloat-abi=soft" } */ >> +/* { dg-add-options arm_arch_v6 } */ >> + >> +#include "../../gcc.c-torture/execute/pr65648.c" >> + > > Hi Yvan, > > These are always tough to get right. > How about: > /* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { "*" } > { "" } } */ > /* { dg-options "-Os -mthumb -mfloat-abi=soft" } */ > /* { dg-add-options arm_arch_v6 } */ > /* { dg-require-effective-target arm_arch_v6_ok } */ > ? > > I think the dg-skip-if will avoid the error when testing > arm-none-linux-gnueabihf: > "error: ./pr65648.exe uses VFP register arguments, /tmp/ccXpRQ41.o does > not" > > The dg-require-effective-target should remove the need for the first > dg-skip-if in your options. > I don't think it's worth skipping the test when the user explicitly asks for > -marm. It won't test the > behaviour of the bug but then again, the user overrode the options, so > presumably knows best. Yes it looks better to me, the -marm skipping in my patch was an artifact of testing it for armv6-m > Is there any case where this fails? none that I can think of, Is it ok to commit after I've re-tested it (maybe once 5.1 is released) ? Yvan
On 13/04/15 15:10, Yvan Roux wrote: > On 13 April 2015 at 15:42, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: >> On 09/04/15 12:10, Yvan Roux wrote: >>> diff --git a/gcc/testsuite/gcc.target/arm/pr65648.c >>> b/gcc/testsuite/gcc.target/arm/pr65648.c >>> new file mode 100644 >>> index 0000000..e075546 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.target/arm/pr65648.c >>> @@ -0,0 +1,9 @@ >>> +/* { dg-do run } */ >>> +/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { >>> "-march=*" } { "-march=armv6" } } */ >>> +/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" >>> } { "" } } */ >>> +/* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { >>> "*" } { "" } } */ >>> +/* { dg-options "-mthumb -Os -mfloat-abi=soft" } */ >>> +/* { dg-add-options arm_arch_v6 } */ >>> + >>> +#include "../../gcc.c-torture/execute/pr65648.c" >>> + >> Hi Yvan, >> >> These are always tough to get right. >> How about: >> /* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { "*" } >> { "" } } */ >> /* { dg-options "-Os -mthumb -mfloat-abi=soft" } */ >> /* { dg-add-options arm_arch_v6 } */ >> /* { dg-require-effective-target arm_arch_v6_ok } */ >> ? >> >> I think the dg-skip-if will avoid the error when testing >> arm-none-linux-gnueabihf: >> "error: ./pr65648.exe uses VFP register arguments, /tmp/ccXpRQ41.o does >> not" >> >> The dg-require-effective-target should remove the need for the first >> dg-skip-if in your options. >> I don't think it's worth skipping the test when the user explicitly asks for >> -marm. It won't test the >> behaviour of the bug but then again, the user overrode the options, so >> presumably knows best. > Yes it looks better to me, the -marm skipping in my patch was an > artifact of testing it for armv6-m > >> Is there any case where this fails? > none that I can think of, Is it ok to commit after I've re-tested it > (maybe once 5.1 is released) ? Yes, the arm part is ok. I believe Jakub ok'ed the gcc.c-torture hunk. I think it can go in now, as it is a testcase for a PR that was fixed for GCC 5. Does it need to be committed to the release branch as well? Thanks, Kyrill > > Yvan >
On Mon, Apr 13, 2015 at 03:36:06PM +0100, Kyrill Tkachov wrote: > Yes, the arm part is ok. I believe Jakub ok'ed the gcc.c-torture hunk. > I think it can go in now, as it is a testcase for a PR that was fixed for GCC 5. > Does it need to be committed to the release branch as well? Yes, but it probably should wait until 5.1 is released, no reason to rush it in immediately. Jakub
On Thu, Apr 9, 2015 at 12:10 PM, Yvan Roux <yvan.roux@linaro.org> wrote: > Hi > > On 7 April 2015 at 22:02, Yvan Roux <yvan.roux@linaro.org> wrote: >> On 7 April 2015 at 21:33, Jakub Jelinek <jakub@redhat.com> wrote: >>> On Tue, Apr 07, 2015 at 09:28:51PM +0200, Yvan Roux wrote: >>>> validation is ongoing, but here is my attempt to add this testcase, >>>> does it look correct (it's the first time I use that kind of include >>>> in testsuite) >>> >>> The intent is that we have a testcase for all targets at various >>> optimization levels, plus one with specific options for the particular >>> target. >>> If you get at least one FAIL with this patch with Vlad's patch reverted and >>> no FAILs with that patch, the patch is ok for trunk with the obvious >>> ChangeLog entry. > > As this testcase needs to be executed, we can have some conflict at > link time, depending on how the libs were compiled. Here is what I've > for the moment, let me know if it's ok and/or if you have suggestion > on how to improve it. > > - armv6 doesn't support the hard-float ABI in Thumb mode, I disable > the testcase with this directive, but not sure it's the best way: > { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { > "*" } { "" } }ot > > - The original problem was reported on armv6-m arch. but is not > related to the M profile, if we stick to armv6-m the link will fail on > compiler which default to the -A profile. As my guess is that -A > profile is more widely tested, I changed the -march flag to armv6. Do > you think it's ok ? IMO there are enough folks who test M profile. I'd drop all the arch specific options and just apply the patch. Thanks, Ramana > > With the attached patch there is only 1 FAIL before Vlad's patch on > the execution of the test and they all PASS when testing > arm-linux-gnueabi, and the target test is UNSUPPORTED when test > arm-linux-gnueabihf. > > Cheers, > Yvan > > 2105-04-09 Yvan Roux <yvan.roux@linaro.org> > > PR target/65648 > * gcc.c-torture/execute/pr65648.c: New test. > * gcc.target/arm/pr65648.c: New test.
On 14 April 2015 at 10:19, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > On Thu, Apr 9, 2015 at 12:10 PM, Yvan Roux <yvan.roux@linaro.org> wrote: >> Hi >> >> On 7 April 2015 at 22:02, Yvan Roux <yvan.roux@linaro.org> wrote: >>> On 7 April 2015 at 21:33, Jakub Jelinek <jakub@redhat.com> wrote: >>>> On Tue, Apr 07, 2015 at 09:28:51PM +0200, Yvan Roux wrote: >>>>> validation is ongoing, but here is my attempt to add this testcase, >>>>> does it look correct (it's the first time I use that kind of include >>>>> in testsuite) >>>> >>>> The intent is that we have a testcase for all targets at various >>>> optimization levels, plus one with specific options for the particular >>>> target. >>>> If you get at least one FAIL with this patch with Vlad's patch reverted and >>>> no FAILs with that patch, the patch is ok for trunk with the obvious >>>> ChangeLog entry. >> >> As this testcase needs to be executed, we can have some conflict at >> link time, depending on how the libs were compiled. Here is what I've >> for the moment, let me know if it's ok and/or if you have suggestion >> on how to improve it. >> >> - armv6 doesn't support the hard-float ABI in Thumb mode, I disable >> the testcase with this directive, but not sure it's the best way: >> { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { >> "*" } { "" } }ot >> >> - The original problem was reported on armv6-m arch. but is not >> related to the M profile, if we stick to armv6-m the link will fail on >> compiler which default to the -A profile. As my guess is that -A >> profile is more widely tested, I changed the -march flag to armv6. Do >> you think it's ok ? > > IMO there are enough folks who test M profile. I'd drop all the arch > specific options and just apply the patch. The issue is more related to armv6 than M profile, but if it is widely tested as well I can just commit the torture test if it's ok for Jakub. Thanks, Yvan > Thanks, > Ramana > >> >> With the attached patch there is only 1 FAIL before Vlad's patch on >> the execution of the test and they all PASS when testing >> arm-linux-gnueabi, and the target test is UNSUPPORTED when test >> arm-linux-gnueabihf. >> >> Cheers, >> Yvan >> >> 2105-04-09 Yvan Roux <yvan.roux@linaro.org> >> >> PR target/65648 >> * gcc.c-torture/execute/pr65648.c: New test. >> * gcc.target/arm/pr65648.c: New test.
On Tue, Apr 14, 2015 at 10:32:16AM +0200, Yvan Roux wrote: > The issue is more related to armv6 than M profile, but if it is widely > tested as well I can just commit the torture test if it's ok for > Jakub. If it is tested by enough people, just the execute.exp test is ok of course. Jakub
On Tue, Apr 14, 2015 at 9:33 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Apr 14, 2015 at 10:32:16AM +0200, Yvan Roux wrote: >> The issue is more related to armv6 than M profile, but if it is widely >> tested as well I can just commit the torture test if it's ok for >> Jakub. > > If it is tested by enough people, just the execute.exp test is ok of course. > > Jakub Yeah the execute.exp test is fine. Ramana
On 14 April 2015 at 10:35, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > On Tue, Apr 14, 2015 at 9:33 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Tue, Apr 14, 2015 at 10:32:16AM +0200, Yvan Roux wrote: >>> The issue is more related to armv6 than M profile, but if it is widely >>> tested as well I can just commit the torture test if it's ok for >>> Jakub. >> >> If it is tested by enough people, just the execute.exp test is ok of course. >> >> Jakub > > Yeah the execute.exp test is fine. Ok, I'll commit it this afternoon then. Thanks YVan > Ramana
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr65648.c b/gcc/testsuite/gcc.c-torture/execute/pr65648.c new file mode 100644 index 0000000..88a2fc9 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr65648.c @@ -0,0 +1,34 @@ +/* PR target/65648 */ + +int a = 0, *b = 0, c = 0; +static int d = 0; +short e = 1; +static long long f = 0; +long long *i = &f; +unsigned char j = 0; + +__attribute__((noinline, noclone)) void +foo (int x, int *y) +{ + asm volatile ("" : : "r" (x), "r" (y) : "memory"); +} + +__attribute__((noinline, noclone)) void +bar (const char *x, long long y) +{ + asm volatile ("" : : "r" (x), "r" (&y) : "memory"); + if (y != 0) + __builtin_abort (); +} + +int +main () +{ + int k = 0; + b = &k; + j = (!a) - (c <= e); + *i = j; + foo (a, &k); + bar ("", f); + return 0; +} diff --git a/gcc/testsuite/gcc.target/arm/pr65648.c b/gcc/testsuite/gcc.target/arm/pr65648.c new file mode 100644 index 0000000..e075546 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pr65648.c @@ -0,0 +1,9 @@ +/* { dg-do run } */ +/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-march=*" } { "-march=armv6" } } */ +/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" } { "" } } */ +/* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { "*" } { "" } } */ +/* { dg-options "-mthumb -Os -mfloat-abi=soft" } */ +/* { dg-add-options arm_arch_v6 } */ + +#include "../../gcc.c-torture/execute/pr65648.c" +