[U-Boot] [LONG] Re: [PATCH v3 0/6] Optimize ARM relocation

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

On Tue, 18 Jun 2013 16:54:57 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> Hi Jeroen,
> 
> On Sun, 16 Jun 2013 15:33:32 +0200, Jeroen Hofstee
> <jeroen@myspectrum.nl> wrote:
> 
> > Hello Albert,
> > 
> > On 06/13/2013 08:54 PM, Jeroen Hofstee wrote:
> > >
> > >>>> binaries only use one type of relocation record,
> > >>>> R_ARM_RELATIVE., then optimizing relocation code
> > >>>> accordingly.
> > >>>
> > >
> > 
> > fyi, I had a look at the clang build and it doesn't boot
> > after these patches...
> > 
> > When constant pointers are used without fpie, e.g. the
> > arguments to printf, gcc will load the address with ldr /
> > R_ARM_ABS32, clang sets the address with a movw /
> > movt pair.
> 
> Hmm... Why do you remove -fpie from the gcc build?
> 
> > ld -r will make the former relative, but the movw / movt
> > cannot be fixed. So I set fpie for clang, which generates
> > a couple of R_ARM_ABS32:
> > 
> > Most notably a reference to do_bootd. Since it is no
> > longer valid after this patch and used in command.c this
> > crashes the board (unless there happens to be a valid
> > address in it). gcc seems to always recalculate it using pc.
> > 
> > Another symbol is _start, but that looks easily fixable.
> > 
> > The last one looks similar like the do_bootd and I haven't
> > bothered to check the details.
> 
> Can you give more precise info on the issue? Such as the U-Boot
> codebase (even if not in shape for submitting) and clang compiler
> version you are using, so that I can try the build on my side and have
> a look at how/why r_arm_abs32 relocation recodes are generated and how
> to avoid it?

Update:

After some IRC discussion with Jeroen, we've been able to pinpoint an
issue when building armv7 (and possibly others) with optimization level
-O2 instead of -Os (OPTFLAGS in ./config.mk). NOTE: -O2 is not the
optimization level defined by default, and default builds do not
exhibit any issue.

Specifically, building the twister target with -O2 results in the build
failing with this error message: "arm-linux-gnueabi-ld.bfd:
arch/arm/cpu/armv7/omap3/libomap3.o: relocation R_ARM_MOVW_ABS_NC
against `a local symbol' can not be used when making a shared object;
recompile with -fPIC".

Compiler option -fPIC was replaced with linker option -pie when
switching from GOT to ELF relocation tables in commit 92d5ecba.

Adding -fPIE (rather than -fPIC as we are linking an executable)
results in the build succeeding again, but with a few R_ARM_ABS32
relocations creeping back in.

Analysis of the absolute relocations show that their causes are:

1. use of 'ldr r0, =_start' instead of 'adr r0,_start' in
   arch/arm/cpu/armv7/start.S (twice) -- actually, ldr refers to the
   link-time _start, which may be right or wrong, whereas adr refers
   to the run-time _start, which is always correct.

2. use of 'ldr r0, =_start' instead of 'adr r0,_start' in
   arch/arm/lib/relocate.S. Same remark as above.

3. Declaration EXPORT_FUNC(do_reset) in include/_exports.h.
   That is the only 'do_xxx' exported function, and I suspect there is
   no actual need for exporting it.
   Incidentally, the U_BOOT_CMD() statement is in common/cmd_boot.c
   while the do_reset function is in arch/arm/lib/reset -- that's
   surprising, although I understand why we might not want a
   common/do_reset.c file with only the U_BOOT_CMD() statement.

4. There is an explicit comparision between a function pointer and
   do_bootd for recursion detection reasons. While I am not sure why
   there is a recursion issue, I am sure however that we could test
   for recursion by turning the 'repeatable' field in cmd_tbl_s into
   a 'flags' field with bit 0 set for 'repeatable' and bit 1 set when
   command is 'do_bootd', thereby removing the test for the do_bootd
   address.

Even though -O2 (plus -fPIE) is not currently used in U-boot, and even
though it reduces the relocation table size, I don't intend to switch to
it because this increases the code and data size -- for twister, global
increase is 4.5% for u-boot and 15.5% for SPL.

Nevertheless, I think we should fix the issues above, if only because
they are indicative of potential trouble, so I'll send out a patch for
this; but it'll be based over the relocation optimization patches to
avoid merge conflicts.

Amicalement,
-- 
Albert.