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

[Albert ARIBAUD]

Hi Aneesh,

Le 18/02/2012 17:34, Aneesh V a écrit :
> 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?

Hmm... considering you're seeing the issue in a fairly new toolchain 
release, I prefer notifying the toolchain makers, rather than removing 
the -fdata-sections from SPL even for specific boards. Can you go and 
see why the Linaro toolchain generated odd "thumb data" at all and get 
this fixed?

> br,
> Aneesh

Amicalement,
-- 
Albert.