Hi Ulrich,

On Tuesday 15 March 2011 09:00 PM, Ulrich Weigand wrote:

Aneesh Vaneesh@ti.com wrote:

...

I was trying to build u-boot in Thumb2 for OMAP4. Everything was fine until I added some patches recently. One of these patches introduced an API (let's say foo()) that has a weakly linked alias(let's say __foo()) and a strongly linked implementation(the real foo()) in an assembly file.

Although I give -mthumb and -mthumb-interwork for all the files, apparently GCC generates ARM code for assembly files. In the final image foobar() calls foo() using a BL. Since foobar() is in Thumb and foo() in ARM, it ends up crashing. Looks like foobar() assumed foo() to be Thumb because __foo() is Thumb.

I'm unable to reproduce this. Do you have a complete test case?

Thank you for looking into this problem.

I've tried with the following small example:

foo1.c:

extern void foo (void) __attribute__ ((weak, alias ("__foo")));

void __foo (void) { }

int main (void) { foo (); }

foo2.S: .text .align 2 .global foo .type foo, %function foo: push {r7} add r7, sp, #0 mov sp, r7 pop {r7} bx lr .size foo, .-foo

Can you try this sequence:

arm-linux-gnueabi-gcc -march=armv7-a -mthumb -mthumb-interwork -o foo1.o foo1.c -c arm-linux-gnueabi-gcc -march=armv7-a -mthumb -mthumb-interwork -o foo2.o foo2.S -c arm-linux-gnueabi-ld -r foo1.o foo2.o arm-linux-gnueabi-objdump -S a.out

With this sequence I get the following output:

00000000 <__foo>: 0: b480 push {r7} 2: af00 add r7, sp, #0 4: 46bd mov sp, r7 6: bc80 pop {r7} 8: 4770 bx lr a: bf00 nop

0000000c <main>: c: b580 push {r7, lr} e: af00 add r7, sp, #0 10: f7ff fffe bl 18 <foo> 14: 4618 mov r0, r3 16: bd80 pop {r7, pc}

00000018 <foo>: 18: e92d0080 push {r7} 1c: e28d7000 add r7, sp, #0 20: e1a0d007 mov sp, r7 24: e8bd0080 pop {r7} 28: e12fff1e bx lr

When building just "gcc foo1.c", I get:

0000835c<__foo>: 835c: b480 push {r7} 835e: af00 add r7, sp, #0 8360: 46bd mov sp, r7 8362: bc80 pop {r7} 8364: 4770 bx lr 8366: bf00 nop

00008368<main>: 8368: b580 push {r7, lr} 836a: af00 add r7, sp, #0 836c: f7ff fff6 bl 835c<__foo> 8370: 4618 mov r0, r3 8372: bd80 pop {r7, pc}

When building both files "gcc foo1.c foo2.S", I get instead:

00008368<main>: 8368: b580 push {r7, lr} 836a: af00 add r7, sp, #0 836c: f000 e802 blx 8374<foo> 8370: 4618 mov r0, r3 8372: bd80 pop {r7, pc}

00008374<foo>: 8374: e92d0080 push {r7} 8378: e28d7000 add r7, sp, #0 837c: e1a0d007 mov sp, r7 8380: e8bd0080 pop {r7} 8384: e12fff1e bx lr

So it seems to me the linker is handling this correctly ...

(This is on Ubuntu Natty using system gcc and binutils.)

Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

-- Dr. Ulrich Weigand | Phone: +49-7031/16-3727 STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E. IBM Deutschland Research& Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter | Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen | Registergericht: Amtsgericht Stuttgart, HRB 243294

Sorry fo the delay in responding. I couldn't find the time to do the experiments in the last couple of days.

On Wednesday 16 March 2011 07:14 PM, Ulrich Weigand wrote:

Aneesh Vaneesh@ti.com wrote on 03/16/2011 10:32:50 AM:

...

Can you try this sequence:

arm-linux-gnueabi-gcc -march=armv7-a -mthumb -mthumb-interwork -o foo1.o foo1.c -c arm-linux-gnueabi-gcc -march=armv7-a -mthumb -mthumb-interwork -o foo2.o foo2.S -c arm-linux-gnueabi-ld -r foo1.o foo2.o arm-linux-gnueabi-objdump -S a.out

With this sequence I get the following output:

...

0000000c<main>: c: b580 push {r7, lr} e: af00 add r7, sp, #0 10: f7ff fffe bl 18<foo> 14: 4618 mov r0, r3 16: bd80 pop {r7, pc}

00000018<foo>: 18: e92d0080 push {r7} 1c: e28d7000 add r7, sp, #0 20: e1a0d007 mov sp, r7 24: e8bd0080 pop {r7} 28: e12fff1e bx lr

Well, sure, but the result of "ld -r" is not a final executable, but just another relocatable object file. In particular, it will still carry relocation records. In fact, if you add --reloc to the objdump line, you should see:

Hmm. I was just trying to mimic the U-Boot link command there. Didn't really check what -r meant.

0000000c<main>: c: b580 push {r7, lr} e: af00 add r7, sp, #0 10: f7ff fffe bl 18<foo> 10: R_ARM_THM_CALL foo 14: 4618 mov r0, r3 16: bd80 pop {r7, pc}

The R_ARM_THM_CALL marks the branch instruction as still to be processed by the final link step. And once you actually *perform* the final link, e.g. via "gcc -o final a.out", the branch gets indeed converted to a blx by the linker for me:

This doesn't seem to be happening with U-boot. In U-boot, the situation is like this:

U-Boot builds multiple individual libraries and finally link them together. The call to the API and the weakly linked implementation is in one library(libarmv7.o), while the real implementation in assembly is in another library(libomap4.o). Here are the relevant contents of these libraries and the final image "u-boot" for your reference:

liarmv7.o: 000001b6 <arm_init_before_mmu>: }

void arm_init_before_mmu(void) { 1b6: b508 push {r3, lr} v7_outer_cache_enable(); 1b8: f7ff fffe bl 1f0 <__v7_outer_cache_enable> 1b8: R_ARM_THM_CALL v7_outer_cache_enable

libomap4.o: 0000111c <v7_outer_cache_enable>:

.globl v7_outer_cache_enable v7_outer_cache_enable: MOV r0, #1 111c: e3a00001 mov r0, #1 B set_pl310_ctrl_reg 1120: eafffff9 b 110c <set_pl310_ctrl_reg>

u-boot: void arm_init_before_mmu(void) { 80100636: b508 push {r3, lr} v7_outer_cache_enable(); 80100638: f001 f998 bl 8010196c <_end+0xfffaf2a4>

Here are the intermediate link commands used for the libraries: arm-linux-gnueabi-ld -r -o libarmv7.o cpu.o cache_v7.o syslib.o arm-linux-gnueabi-ld -r -o libomap4.o board.o mem.o sys_info.o ...

Here is the final link command(shortened and line-wrapped in the interest of readability - attaching the full build log): UNDEF_SYM=`arm-linux-gnueabi-objdump -x arch/arm/cpu/armv7/libarmv7.o arch/arm/cpu/armv7/omap4/libomap4.o arch/arm/lib/libarm.o | sed -n -e 's/.*(__u_boot_cmd_.*)/-u\1/p'|sort|uniq`; cd /home/a0393566local /u-boot-denx-bak && arm-linux-gnueabi-ld -pie -Bstatic -T u-boot.lds -Ttext 0x80100000 $UNDEF_SYM arch/arm/cpu/armv7/start.o --start-group arch/arm/cpu/armv7/libarmv7.o arch/arm/cpu/armv7/omap4/libomap4.o arch/arm/lib/libarm.o --end-group /home/a0393566local/u-boot-denx-bak /arch/arm/lib/eabi_compat.o -L /usr/lib/gcc/arm-linux-gnueabi/4.5.2 -lgcc -Map u-boot.map -o u-boot

I don't know what the UNDEF_SYM is about. Do you think that could be creating a problem?

This is now looking more like U-Boot build infrastructure issue. Maybe, I should take this up in the U-Boot mailing list now?

best regards, Aneesh

On Mon, Feb 20, 2012 at 06:59:53PM +0100, Ulrich Weigand wrote:

"V, Aneesh" aneesh@ti.com wrote:

...

I agree that not marking the assembly functions ' %function' is a problem in the code, so it's not a critical bug. But I would've been happier if the linker refused to link it rather than branching with the wrong instruction. Isn't that a problem?

Well, if the target symbol of a branch is not marked as %function, the linker has no way of knowing whether that target is ARM or Thumb, so it cannot specifically error out if (and only if) the instruction is wrong.

The linker *could* in theory give an error *unconditionally* whenever it detects a branch to a non-%function symbol. The reason this is not done is probably for backwards compatibility with old hand-written code, say from an ARM-only era: branches to non-function symbols used to be allowed, and in fact work fine if all code is ARM-only. Adding an error would break such old code.

...

Problem No:2

---

Linaro GCC 2012.01 is giving a new problem w.r.to Thumb build that is not existing in Sourcery G++ Lite 2010q1-202. However, I couldn't reproduce this problem with a small program like above. So, let me give you reference to the original u-boot code that shows the problem and steps to reproduce it.

[snip]

...

Please note that the .rodata symbols have odd addresses. These arrays actually need to be aligned at least to half-word boundary. In fact, in the image I verified that they are put at even addresses. So, the symbols have been kept as real address +1.

This seems strange. How did you verify "that they are put at even addresses"? 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.

Is "packed" even wanted here?

Based on a very brief skim of the code, it looks like the packed attribute is an unnecessary attempt to save some space -- unnecessary because all ARM ABI variants I know of (actually, all arches I know of) will pack that structure into a word anyway as a result of natural alignment of the members. It doesn't look to me like the packed structure is used to map a memory-mapped peripheral directly or otherwise communicate with the outside world -- which would be the only situations in which a packed structure would normally make sense.

Of course, I may have missed something...

Cheers ---Dave

Andrew Stubbs andrew.stubbs@linaro.org wrote on 21.02.2012 11:56:07:

I'm not sure, but I believe that the compiler requires (prefers) any structs that you want included inside packed structs to be themselves packed, so you can end up with some structs with apparently unnecessary attributes on them.

I don't see why the compiler would care. Its just that if an inner struct already has padding somewhere, this padding doesn't go away (the struct layout is not recomputed) just because it is embedded in a larger struct which is marked packed. [Marking the outer struct packed will still affect the overall alignment requirement of the inner struct; it just won't affect it inner layout.]

It might also have an effect when you place the struct inside an unpacked struct?

Yes, of course: if the struct is packed, then the struct as a whole has an alignment requirement of 1, which may affect the placement of the struct as an element within an outside (unpacked) struct.

Without context I've no idea whether that's what's going on here. Of course, a no-op "packed" attribute ought to be harmless...

It is not really "no-op": it reduces the alignment requirement of the struct from 2 (in this case) to 1, which is actually exactly what's causing the problems in the original test case.

Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

-- Dr. Ulrich Weigand | Phone: +49-7031/16-3727 STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E. IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz | Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen | Registergericht: Amtsgericht Stuttgart, HRB 243294

On Tuesday 21 February 2012 03:27 PM, Dave Martin wrote:

On Mon, Feb 20, 2012 at 06:59:53PM +0100, Ulrich Weigand wrote:

...

"V, Aneesh"aneesh@ti.com wrote:

...

I agree that not marking the assembly functions ' %function' is a problem in the code, so it's not a critical bug. But I would've been happier if the linker refused to link it rather than branching with the wrong instruction. Isn't that a problem?

Well, if the target symbol of a branch is not marked as %function, the linker has no way of knowing whether that target is ARM or Thumb, so it cannot specifically error out if (and only if) the instruction is wrong.

The linker *could* in theory give an error *unconditionally* whenever it detects a branch to a non-%function symbol. The reason this is not done is probably for backwards compatibility with old hand-written code, say from an ARM-only era: branches to non-function symbols used to be allowed, and in fact work fine if all code is ARM-only. Adding an error would break such old code.

...

Problem No:2

---

Linaro GCC 2012.01 is giving a new problem w.r.to Thumb build that is not existing in Sourcery G++ Lite 2010q1-202. However, I couldn't reproduce this problem with a small program like above. So, let me give you reference to the original u-boot code that shows the problem and steps to reproduce it.

[snip]

...

Please note that the .rodata symbols have odd addresses. These arrays actually need to be aligned at least to half-word boundary. In fact, in the image I verified that they are put at even addresses. So, the symbols have been kept as real address +1.

This seems strange. How did you verify "that they are put at even addresses"? 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.

Is "packed" even wanted here?

Based on a very brief skim of the code, it looks like the packed attribute is an unnecessary attempt to save some space -- unnecessary because all ARM ABI variants I know of (actually, all arches I know of) will pack that structure into a word anyway as a result of natural alignment of the members. It doesn't look to me like the packed structure is used to map a memory-mapped peripheral directly or otherwise communicate with the outside world -- which would be the only situations in which a packed structure would normally make sense.

Of course, I may have missed something...

No. I think packed is not needed here. Removing it didn't change the size of the arrays. I will remove it.

Thanks, Aneesh

On 23/02/12 10:27, Aneesh V wrote:

Ok. Agree. I never used to use %function when I wrote assembly functions earlier. I am sure a lot of code will break if this was enforced.

If you've not used %function on ARM, then your code is semantically broken even if it isn't syntactically broken.

The ABI rules for dealing with interworking and and out-of-range branches all rely on %function being used correctly.

R.