[U-Boot] [PATCH v2 2/2] arm: work around assembler bug

[Tom Rini]

On Tue, Jul 10, 2012 at 11:04:50AM -0700, Allen Martin wrote:
> On Tue, Jul 10, 2012 at 10:50:34AM -0700, Tom Rini wrote:
> > On Tue, Jul 10, 2012 at 10:44:33AM -0700, Allen Martin wrote:
> > > On Tue, Jul 10, 2012 at 10:25:05AM -0700, Tom Rini wrote:
> > > > * PGP Signed by an unknown key
> > > > 
> > > > On Tue, Jul 10, 2012 at 10:00:17AM -0700, Allen Martin wrote:
> > > > > Disable sibling call optimization based on binutils version.  This is
> > > > > to work around a bug in the assember in binutils versions < 2.22.
> > > > > Branches to weak symbols can be incorrectly optimized in thumb mode to
> > > > > a short branch (b.n instruction) that won't reach when the symbol gets
> > > > > preempted.
> > > > > 
> > > > > http://sourceware.org/bugzilla/show_bug.cgi?id=12532
> > > > > 
> > > > > Signed-off-by: Allen Martin <amartin at nvidia.com>
> > > > > ---
> > > > > changes for v2:
> > > > >  -changed GAS_BUG_12532 from yes/no to y/n to be consistent
> > > > >  -added additional warning about code size increase
> > > > > ---
> > > > >  arch/arm/config.mk |   21 +++++++++++++++++++++
> > > > >  1 file changed, 21 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm/config.mk b/arch/arm/config.mk
> > > > > index 3f4453a..bcabf17 100644
> > > > > --- a/arch/arm/config.mk
> > > > > +++ b/arch/arm/config.mk
> > > > > @@ -87,3 +87,24 @@ endif
> > > > >  ifndef CONFIG_NAND_SPL
> > > > >  LDFLAGS_u-boot += -pie
> > > > >  endif
> > > > > +
> > > > > +#
> > > > > +# binutils versions < 2.22 have a bug in the assembler where branches
> > > > > +# to weak symbols can be incorrectly optimized in thumb mode to a
> > > > > +# short branch (b.n instruction) that won't reach when the symbol
> > > > > +# gets preempted
> > > > > +#
> > > > > +# http://sourceware.org/bugzilla/show_bug.cgi?id=12532
> > > > > +#
> > > > > +ifeq ($(CONFIG_SYS_THUMB_BUILD),y)
> > > > > +ifeq ($(GAS_BUG_12532),)
> > > > > +export GAS_BUG_12532:=$(shell if [ $(call binutils-version) -lt 0222 ] ; then echo y; else echo n; fi)
> > > > 
> > > > Please don't squash out whitespace, just break this out into a
> > > > multi-line test ala the others in the toplevel config.mk.
> > > 
> > > Ok
> > > 
> > > > > +ifeq ($(GAS_BUG_12532),y)
> > > > > +$(warning *** disabling sibling call optimzation because binutils version < 2.22)
> > > > > +$(warning *** code size may be slightly larger)
> > > > > +endif
> > > > > +endif
> > > > > +ifeq ($(GAS_BUG_12532),y)
> > > > > +PLATFORM_RELFLAGS += -fno-optimize-sibling-calls
> > > > > +endif
> > > > > +endif
> > > > 
> > > > This seems a bit longer than needed.  Just check that it's y once and do
> > > > both things.  Also, I _think_ we should be able to squish this down to:
> > > > ifeq ($(CONFIG_SYS_THUMB_BUILD),y)
> > > > GAS_BUG_12532 := $(shell if [ $(call binutils-version) -lt 0222 ] ; \
> > > > 	then echo y; else echo n; fi)
> > > > ifeq ($(GAS_BUG_125320),y)
> > > > $(warning ...)
> > > > $(warning ...)
> > > > PLATFORM_RELFLAGS += ...
> > > > endif
> > > > endif
> > > > 
> > > > But I haven't tested it.
> > > 
> > > I wrote it the way I did because I only want the expensive
> > > shell/binutils-version to get evaluated once per "make".  I think what
> > > you proposed would cause it to get re-evaluated every time the build
> > > goes down into a subdir.
> > 
> > It should still get evaluated once, I think.  Kluding the
> > binutils-version script to echo 1 >> /tmp/test every time it's run would
> > tell tho for sure :)
> 
> I tried it out, it's obvious from the $(warning) it's getting
> reevaluated:
> 
> $ ./MAKEALL -s tegra20
> /home/arm/u-boot/arch/arm/config.mk:102: *** disabling sibling call optimzation because binutils version < 2.22
> /home/arm/u-boot/arch/arm/config.mk:103: *** code size may be slightly larger
> Configuring for harmony board...
>    text    data     bss     dec     hex filename
>  170334    4284  278352  452970   6e96a ./u-boot
>   11676     161      36   11873    2e61 ./spl/u-boot-spl
> /home/arm/u-boot/arch/arm/config.mk:102: *** disabling sibling call optimzation because binutils version < 2.22
> /home/arm/u-boot/arch/arm/config.mk:103: *** code size may be slightly larger
> /home/arm/u-boot/arch/arm/config.mk:102: *** disabling sibling call optimzation because binutils version < 2.22
> /home/arm/u-boot/arch/arm/config.mk:103: *** code size may be slightly larger
> /home/arm/u-boot/arch/arm/config.mk:102: *** disabling sibling call optimzation because binutils version < 2.22
> /home/arm/u-boot/arch/arm/config.mk:103: *** code size may be slightly larger
> /home/arm/u-boot/arch/arm/config.mk:102: *** disabling sibling call optimzation because binutils version < 2.22
> /home/arm/u-boot/arch/arm/config.mk:103: *** code size may be slightly larger
> /home/arm/u-boot/arch/arm/config.mk:102: *** disabling sibling call optimzation because binutils version < 2.22
> /home/arm/u-boot/arch/arm/config.mk:103: *** code size may be slightly larger
> /home/arm/u-boot/arch/arm/config.mk:102: *** disabling sibling call optimzation because binutils version < 2.22
> /home/arm/u-boot/arch/arm/config.mk:103: *** code size may be slightly larger
> ... and on and on
> 
> 
> It makes sense since unless GAS_BUG_125320 is "export"ed to the new
> make, it's going to be unset again each the build starts a new
> "make -C" 

Thanks for proving me wrong.  Just the white-space / multi line thing
please, thanks :)

-- 
Tom