Re: [U-Boot] [PATCH v2 1/2] arm920t/at91/reset.c: fix weak reset_board()
Dear all, Am 04.11.2010 02:23, schrieb Joakim Tjernlund: Graeme Russ graeme.r...@gmail.com wrote on 2010/11/04 02:13:44: On Thu, Nov 4, 2010 at 12:00 PM, Joakim Tjernlund joakim.tjernl...@transmode.se wrote: The arm920t compiler/linker dif not handle weak functions correctely. Therefore the linker tried to link outside the ELF (isn't that lazy linking?). This leads to segfault of linker in the end. This patch adds a empty stub for weak function reset_board() to catch that case. I believe this is the wrong approach. Instead you should fix the relocation/fixup routine not to relocate NULL ptrs. NULL ptrs are absolute values and should be left as is. This is ok and should be fixed in reloc routine. But it seems these undefined weak functions are _not_ null for me (have to check this ...). I personally think weak functions are 'cleaner' but may result in slightly larger and slower code due to the call overhead (is the optimiser clever enough to strip these out if the stub function is empty?) No idea. It seems my linker do not strip these functions. As Sebastian pointed out the linker adds .plt sections for these undefined weak functions. Do we use the linker in right way? Would converting all instances of if (function()) to weak functions be such a bad thing? nope, but I still think the reloc routine(s) needs to be fixed. If reloc routine is defective, it will be fixed ... currently the link of weak functions is the bigger problem. Without the empty stub for reset_board() my linker segfaults. Can please one test this issue with e.g. ELDK or other toolchains? CS 2010-q1 is known to be defective too! To test the described issue use '[PATCH v3] arm920t: implement elf relocation' and build for at91rm9200ek. regards Andreas Bießmann ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] arm920t/at91/reset.c: fix weak reset_board()
Dear Sebastien Carlier, Am 04.11.2010 04:27, schrieb Sebastien Carlier: These weakly defined empty functions prevent the strong definition from being linked in. For example, libarm.a contains a weak symbol 'red_LED_on', which is expected to be defined (strongly) in the board library. Because archive libraries are being used, this fails (testing with binutils 2.20.1), and only the empty __red_LED_on stub is linked in; the red_LED_on definition in the board library is throw away. I have detected this issue yesterday evening too. This behavior is documented and it is the intended one; from http://www.sco.com/developers/gabi/latest/ch4.symtab.html: When the link editor searches archive libraries [see ``Archive File'' in Chapter 7], it extracts archive members that contain definitions of undefined global symbols. The member's definition may be either a global or a weak symbol. The link editor does not extract archive members to resolve undefined weak symbols. Unresolved weak symbols have a zero value. Empty weak definitions would have to be supplied to the linker only _after_ the strong definitions have been provided. So it would be a work around to change Makefile from: ---8--- __LIBS := $(subst $(obj),,$(LIBS)) $(subst $(obj),,$(LIBBOARD)) ---8--- to: ---8--- __LIBS := $(subst $(obj),,$(LIBBOARD)) $(subst $(obj),,$(LIBS)) ---8--- This could work cause in most cases the strong definitions are in $(LIBBOARD) and overload the weak functions in $(LIBS). But this is just a work around and will not fit any use-case of weak functions. The root cause seems to be a linker problem. But I dunno whether it is a mis-usage or a bug. Any gcc-guys here to comment? Leaving undefined weak symbols and testing for NULL-ity at call sites seems to be a more robust approach. Note that with some ld versions (at least with 2.20.1), ld creates PLT entries for undefined weak symbols and crashes when the PLT-related sections (.plt, .got.plt, and .rel.plt) are discarded... This seems to be the main issue here ... but how to get it solved? regards Andreas Bießmann ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] arm920t/at91/reset.c: fix weak reset_board()
On Thu, Nov 4, 2010 at 11:54 AM, Andreas Bießmann andreas.de...@googlemail.com wrote: ---8--- __LIBS := $(subst $(obj),,$(LIBBOARD)) $(subst $(obj),,$(LIBS)) ---8--- Still, why would the linker pull definitions from libboard.a? Library archive are only searched by the linker to resolved undefined definitions. Weak symbols do not count as undefined. I think the following should work: - In the u-boot code, forward declare (no 'weak' attribute) functions which are to have a default implementation. - Keep the linking order as it is now (main u-boot libs, then board lib), and append a library containing all the weak definitions. But this is just a work around and will not fit any use-case of weak functions. The root cause seems to be a linker problem. But I dunno whether it is a mis-usage or a bug. Any gcc-guys here to comment? I think the answer is that weak symbols are not meant to be used with library archives. They work well with dynamic libraries. Note that with some ld versions (at least with 2.20.1), ld creates PLT entries for undefined weak symbols and crashes when the PLT-related sections (.plt, .got.plt, and .rel.plt) are discarded... This seems to be the main issue here ... but how to get it solved? I think there is no way around including the PLT. If I understand well, weak symbols are meant to be resolved during dynamic linking, so the extra indirection going through the PLT is needed to alter the address of the weak symbols when dynamic objects are loaded. -- Sebastien ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2 1/2] arm920t/at91/reset.c: fix weak reset_board()
The arm920t compiler/linker dif not handle weak functions correctely.
Therefore the linker tried to link outside the ELF (isn't that lazy
linking?). This leads to segfault of linker in the end.
This patch adds a empty stub for weak function reset_board() to catch
that case.
Signed-off-by: Andreas Bießmann andreas.de...@googlemail.com
---
introduced in v2
arch/arm/cpu/arm920t/at91/reset.c | 12 +---
1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/arch/arm/cpu/arm920t/at91/reset.c
b/arch/arm/cpu/arm920t/at91/reset.c
index ce9c156..8c81030 100644
--- a/arch/arm/cpu/arm920t/at91/reset.c
+++ b/arch/arm/cpu/arm920t/at91/reset.c
@@ -35,7 +35,14 @@
#include asm/arch/hardware.h
#include asm/arch/at91_st.h
-void board_reset(void) __attribute__((__weak__));
+void __attribute__((weak)) board_reset(void)
+{
+ /*
+* do absolute nothing here
+* but to have a empty stub for weak function to satisfy the linker
+*/
+}
void reset_cpu(ulong ignored)
{
@@ -45,8 +52,7 @@ void reset_cpu(ulong ignored)
serial_exit();
#endif
- if (board_reset)
- board_reset();
+ board_reset();
/* Reset the cpu by setting up the watchdog timer */
writel(AT91_ST_WDMR_RSTEN | AT91_ST_WDMR_EXTEN | AT91_ST_WDMR_WDV(2),
--
1.7.3.2
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] arm920t/at91/reset.c: fix weak reset_board()
The arm920t compiler/linker dif not handle weak functions correctely. Therefore the linker tried to link outside the ELF (isn't that lazy linking?). This leads to segfault of linker in the end. This patch adds a empty stub for weak function reset_board() to catch that case. I believe this is the wrong approach. Instead you should fix the relocation/fixup routine not to relocate NULL ptrs. NULL ptrs are absolute values and should be left as is. See http://git.denx.de/?p=u-boot.git;a=commit;h=d1e0b10accdbac2e0a8b2cbf7c589645442f87c5 for a fix to ppc that went in recently Jocke ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] arm920t/at91/reset.c: fix weak reset_board()
On Thu, Nov 4, 2010 at 12:00 PM, Joakim Tjernlund joakim.tjernl...@transmode.se wrote: The arm920t compiler/linker dif not handle weak functions correctely. Therefore the linker tried to link outside the ELF (isn't that lazy linking?). This leads to segfault of linker in the end. This patch adds a empty stub for weak function reset_board() to catch that case. I believe this is the wrong approach. Instead you should fix the relocation/fixup routine not to relocate NULL ptrs. NULL ptrs are absolute values and should be left as is. See http://git.denx.de/?p=u-boot.git;a=commit;h=d1e0b10accdbac2e0a8b2cbf7c589645442f87c5 for a fix to ppc that went in recently I seem to recall a discussion some time ago regarding the use of the if (function()) construct versus calls to weakly defined empty functions but cannot remember the outcome. I personally think weak functions are 'cleaner' but may result in slightly larger and slower code due to the call overhead (is the optimiser clever enough to strip these out if the stub function is empty?) Would converting all instances of if (function()) to weak functions be such a bad thing? Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] arm920t/at91/reset.c: fix weak reset_board()
Graeme Russ graeme.r...@gmail.com wrote on 2010/11/04 02:13:44: On Thu, Nov 4, 2010 at 12:00 PM, Joakim Tjernlund joakim.tjernl...@transmode.se wrote: The arm920t compiler/linker dif not handle weak functions correctely. Therefore the linker tried to link outside the ELF (isn't that lazy linking?). This leads to segfault of linker in the end. This patch adds a empty stub for weak function reset_board() to catch that case. I believe this is the wrong approach. Instead you should fix the relocation/fixup routine not to relocate NULL ptrs. NULL ptrs are absolute values and should be left as is. See http://git.denx.de/?p=u-boot.git;a=commit;h=d1e0b10accdbac2e0a8b2cbf7c589645442f87c5 for a fix to ppc that went in recently I seem to recall a discussion some time ago regarding the use of the if (function()) construct versus calls to weakly defined empty functions but cannot remember the outcome. Me neither. I personally think weak functions are 'cleaner' but may result in slightly larger and slower code due to the call overhead (is the optimiser clever enough to strip these out if the stub function is empty?) No idea. Would converting all instances of if (function()) to weak functions be such a bad thing? nope, but I still think the reloc routine(s) needs to be fixed. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] arm920t/at91/reset.c: fix weak reset_board()
These weakly defined empty functions prevent the strong definition from being linked in. For example, libarm.a contains a weak symbol 'red_LED_on', which is expected to be defined (strongly) in the board library. Because archive libraries are being used, this fails (testing with binutils 2.20.1), and only the empty __red_LED_on stub is linked in; the red_LED_on definition in the board library is throw away. This behavior is documented and it is the intended one; from http://www.sco.com/developers/gabi/latest/ch4.symtab.html: When the link editor searches archive libraries [see ``Archive File'' in Chapter 7], it extracts archive members that contain definitions of undefined global symbols. The member's definition may be either a global or a weak symbol. The link editor does not extract archive members to resolve undefined weak symbols. Unresolved weak symbols have a zero value. Empty weak definitions would have to be supplied to the linker only _after_ the strong definitions have been provided. Leaving undefined weak symbols and testing for NULL-ity at call sites seems to be a more robust approach. Note that with some ld versions (at least with 2.20.1), ld creates PLT entries for undefined weak symbols and crashes when the PLT-related sections (.plt, .got.plt, and .rel.plt) are discarded... -- Sebastien Carlier ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
