mips64-linux-gnuabi64 testsuite breakage

mips64-linux-gnuabi64 testsuite breakage

[Alan Modra]

Since 32f1c80375e "MIPS: support mips*64 as CPU and gnuabi64 as ABI",
I'm seeing 187 new testsuite fails on mips64-linux.  That strongly
indicates untested, wrong changes have been committed.

-- 
Alan Modra
Australia Development Lab, IBM

Re: mips64-linux-gnuabi64 testsuite breakage

[YunQiang Su]

Alan Modra via Binutils <binutils@sourceware.org> 于2023年4月26日周三 12:53写道：
>
> Since 32f1c80375e "MIPS: support mips*64 as CPU and gnuabi64 as ABI",

I will fix the test suite.
The reason is that: the previous test suite assumes the default ABI is
n32 even the triple
with -gnuabi64.

> I'm seeing 187 new testsuite fails on mips64-linux.  That strongly
> indicates untested, wrong changes have been committed.
>
> --
> Alan Modra
> Australia Development Lab, IBM



-- 
YunQiang Su

Re: mips64-linux-gnuabi64 testsuite breakage

[Maciej W. Rozycki]

On Wed, 26 Apr 2023, YunQiang Su wrote:

> > Since 32f1c80375e "MIPS: support mips*64 as CPU and gnuabi64 as ABI",
> 
> I will fix the test suite.
> The reason is that: the previous test suite assumes the default ABI is
> n32 even the triple
> with -gnuabi64.

 I can see extra 3346 regressions across various MIPS targets compared to 
a checkout from Jan.  Including ones like (parts of log trimmed for 
clarity):

../as-new  --defsym count=960 /scratch/vol1/binutils/binutils-mips-test/binutils-src/gas/testsuite/gas/mips/branch-swap-2.s
gas/testsuite/gas/mips/branch-swap-2.s:1: Fatal error: `micromips' cannot be used with `mips32r6'
FAIL: MIPS branch swapping (960)

for `mips-img-elf' which indicates that the default architecture has 
changed for GAS for this target.  I think this is due to your commit 
9171de358f23 ("MIPS: default output r6 obj if the triple is r6"), which 
has this part among others:

+	# If Vendor is IMG, then MIPSr6 is used
+	case ${target} in
+	  mips*64*-img-*)
+	    mips_cpu=mips64r6
+	    ;;
+	  mips*-img-*)
+	    mips_cpu=mips32r6
+	    ;;
+	esac

Not only this change is wrong (you can't just arbitrarily change a 
configuration that has been in the wild for ~8.5 years as it'll break 
things for people who rely on the established semantics), but it hasn't 
been properly verified either, as a change is supposed not to cause 
regressions in the testsuite.

 Please revert the part quoted and fix the regressions.  Thank you.

  Maciej

Re: mips64-linux-gnuabi64 testsuite breakage

[YunQiang Su]

Maciej W. Rozycki <macro@orcam.me.uk> 于2023年5月3日周三 03:33写道：
>
> On Wed, 26 Apr 2023, YunQiang Su wrote:
>
> > > Since 32f1c80375e "MIPS: support mips*64 as CPU and gnuabi64 as ABI",
> >
> > I will fix the test suite.
> > The reason is that: the previous test suite assumes the default ABI is
> > n32 even the triple
> > with -gnuabi64.
>
>  I can see extra 3346 regressions across various MIPS targets compared to
> a checkout from Jan.  Including ones like (parts of log trimmed for
> clarity):
>
> ../as-new  --defsym count=960 /scratch/vol1/binutils/binutils-mips-test/binutils-src/gas/testsuite/gas/mips/branch-swap-2.s
> gas/testsuite/gas/mips/branch-swap-2.s:1: Fatal error: `micromips' cannot be used with `mips32r6'
> FAIL: MIPS branch swapping (960)
>
> for `mips-img-elf' which indicates that the default architecture has
> changed for GAS for this target.  I think this is due to your commit
> 9171de358f23 ("MIPS: default output r6 obj if the triple is r6"), which
> has this part among others:
>
> +       # If Vendor is IMG, then MIPSr6 is used
> +       case ${target} in
> +         mips*64*-img-*)
> +           mips_cpu=mips64r6
> +           ;;
> +         mips*-img-*)
> +           mips_cpu=mips32r6
> +           ;;
> +       esac
>
> Not only this change is wrong (you can't just arbitrarily change a
> configuration that has been in the wild for ~8.5 years as it'll break
> things for people who rely on the established semantics), but it hasn't
> been properly verified either, as a change is supposed not to cause
> regressions in the testsuite.
>
>  Please revert the part quoted and fix the regressions.  Thank you.

Reverted.

And I am working on fix the tesutsuite for `-gnuabi64` and `mipsisaXXr6`.

>
>   Maciej



-- 
YunQiang Su

Re: mips64-linux-gnuabi64 testsuite breakage

[Maciej W. Rozycki]

On Thu, 4 May 2023, YunQiang Su wrote:

> > +       # If Vendor is IMG, then MIPSr6 is used
> > +       case ${target} in
> > +         mips*64*-img-*)
> > +           mips_cpu=mips64r6
> > +           ;;
> > +         mips*-img-*)
> > +           mips_cpu=mips32r6
> > +           ;;
> > +       esac
> >
> > Not only this change is wrong (you can't just arbitrarily change a
> > configuration that has been in the wild for ~8.5 years as it'll break
> > things for people who rely on the established semantics), but it hasn't
> > been properly verified either, as a change is supposed not to cause
> > regressions in the testsuite.
> >
> >  Please revert the part quoted and fix the regressions.  Thank you.
> 
> Reverted.

 Thank you.

 FAOD if you think a triplet that implicitly defaults to R6 is useful, 
then feel free to submit a change for say `mips*-cipunited-*' to do so (or 
maybe `mips*-cip-*' if you want to keep it short and the company policy 
allows using such a short name), as this would be a configuration that had 
no previous mention/semantics in our tree and therefore I would have no 
concerns about it.

 Overall I'd suggest the `--with-arch=' approach previously discussed and 
already implemented for MIPS targets in GCC and also some other targets in 
binutils.  It is more generic, although it may complicate testing, so it 
may have to be deployed with care.

> And I am working on fix the tesutsuite for `-gnuabi64` and `mipsisaXXr6`.

 I look forward to your fixes.

  Maciej

Re: mips64-linux-gnuabi64 testsuite breakage

[YunQiang Su]

Maciej W. Rozycki <macro@orcam.me.uk> 于2023年5月8日周一 01:46写道：
>
> On Thu, 4 May 2023, YunQiang Su wrote:
>
> > > +       # If Vendor is IMG, then MIPSr6 is used
> > > +       case ${target} in
> > > +         mips*64*-img-*)
> > > +           mips_cpu=mips64r6
> > > +           ;;
> > > +         mips*-img-*)
> > > +           mips_cpu=mips32r6
> > > +           ;;
> > > +       esac
> > >
> > > Not only this change is wrong (you can't just arbitrarily change a
> > > configuration that has been in the wild for ~8.5 years as it'll break
> > > things for people who rely on the established semantics), but it hasn't
> > > been properly verified either, as a change is supposed not to cause
> > > regressions in the testsuite.
> > >
> > >  Please revert the part quoted and fix the regressions.  Thank you.
> >
> > Reverted.
>
>  Thank you.
>
>  FAOD if you think a triplet that implicitly defaults to R6 is useful,

No, I don't think that it is useful at all. It is so uglyyyyyyy.
In fact, we should use the correct cpu name in triple, just like
mipsisa32r6 etc.

> then feel free to submit a change for say `mips*-cipunited-*' to do so (or
> maybe `mips*-cip-*' if you want to keep it short and the company policy
> allows using such a short name), as this would be a configuration that had
> no previous mention/semantics in our tree and therefore I would have no
> concerns about it.
>

No, I won't. I submit the patch about `img`, is due to that, this vendor value
did/does/being used.

IMG/MIPS/Wave/MIPS does provide the toolchain with img as its vendor name,
and the default arch is R6.

https://codescape.mips.com/components/toolchain/2021.09-01/downloads.html

>  Overall I'd suggest the `--with-arch=' approach previously discussed and
> already implemented for MIPS targets in GCC and also some other targets in
> binutils.  It is more generic, although it may complicate testing, so it
> may have to be deployed with care.
>
> > And I am working on fix the tesutsuite for `-gnuabi64` and `mipsisaXXr6`.
>
>  I look forward to your fixes.
>
>   Maciej



-- 
YunQiang Su

Re: mips64-linux-gnuabi64 testsuite breakage

[Richard Sandiford]

"Maciej W. Rozycki" <macro@orcam.me.uk> writes:
> On Wed, 26 Apr 2023, YunQiang Su wrote:
>
>> > Since 32f1c80375e "MIPS: support mips*64 as CPU and gnuabi64 as ABI",
>> 
>> I will fix the test suite.
>> The reason is that: the previous test suite assumes the default ABI is
>> n32 even the triple
>> with -gnuabi64.
>
>  I can see extra 3346 regressions across various MIPS targets compared to 
> a checkout from Jan.  Including ones like (parts of log trimmed for 
> clarity):
>
> ../as-new  --defsym count=960 /scratch/vol1/binutils/binutils-mips-test/binutils-src/gas/testsuite/gas/mips/branch-swap-2.s
> gas/testsuite/gas/mips/branch-swap-2.s:1: Fatal error: `micromips' cannot be used with `mips32r6'
> FAIL: MIPS branch swapping (960)
>
> for `mips-img-elf' which indicates that the default architecture has 
> changed for GAS for this target.  I think this is due to your commit 
> 9171de358f23 ("MIPS: default output r6 obj if the triple is r6"), which 
> has this part among others:
>
> +	# If Vendor is IMG, then MIPSr6 is used
> +	case ${target} in
> +	  mips*64*-img-*)
> +	    mips_cpu=mips64r6
> +	    ;;
> +	  mips*-img-*)
> +	    mips_cpu=mips32r6
> +	    ;;
> +	esac
>
> Not only this change is wrong (you can't just arbitrarily change a 
> configuration that has been in the wild for ~8.5 years as it'll break 
> things for people who rely on the established semantics),

It wasn't supposed to be an arbitrary change, but instead was supposed
to sync GAS's default to GCC's.  The GCC img toolchain has been an r6
toolchain since it was added in 2014.  The fact that GAS instead defaults
to mips1/mips3 seems like a bug, since mips1 isn't link-compatible with
mips32r6 and mips3 isn't link-compatible with mips64r6.

> but it hasn't been properly verified either, as a change is supposed
> not to cause regressions in the testsuite.

Can't argue with that part.  But...

>  Please revert the part quoted and fix the regressions.  Thank you.

...I think it does make sense to resurrect the patch in a testsuite-friendly
form.

Thanks,
Richard

Re: mips64-linux-gnuabi64 testsuite breakage

[Maciej W. Rozycki]

On Tue, 9 May 2023, Richard Sandiford wrote:

> > Not only this change is wrong (you can't just arbitrarily change a 
> > configuration that has been in the wild for ~8.5 years as it'll break 
> > things for people who rely on the established semantics),
> 
> It wasn't supposed to be an arbitrary change, but instead was supposed
> to sync GAS's default to GCC's.  The GCC img toolchain has been an r6
> toolchain since it was added in 2014.  The fact that GAS instead defaults
> to mips1/mips3 seems like a bug, since mips1 isn't link-compatible with
> mips32r6 and mips3 isn't link-compatible with mips64r6.

 Fair enough, but:

1. It should have been submitted as a distinct patch on its own, rather
   than bundled with an unrelated change.

2. The rationale as you quote it should have been given in the change 
   description.

> >  Please revert the part quoted and fix the regressions.  Thank you.
> 
> ...I think it does make sense to resurrect the patch in a testsuite-friendly
> form.

 And following the patch submission requirements repeated above, right.

  Maciej