Hi Graeme,
On Tue, 10 Jul 2012 10:57:39 +1000, Graeme Russ graeme.russ@gmail.com wrote:
Hi Allen
On Tue, Jul 10, 2012 at 10:45 AM, Allen Martin amartin@nvidia.com wrote:
On Sat, Jul 07, 2012 at 03:15:36AM -0700, Albert ARIBAUD wrote:
Hi Allen,
On Fri, 6 Jul 2012 16:17:19 -0700, Allen Martin amartin@nvidia.com wrote:
[snip]
And I forgot to mention, the code bloat from disabling the optimization is about 400 bytes (185136 -> 185540), so it's not bad, but it still seems a shame to disable all short branches because of one misoptimized one.
0.2% be my calcs
Can this not be limited to compiling the object files which are known to be sensitive to the problem?
I understand this issue fairly well now. It's a known bug in the assembler that has already been fixed:
http://sourceware.org/bugzilla/show_bug.cgi?id=12532
It only impacts preembtable symbols, and since u-boot doesn't have any dynamic loadable objects it's only explictly defined weak symbols that should trigger the bug.
I built a new toolchain with binutils 2.22 and verified the bug is no longer present there, and -fno-optimize-sibling-calls is the correct workaround for toolchains that do have the bug, so conditionally disabling the optimization for binutils < 2.22 seems like the right fix.
I ran a quick scrub of the u-boot tree and there's 195 instances of __attribute__((weak)) spread across 123 source files, so I think just disabling optimization on the failing object files may be too fragile, as code movement could cause others to crop up.
Adding -fno-optimize-sibling-calls for binutils < 2.22 - 0.2% code size increase for people using slightly older tools
Maintain the tweaking of a set of files - someone using binutils >= 2.22 adds __attribute__((weak)) to a single function and *BAM* three months later someone complains that something broke
I vote option 1
I do wonder, though, if we should spit out warnings when applying workaraounds for older tool-chains?
I am against this idea: this persistent warning will either worry or anoy the reader, neither of which is good IMO. OTOH, when in the future the workaround is removed because the toolchain version it fixes is considered obsolete, *then* we shall add a warning to let developers know that they use an *unsupported* binutils version.
Meanwhile, we could mark the workaround with a FIXME note in the code, for present and future U-Boot *developers* to notice and remember what they should do with this workaround. :)
Regards,
Graeme
Amicalement,