[U-Boot] [PATCH 2/4] arm: add %function attribute to assembly functions

[Aneesh V]

On Saturday 18 February 2012 06:54 PM, Aneesh V wrote:
> Hi Albert,
>
> On Saturday 18 February 2012 03:43 PM, Albert ARIBAUD wrote:
>> Hi Aneesh,
>>
>> Le 17/02/2012 12:09, Aneesh V a écrit :
>>> Hi Albert,
>>>
>>> On Wednesday 15 February 2012 07:27 PM, Aneesh V wrote:
>>>> This is done using the following directive preceding
>>>> each function definition:
>>>>
>>>> .type<func-name>, %function
>>>>
>>>> This marks the symbol as a function in the object
>>>> header which in turn helps the linker in some cases.
>>>>
>>>> In particular this was found needed for resolving ARM/Thumb
>>>> calls correctly in a build with Thumb interworking enabled.
>>>>
>>>> This solves the following problem I had reported earlier:
>>>>
>>>> "When U-Boot/SPL is built using the Thumb instruction set the
>>>> toolchain has a potential issue with weakly linked symbols.
>>>> If a function has a weakly linked default implementation in C
>>>> and a real implementation in assembly GCC is confused about the
>>>> instruction set of the assembly implementation. As a result
>>>> the assembly function that is built in ARM is executed as
>>>> if it is Thumb. This results in a crash"
>>>>
>>>> Signed-off-by: Aneesh V<aneesh at ti.com>
>>>
>>> Does this look good to you. I was a bit nervous about touching so many
>>> files. Please let me know if you would prefer to change only the OMAP
>>> function that was creating the ARM/Thumb problem. I did a "MAKEALL -a
>>> arm" and didn't see any new errors.
>>>
>>> Let me know if this is an acceptable solution to the problem.
>>
>> Regarding the solution: it is quite ok to me. I would just like to
>> understand the exact effect of the .function directive, what its options
>> are and if some of these should not be explicitly specified.
>>
>> Regarding touching many files: I won't be worried as long as you check
>> that the first three patches have no effect on existing boards. This can
>> be verified as follows -- if you haven't done so already:
>>
>> - build your OMAP target without the patch set and do a hex dump of
>> u-boot.bin;
>>
>> - apply the first three patches of your set, rebuild your OMAP target
>> without the patch set and do a hex dump of u-boot.bin;
>>
>> - compare both dumps. Normally you should only see one difference, in
>> the build version and date -- if .function does not actually alter the
>> assembly code, which I hope it indeed does not when building for ARM.
>>
>> If there are more changes than build version and date, then they might
>> be due to .function requiring some yet unknown additional option, or to
>> some change in patch 1 or 3 not being completely conditioned on
>> CONFIG_SYS_THUMB_BUILD.
>
> I can reproduce the problem with a simple test program.
> Note: I can reproduce this with Sourcery G++ Lite 2010q1-202 (GCC 4.4.1
> - Binutils 2.19.51.20090709)
> But I *can not* reproduce reproduce this with Linaro GCC 2012.01 (GCC
> 4.6.3 , Binutils 2.22)

Linaro GCC 2012.01 has the same problem when assembly function(ARM is
called from C (Thumb). I can reproduce it using this program:

a.c:
====
int main (void)
{
   foo ();
}

b.S:
====
.text
.align  2
.global foo
foo:
	push	{r7}
	add	r7, sp, #0
	mov	sp, r7
	pop	{r7}
	bx	lr
	.size	foo, .-foo

.global __aeabi_unwind_cpp_pr0
__aeabi_unwind_cpp_pr0:
	bx	lr

arm-linux-gnueabi-gcc -mthumb -mthumb-interwork -c a.c
arm-linux-gnueabi-gcc -mthumb -mthumb-interwork -c b.S
arm-linux-gnueabi-ld -r a.o -o alib.o
arm-linux-gnueabi-ld -r b.o -o blib.o
arm-linux-gnueabi-ld --start-group alib.o blib.o --end-group -o a.out
arm-linux-gnueabi-objdump -S --reloc a.out

gives:
     8076:	af00      	add	r7, sp, #0
     8078:	f000 f802 	bl	8080 <foo>
     807c:	4618      	mov	r0, r3

It should have been "blx foo"

Again, %function solves it. Conclusion: %function is necessary with
both old and new tool-chains when building for Thumb.


It should have been "blx	8080 <foo>", isn't it? Again, %function
solves it.

Conclusion: %function is necessary with both old and new tool-chains
when building for Thumb.

> So apparently the issue has been fixed recently. Unfortunately Linaro
> GCC 2012.01 creates a new Thumb problem that I am investigating now.
> Somehow I missed this when I tested earlier. So, my Thumb build is
> not working with Linaro GCC 2012.01. But this one is not reproduced on
> Sourcery G++ Lite 2010q1-202!
>
> Here is the program I used to reproduce the problem in Sourcery G++
> Lite 2010q1-202 that this patch is addressing
>
> a.c:
> ====
> extern void foo (void) __attribute__ ((weak, alias ("__foo")));
>
> void __foo (void)
> {
> }
>
> extern void call_foo(void);
>
> int main (void)
> {
> call_foo ();
> }
>
> b.S:
> ====
> .text
> .align 2
> .global foo
> foo:
> push {r7}
> add r7, sp, #0
> mov sp, r7
> pop {r7}
> bx lr
> .size foo, .-foo
>
>
> c.S:
> ====
> .text
> .align 2
>
> .global call_foo
> call_foo:
> bl foo
> bx lr
>
> .global __aeabi_unwind_cpp_pr0
> __aeabi_unwind_cpp_pr0:
> bx lr
>
> Now build it and take the assembly dump using the following commands:
>
> arm-none-linux-gnueabi-gcc -mthumb -mthumb-interwork -c a.c
> arm-none-linux-gnueabi-gcc -mthumb -mthumb-interwork -c b.S
> arm-none-linux-gnueabi-gcc -mthumb -mthumb-interwork -c c.S
> arm-none-linux-gnueabi-ld -r a.o -o alib.o
> arm-none-linux-gnueabi-ld -r b.o -o blib.o
> arm-none-linux-gnueabi-ld -r c.o -o clib.o
> arm-none-linux-gnueabi-ld --start-group clib.o alib.o blib.o --end-group
> -o a.out
> arm-none-linux-gnueabi-objdump -S --reloc a.out
>
> You will get something like this in the assembly dump:
> 00008094 <call_foo>:
> 8094: fa000006 blx 80b4 <foo>
> 8098: e12fff1e bx lr
>
> The blx is wrong as we are jumping to an ARM function from ARM.
>
> Now if you change b.S like this:
>
> .text
> .align 2
> +.type foo, %function
> .global foo
> foo:
> push {r7}
>
>
> And compile it again in the same way you will see:
> 00008094 <call_foo>:
> 8094: eb000006 bl 80b4 <foo>
> 8098: e12fff1e bx lr
>
> Please note that the branch to foo is correct now.
>
> I hope this convinces you that %function indeed has an effect.
>
> I will get back with more details on the Linaro GCC 2012.01 later.

I meant "the Linaro GCC 2012.01 tool-chain problem"

This is a different problem. Some of the .rodata symbols are given an 
odd address although they should be aligned to at least 2-byte boundary 
). In fact the data is actually put at the even address but the symbol's 
value is +1 of the actual address. This is the ARM convention for Thumb 
functions, but they have applied it here for data too. That's the 
problem. I see that this doesn't happen to all the .rodata in SPL. 
Neither could I reproduce it with a small program. But the workaround 
for this problem is to avoid -fdata-sections. The following patch works
around it.

diff --git a/config.mk b/config.mk
index ddaa477..723286a 100644
--- a/config.mk
+++ b/config.mk
@@ -190,7 +190,7 @@ CPPFLAGS := $(DBGFLAGS) $(OPTFLAGS) $(RELFLAGS)		\

  # Enable garbage collection of un-used sections for SPL
  ifeq ($(CONFIG_SPL_BUILD),y)
-CPPFLAGS += -ffunction-sections -fdata-sections
+CPPFLAGS += -ffunction-sections
  LDFLAGS_FINAL += --gc-sections
  endif

Will you take a patch to make -fdata-sections optional, that is, having
it under something like CONFIG_SYS_SPL_NO_FDATA_SECTIONS?

br,
Aneesh