Re: [PATCH v4 1/2] MIPS: support mips*64 as CPU and gnuabi64 as ABI

["Maciej W. Rozycki" <macro@orcam.me.uk>]

On Fri, 21 Jul 2023, YunQiang Su wrote:

> Debian, as a *REAL* OS, instead of anything you are imagining, is *USING* it.
> I don't agree that you make any "features" heavenly.
> If so, we cannot fix any bug.

 Sometimes bugs have to live forever if the removal would cause more harm 
than keeping them.  So we best don't introduce them in the first place.  
Of course a human beeings we are all bound to make mistakes, but this is 
the very reason we have come up with thorough peer reviews: to catch any 
mistakes early on, before they cause actual harm.  Some will escape 
regardless, but we need to try hard and prevent that from happening 
rather than being lax.

 The use of `mipsisa64-*-*' triplets for o32 ABI configurations wasn't a 
bug though, it was a design decision.  Now making these triplets mean n32 
after they meant something else for 20+ years seems like a bug to me.

> More and more software in the *REAL* world, use the CPU section (here mipsisa64)
> to determine the 64bit OS/env.
> Anyway, the *REAL* world is much more important the the world only in
> your *MIND*.

 Personal insults are not going to help you with getting changes accepted, 
as we're making decisions based on technical merits rather than opinions.

 I don't think a fait accompli ("Debian does it") is a valid argument in a 
technical review.  It is just a statement of fact and we do not have to 
accept the same solution while Debian is free to continue using it.

 At the risk of repeating myself, you do need to justify your change 
before it can be accepted, that is you have to provide technical arguments 
to prove it to the reviewer that taking your change is the right thing to 
do.  You may certainly reuse the arguments that were given at the time the 
choice was evaluated for Debian and if we find them convincing, we will 
accept the change.

 I don't think we have a document written down for submitting patches
specifically for binutils, but we have been broadly following the rules
for the GNU toolchain, as documented on pages for our sibling projects:

<https://gcc.gnu.org/contribute.html>,
<https://sourceware.org/gdb/wiki/ContributionChecklist>,
<https://sourceware.org/glibc/wiki/Contribution_checklist>.

Please familiarise yourself with these documents before posting further 
changes and follow the patch submission guidelines set out there.

 I'd expect you, now as a nominated MIPS GCC backend maintainer, to be 
especially familiar with the first of these documents, the rules of which 
you are expected not only to apply to your own changes, but to enforce 
them for patches submitted by other people, whether by the general public 
or your fellow employees.  As far I can tell it wasn't regrettably the 
case with the MIPS16e2 patchset I volunteered to properly review (having 
been involved with the instruction set design and having implemented the 
binutils side) and which offer was ignored.

 And in any case you can do whatever you want in Debian as long as it's 
within what the licence allows you to, but we do not necessarily have to 
accept your changes.  Ideally Debian developers would have asked us in 
advance if an incompatible change they want to make locally would be 
accepted, or actually would have submitted it so that it lands here first 
(or is rejected, in which case they'd have a chance to find an alternative 
solution).

 I would have argued then you probably want `mips64isa64*-*-*' or suchlike 
a triplet for your 64-bit ABI configuration, leaving `mipsisa64*-*-*' ones 
for 32-bit ABI configurations (at least as far as the default choice is 
concerned), as that would be consistent with our current `mips*-*-*' vs 
`mips64*-*-*' naming scheme.

 In that scheme for the machine part we have, in this order:

1. The base prefix: `mips'/`mips64' to tell 32-bit/64-bit configurations
   apart.

2. An optional implementation specification, e.g. `vr4300', `r5900', 
   `isa32r2', etc., which sets the CPU default as with the `-march=' 
   option.

3. An optional `el' endianness suffix to make the little endianness the 
   default rather than the big one.

So `mipstx39el-*-*' is a 32-bit configuration with the TX39 CPU chosen by 
default and little endianness.  Likewise `mips64r10000-*-*' is a 64-bit 
big-endian one with the R10k default.  Then `mipsocteon+-*-*' is a 32-bit 
one with Octeon+, and for a 64-bit one then say `mips64octeon+-*-*'.

 Why would you want to make it different for `isa64' then?  Say 
`mipsisa64-*-*' for a 32-bit configuration and then `mips64isa64-*-*' for 
a 64-bit one.  In fact it already works like this: you can use 
`mips64isa64r6-linux-gnu' and `mips64isa64r6el-linux-gnu' for your Debian 
system right away and get what you need, there's nothing to change.

 We do have a couple of legacy aberrations from very old, pre-MIPS32 days, 
most notably `mips-sgi-irix6', but I think there's no need to go back 
there.

 Of course not all configurations are valid, e.g. `mips64isa32-*-*' is 
not, but neither is say `mips64tx39-*-*', so that's no news.  And then 
`mipsinteraptiv-mr2el-*-*' is broken due to the presence of the hyphen in 
`mipsinteraptiv-mr2', which is an oversight of mine: we need to define an 
`interaptiv_mr2' (or `interaptiv+mr2') alias to allow this configuration, 
similarly to how say `74kf3_2' has been named, and remember not to use a 
hyphen in any future CPU names.

 There's really no need to break anything here, we've had it from time 
immemorial.

 I note however that `config.sub' hasn't been correctly updated for the 
MIPS CPU names and consequently we have a random choice only that works 
and other ones don't.  In fact given the variability of our MIPS 
configurations I fail to see why it doesn't just let it all through.  I 
have submitted a fix now.

  Maciej