Re: [PATCH] MIPS/BFD: Correct/clean up ELF FP attribute warning messages

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Mon, 19 Nov 2012, Richard Sandiford wrote:

> >  While working on MIPS BFD ELF attribute code I have noticed that the 
> > double- and single-float cases are swapped in warning messages -- the 
> > former is denoted with "1" and the latter is denoted with "2" and the 
> > messages interpret them in the opposite manner.  I guess that is an 
> > unfortunate consequence of using magic numbers everywhere rather than 
> > meaningful names and then a human error.  I have a follow-up change that 
> > will correct this approach, however here is an independent essential 
> > update to put the messages straight.
> >
> >  While working on this I have also noticed the messages are inconsistent 
> > in that some report the name and the attribute of the output file first, 
> > followed by the offending input file and its attribute, while the others 
> > use the opposite order.  This only adds to the overall confusion, so I 
> > have decided to take the opportunity and make the structure of all the 
> > messages consistent, with the output file first -- this way the left-hand 
> > side is going to stay the same across all the messages, if multiple are 
> > produced.
> 
> Sorry to be a pain, but I don't think this part is a good change.
> The current code is consistent in that it always uses the same error
> message for the same incompatibility, whereas the changed code is
> consistent in the order of the bfds.  From that point of view the
> patch is trading one consistency for another.  But really, the fact
> that we're using the output bfd at all is poor error reporting.
> The real incompability is between two input objects, so ideally the
> error message would be too.  (I've hit cases where the input bfd
> had the ABI I wanted and it's taken me a while to figure out where
> the "wrong" one came from.)  If that was fixed, I think we'd want
> the current messages instead of the new ones.

 BFD infers the output ABI from the first input file mapped to output, 
there's no option to the linker to choose one AFAICT, at least not for the 
MIPS target and this attribute (but then it'll be up to the individual 
backend to choose which file name to use in reporting).  I suppose it 
should be moderately easy to remember the name and use it.  I'll see if I 
can spend half an hour to implement that.

> As things stand, this patch doubles the number of error messages
> that need to be translated.

 I don't think we should ever trade user friendliness for easier 
maintenance unless the effort required turns out prohibitive.  Which is 
clearly not the case here, so I'm disregarding this argument, sorry.

 And I stand by my view: whether it's the output or the first input file 
that's reported, it's any later inputs that may be incompatible to it, not 
the other way round.  I fail to see how e.g.:

Warning: bar.o uses -msingle-float, foo.o uses -mdouble-float
Warning: foo.o uses -mdouble-float, baz.o uses -mips32r2 -mfp64

is any clearer than:

Warning: foo.o uses -mdouble-float, bar.o uses -msingle-float
Warning: foo.o uses -mdouble-float, baz.o uses -mips32r2 -mfp64

-- in fact I think the opposite is the case.  With the latter (and the 
knowledge the ABI chosen for output is reported first) you immediately see 
what ABI the linker decided to use.  With the former you have to study the 
messages in detail and investigate actual file names reported to see which 
one is repeated; and also IMO the wording slightly suggests that the ABI 
is actually switched as the changes are seen: first it's -msingle-float, 
then -mdouble-float, and finally -mips32r2 -mfp64.  And last but not least 
if there is only one message, then you never know which ABI resulted and 
have to reach for `readelf', etc.

 Of course you can say this may not necessarily matter in practice, but I 
think in the face of such an ABI incompatibility problem to know whether 
the ABI chosen by the linker meets one's expectations has a value too.

> A patch to fix the -msingle-float/-mdouble-float mixup without changing
> the set of error strings is preapproved though.  Thanks for catching it.

 I'll defer any further work here until you have convinced me warning 
reporting following your point of view provides value beyond one following 
mine (there's no bonus for keeping status quo).

 Meanwhile I'll look into keeping that first input BFD file name for 
reporting.  And actually, after a bit of thinking, I believe it may even 
make sense to say e.g.:

Warning: foobar uses -mdouble-float (set by foo.o), baz.o uses -mips32r2 -mfp64

to emphasize both the ABI chosen for output and where it came from.  What 
do you think?

  Maciej