This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] MIPS/BFD: Correct/clean up ELF FP attribute warning messages
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> 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?
OK, go ahead with your patch.
Richard