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

[Aneesh V]

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)
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.

br,
Aneesh