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

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@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:
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

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@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,

On Saturday 18 February 2012 10:18 PM, Albert ARIBAUD wrote:
> Hi Aneesh,
>

[...]

>>> 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?

I tried investigating a bit. As I mentioned earlier it doesn't happen
with some other files that use the same compiler and linker commands.
So, I don't know what's going on. Also, I couldn't reproduce it with a
simple program unlike in the other cases. Anyway, I have notified
tool-chain folks at Linaro:

http://article.gmane.org/gmane.linux.linaro.toolchain/2096

br,
Aneesh

On Monday 20 February 2012 09:38 PM, Aneesh V wrote:
> On Saturday 18 February 2012 10:18 PM, Albert ARIBAUD wrote:
>> Hi Aneesh,
>>
>
> [...]
>
>>>> 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?
>
> I tried investigating a bit. As I mentioned earlier it doesn't happen
> with some other files that use the same compiler and linker commands.
> So, I don't know what's going on. Also, I couldn't reproduce it with a
> simple program unlike in the other cases. Anyway, I have notified
> tool-chain folks at Linaro:
>
> http://article.gmane.org/gmane.linux.linaro.toolchain/2096

Ulrich Weigand has identified the root-cause [1]. The problem was due
to __attribute__ ((packed)) used in a structure. Let me quote him:

**********************************************************************
"I can reproduce the odd addresses of .rodata symbols.  However, this
occurs simply because the linker put *no* alignment requirement whatsoever
on those sections:

Section Headers:
   [Nr] Name              Type            Addr     Off    Size   ES Flg Lk
Inf Al
[snip]
   [11] .rodata.wkup_padc PROGBITS        00000000 000100 000004 00   A  0
0  1
   [12] .rodata.wkup_padc PROGBITS        00000000 000104 000048 00   A  0
0  1
   [13] .rodata.wkup_padc PROGBITS        00000000 00014c 00000c 00   A  0
0  1
   [14] .rodata.wkup_padc PROGBITS        00000000 000158 000004 00   A  0
0  1

Note the "Al" column values of 1.  In the final executable, those sections
happen to end up immediately following a .rodata.str string section with
odd
lenght, and since they don't have any alignment requirement, they start out
at an odd address.

The reason for the lack of alignment requirement is actually in the source:

const struct pad_conf_entry core_padconf_array_essential[] = {

where

struct pad_conf_entry {

         u16 offset;

         u16 val;

} __attribute__ ((packed));

The "packed" attribute specifies that all struct elements ought to be
considered to have alignment requirement 1 instead of their default
alignment.  Thus the whole struct ends up having alignment requirement 1;
and since the section contains only a single variable of such struct
type, this is then also the alignment requirement of the section."
**********************************************************************


I tried removing "packed" and it works. I will send an updated series
with this fix.

br,
Aneesh


[1] http://article.gmane.org/gmane.linux.linaro.toolchain/2099