[RFC PATCH 00/10] Improve ARM target's support for LLVM toolchain
Here is a series of patches aimed at improving support for the LLVM toolchain (clang, lld, and to a lesser extent, llvm-objcopy) when targeting ARM. This toolchain is a cross-compiler "by default" -- a user generally should not need to install anything more to target their board of choice if they already have Clang installed. Additionally, Clang has a few nice diagnostics that should help appreciably with code quality, if Clang starts to be used regularly by a few more U-Boot developers. ;)
Most of these patches are trivial and as such they should be pretty easy to review, but the later patches in the patchset start making some pretty big changes to the linker scripts. There are no behavioral changes with those (U-Boot should still function the same) but there is always the risk of compatibility with some third-party tool or loader being broken. Fortunately, I foresee any problems making themselves immediately apparent upon testing.
I have tested booting on sunxi/T113, and building for Raspberry Pi (32-bit and 64-bit). The remaining testing efforts should be focused on: - Exynos - EFI loader (esp. on Lenovo X13s, which Heinrich Schuchardt had mentioned in a previous commit as being fickle) - Zynq - Rockchip TPL - AArch64 SPL
I'm submitting this as an RFC since this doesn't completely guarantee LLVM toolchain compatibility yet; my focus here is mostly on ensuring that I haven't caused any regressions in GNU-land. Also, I haven't discussed most of these changes before doing them. Perhaps alternate approaches to some of these things can be proposed - I'm all ears.
Outstanding problems are: - LLD sometimes puts .hash outside of the image binary area, though this is flagged by binary_size_check - The main makefile uses --gap-fill=0xff, which is not supported in llvm-objcopy - llvm-objcopy also doesn't appear to speak S-Record; the u-boot.srec target has to be deleted manually - llvm-objcopy gets upset at some of the EFI code, since the EFI linker scripts preserve dynamic sections that llvm-objcopy doesn't want to strip off
Cheers, Sam
Sam Edwards (10): makefile: Fix symbol typo in binary_size_check arm: set alignment properly for asm funcs arm: exclude eabi_compat from LTO arm: add __aeabi_memclr in eabi_compat arm: add aligned-memory aliases to eabi_compat arm: discard .gnu.version* sections arm: efi_loader: discard hash, unwind information arm: efi_loader: move .dynamic out of .text in EFI arm: discard all .dyn* sections arm: migrate away from sections.c
Makefile | 2 +- arch/arm/cpu/armv8/spl_data.c | 4 +- arch/arm/cpu/armv8/u-boot-spl.lds | 26 ++---- arch/arm/cpu/armv8/u-boot.lds | 48 +++-------- arch/arm/cpu/u-boot.lds | 103 +++++++---------------- arch/arm/include/asm/linkage.h | 4 +- arch/arm/lib/Makefile | 3 +- arch/arm/lib/eabi_compat.c | 17 ++++ arch/arm/lib/elf_aarch64_efi.lds | 3 +- arch/arm/lib/elf_arm_efi.lds | 3 + arch/arm/lib/sections.c | 36 -------- arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 26 ++---- arch/arm/mach-zynq/u-boot.lds | 73 +++------------- 13 files changed, 96 insertions(+), 252 deletions(-) delete mode 100644 arch/arm/lib/sections.c
The start-of-image marker symbol is `__image_copy_start`; by searching for `_image_copy_start` instead, this check can accidentally match `_image_copy_start_ofs`.
Signed-off-by: Sam Edwards CFSworks@gmail.com Reviewed-by: Tom Rini trini@konsulko.com ---
Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile index a5ab5e3da9..19017cbfa0 100644 --- a/Makefile +++ b/Makefile @@ -1262,7 +1262,7 @@ OBJCOPYFLAGS_u-boot-nodtb.bin := -O binary \ binary_size_check: u-boot-nodtb.bin FORCE @file_size=$(shell wc -c u-boot-nodtb.bin | awk '{print $$1}') ; \ map_size=$(shell cat u-boot.map | \ - awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end = $$1} END {if (start != "" && end != "") print "ibase=16; " toupper(end) " - " toupper(start)}' \ + awk '/__image_copy_start/ {start = $$1} /_image_binary_end/ {end = $$1} END {if (start != "" && end != "") print "ibase=16; " toupper(end) " - " toupper(start)}' \ | sed 's/0X//g' \ | bc); \ if [ "" != "$$map_size" ]; then \
ARM requires a 4-byte alignment on all ARM code (though this requirement is relaxed to 2-byte for some THUMB code) and we should be explicit about that here.
GAS has its own fix for this[1] that forces proper alignment on any section containing assembled instructions, but this is not universal: Clang's and other gaslike assemblers lack this implicit alignment. Whether or not this is considered a bug in those assemblers, it is better to ask directly for what we want.
[1]: https://sourceware.org/bugzilla/show_bug.cgi?id=12931
Signed-off-by: Sam Edwards CFSworks@gmail.com ---
arch/arm/include/asm/linkage.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/include/asm/linkage.h b/arch/arm/include/asm/linkage.h index dbe4b4e31a..73bf25ba4e 100644 --- a/arch/arm/include/asm/linkage.h +++ b/arch/arm/include/asm/linkage.h @@ -1,7 +1,7 @@ #ifndef __ASM_LINKAGE_H #define __ASM_LINKAGE_H
-#define __ALIGN .align 0 -#define __ALIGN_STR ".align 0" +#define __ALIGN .p2align 2 +#define __ALIGN_STR ".p2align 2"
#endif
These symbols need to survive the IR-level dead function elimination pass, since nothing at the IR level is referencing them (calls to these are inserted later, at codegen time).
Signed-off-by: Sam Edwards CFSworks@gmail.com ---
arch/arm/lib/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile index 62cf80f373..d65dc33f5b 100644 --- a/arch/arm/lib/Makefile +++ b/arch/arm/lib/Makefile @@ -88,6 +88,7 @@ obj-$(CONFIG_DEBUG_LL) += debug.o # For EABI conformant tool chains, provide eabi_compat() ifneq (,$(findstring -mabi=aapcs-linux,$(PLATFORM_CPPFLAGS))) extra-y += eabi_compat.o +CFLAGS_REMOVE_eabi_compat.o := $(LTO_CFLAGS) endif
# some files can only build in ARM or THUMB2, not THUMB1
This is sometimes used by LLVM's code generator.
Signed-off-by: Sam Edwards CFSworks@gmail.com ---
arch/arm/lib/eabi_compat.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/arm/lib/eabi_compat.c b/arch/arm/lib/eabi_compat.c index f7029918d4..059ca07265 100644 --- a/arch/arm/lib/eabi_compat.c +++ b/arch/arm/lib/eabi_compat.c @@ -35,3 +35,8 @@ void __aeabi_memset(void *dest, size_t n, int c) { (void) memset(dest, c, n); } + +void __aeabi_memclr(void *dest, size_t n) +{ + (void)memset(dest, 0, n); +}
These are sometimes used by LLVM's code-generator, when it can guarantee that the memory buffer being passed is aligned on a (4- or 8-byte) boundary. They can safely be aliases to the unaligned versions.
Signed-off-by: Sam Edwards CFSworks@gmail.com ---
arch/arm/lib/eabi_compat.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/arch/arm/lib/eabi_compat.c b/arch/arm/lib/eabi_compat.c index 059ca07265..d00f68a3f6 100644 --- a/arch/arm/lib/eabi_compat.c +++ b/arch/arm/lib/eabi_compat.c @@ -31,12 +31,24 @@ void __aeabi_memcpy(void *dest, const void *src, size_t n) (void) memcpy(dest, src, n); }
+void __aeabi_memcpy4(void *dest, const void *src, size_t n) __alias(__aeabi_memcpy); + +void __aeabi_memcpy8(void *dest, const void *src, size_t n) __alias(__aeabi_memcpy); + void __aeabi_memset(void *dest, size_t n, int c) { (void) memset(dest, c, n); }
+void __aeabi_memset4(void *dest, size_t n, int c) __alias(__aeabi_memset); + +void __aeabi_memset8(void *dest, size_t n, int c) __alias(__aeabi_memset); + void __aeabi_memclr(void *dest, size_t n) { (void)memset(dest, 0, n); } + +void __aeabi_memclr4(void *dest, size_t n) __alias(__aeabi_memclr); + +void __aeabi_memclr8(void *dest, size_t n) __alias(__aeabi_memclr);
These are often a consequence of --pie, but they aren't actually used in the runtime relocation code. It is better to discard them than to aggregate them, because they tend to be of different types, and this upsets some linkers (e.g. LLD).
Signed-off-by: Sam Edwards CFSworks@gmail.com ---
arch/arm/cpu/u-boot.lds | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index fc4f63d834..8cdf08a730 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -229,6 +229,12 @@ SECTIONS KEEP(*(.__bss_end)); }
+ /* + * LLD's --pie may synthesize these sections, even if they are empty; + * discard them, for we do not need version symbols + */ + /DISCARD/ : { *(.gnu.version*) } + .dynsym _image_binary_end : { *(.dynsym) } .dynbss : { *(.dynbss) } .dynstr : { *(.dynstr*) }
LLD tends to put these at the very beginning of the file, only for the .text 0x0 directive to end up going backward and overlapping them, creating an error.
Since they don't appear to be used at runtime, just discard them.
Signed-off-by: Sam Edwards CFSworks@gmail.com ---
arch/arm/lib/elf_arm_efi.lds | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/arch/arm/lib/elf_arm_efi.lds b/arch/arm/lib/elf_arm_efi.lds index 767ebda635..0b6cc5bcb6 100644 --- a/arch/arm/lib/elf_arm_efi.lds +++ b/arch/arm/lib/elf_arm_efi.lds @@ -62,5 +62,8 @@ SECTIONS *(.dynstr) *(.note.gnu.build-id) *(.comment) + *(.hash) + *(.gnu.hash) + *(.ARM.exidx) } }
Hi Sam
On Sat, 20 May 2023 at 23:56, Sam Edwards cfsworks@gmail.com wrote:
LLD tends to put these at the very beginning of the file, only for the .text 0x0 directive to end up going backward and overlapping them, creating an error.
Since they don't appear to be used at runtime, just discard them.
Signed-off-by: Sam Edwards CFSworks@gmail.com
arch/arm/lib/elf_arm_efi.lds | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/arch/arm/lib/elf_arm_efi.lds b/arch/arm/lib/elf_arm_efi.lds index 767ebda635..0b6cc5bcb6 100644 --- a/arch/arm/lib/elf_arm_efi.lds +++ b/arch/arm/lib/elf_arm_efi.lds @@ -62,5 +62,8 @@ SECTIONS *(.dynstr) *(.note.gnu.build-id) *(.comment)
*(.hash)*(.gnu.hash)
The reason we end up with both hash and gnu.hash is because the hash style is set to 'both'. Should we perhaps use (and strip) only one of them?
Thanks /Ilias
*(.ARM.exidx) }}
2.39.2
Hi Ilias,
On 5/22/23 01:00, Ilias Apalodimas wrote:
The reason we end up with both hash and gnu.hash is because the hash style is set to 'both'. Should we perhaps use (and strip) only one of them?
If we do keep one, it should probably be .hash -- see commit b02bfc4dfc.
I admit I'm completely mystified as to why we need the hash tables at all. The ELF spec says those are just for the dynamic linker, but neither the EFI code nor the self-relocating thunk require it, and I don't know of any target where the u-boot ELF itself is the shipped binary. For all I know, there never was a need to include .hash and Albert's commit fixed whatever problem he was facing only accidentally. Do you have any insights?
LLD's --hash-style option doesn't appear to have a "none" option or I'd just be making use of that here.
Cheers, Sam
Hi Sam,
On Mon, 22 May 2023 at 22:25, Sam Edwards cfsworks@gmail.com wrote:
Hi Ilias,
On 5/22/23 01:00, Ilias Apalodimas wrote:
The reason we end up with both hash and gnu.hash is because the hash style is set to 'both'. Should we perhaps use (and strip) only one of them?
If we do keep one, it should probably be .hash -- see commit b02bfc4dfc.
I admit I'm completely mystified as to why we need the hash tables at all. The ELF spec says those are just for the dynamic linker, but neither the EFI code nor the self-relocating thunk require it, and I don't know of any target where the u-boot ELF itself is the shipped binary.
Me neither
For all I know, there never was a need to include .hash and Albert's commit fixed whatever problem he was facing only accidentally. Do you have any insights?
Unfortunately not. I just started looking up the linker scripts myself.
LLD's --hash-style option doesn't appear to have a "none" option or I'd just be making use of that here.
Indeed. I am fine with the patch regardless, switching the makefile to only produce one of them is a nit anyway, since we'll eventually get rid of them
Thanks /Ilias
Cheers, Sam
Am 22. Mai 2023 21:25:50 MESZ schrieb Sam Edwards cfsworks@gmail.com:
Hi Ilias,
On 5/22/23 01:00, Ilias Apalodimas wrote:
The reason we end up with both hash and gnu.hash is because the hash style is set to 'both'. Should we perhaps use (and strip) only one of them?
If we do keep one, it should probably be .hash -- see commit b02bfc4dfc.
I admit I'm completely mystified as to why we need the hash tables at all. The ELF spec says those are just for the dynamic linker, but neither the EFI code nor the self-relocating thunk require it, and I don't know of any target where the u-boot ELF itself is the shipped binary. For all I know, there never was a need to include .hash and Albert's commit fixed whatever problem he was facing only accidentally. Do you have any insights?
Ubuntu's and Debian's u-boot-qemu package ships uboot.elf for multiple architectures. Cf. https://packages.debian.org/bullseye/all/u-boot-qemu/filelist
You can pass uboot.elf as -kernel parameter to QEMU.
Best regards
Heinrich
LLD's --hash-style option doesn't appear to have a "none" option or I'd just be making use of that here.
Cheers, Sam
This is not proper: A .text section is SHT_PROGBITS, while the .dynamic section is SHT_DYNAMIC. Attempting to combine them like this creates a section type mismatch.
It does seem that GNU ld does not complain, but LLVM's lld considers this an error.
Signed-off-by: Sam Edwards CFSworks@gmail.com Cc: Heinrich Schuchardt heinrich.schuchardt@canonical.com ---
arch/arm/lib/elf_aarch64_efi.lds | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/arm/lib/elf_aarch64_efi.lds b/arch/arm/lib/elf_aarch64_efi.lds index 5dd9809169..986f13936d 100644 --- a/arch/arm/lib/elf_aarch64_efi.lds +++ b/arch/arm/lib/elf_aarch64_efi.lds @@ -24,10 +24,9 @@ SECTIONS *(.gnu.linkonce.t.*) *(.srodata) *(.rodata*) - . = ALIGN(16); - *(.dynamic); . = ALIGN(512); } + .dynamic : { *(.dynamic) } .rela.dyn : { *(.rela.dyn) } .rela.plt : { *(.rela.plt) } .rela.got : { *(.rela.got) }
Hi Sam,
On Sat, 20 May 2023 at 23:56, Sam Edwards cfsworks@gmail.com wrote:
This is not proper: A .text section is SHT_PROGBITS, while the .dynamic section is SHT_DYNAMIC. Attempting to combine them like this creates a section type mismatch.
It does seem that GNU ld does not complain, but LLVM's lld considers this an error.
Signed-off-by: Sam Edwards CFSworks@gmail.com Cc: Heinrich Schuchardt heinrich.schuchardt@canonical.com
arch/arm/lib/elf_aarch64_efi.lds | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/arm/lib/elf_aarch64_efi.lds b/arch/arm/lib/elf_aarch64_efi.lds index 5dd9809169..986f13936d 100644 --- a/arch/arm/lib/elf_aarch64_efi.lds +++ b/arch/arm/lib/elf_aarch64_efi.lds @@ -24,10 +24,9 @@ SECTIONS *(.gnu.linkonce.t.*) *(.srodata) *(.rodata*)
. = ALIGN(16);*(.dynamic); . = ALIGN(512); }
.dynamic : { *(.dynamic) }
This looks correct. However, this changed recently on commit d7ddeb66a6ce ("efi_loader: fix building aarch64 EFI binaries").
Heinrich any chance you can repeat your tests?
Thanks /Ilias
.rela.dyn : { *(.rela.dyn) } .rela.plt : { *(.rela.plt) } .rela.got : { *(.rela.got) }-- 2.39.2
On 5/20/23 22:55, Sam Edwards wrote:
This is not proper: A .text section is SHT_PROGBITS, while the .dynamic section is SHT_DYNAMIC. Attempting to combine them like this creates a section type mismatch.
It does seem that GNU ld does not complain, but LLVM's lld considers this an error.
Signed-off-by: Sam Edwards CFSworks@gmail.com Cc: Heinrich Schuchardt heinrich.schuchardt@canonical.com
arch/arm/lib/elf_aarch64_efi.lds | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/arm/lib/elf_aarch64_efi.lds b/arch/arm/lib/elf_aarch64_efi.lds index 5dd9809169..986f13936d 100644 --- a/arch/arm/lib/elf_aarch64_efi.lds +++ b/arch/arm/lib/elf_aarch64_efi.lds @@ -24,10 +24,9 @@ SECTIONS *(.gnu.linkonce.t.*) *(.srodata) *(.rodata*)
. = ALIGN(16);
.dynamic should be aligned. Structure Elf64_Dyn requires at least 8 byte alignment.
*(.dynamic);
The symbol _etext below should be 512 aligned as in arch/arm/lib/crt0_aarch64_efi.S we have set FileAlignment = 0x200.
Best regards
Heinrich
}
- .dynamic : { *(.dynamic) } .rela.dyn : { *(.rela.dyn) } .rela.plt : { *(.rela.plt) } .rela.got : { *(.rela.got) }
On 5/22/23 02:10, Heinrich Schuchardt wrote:
Hi Heinrich,
.dynamic should be aligned. Structure Elf64_Dyn requires at least 8 byte alignment.
As best as I can tell, linkers (certainly lld[1], apparently also GNU ld judging by its default linker scripts) themselves set the proper word alignment on the .dynamic section that they synthesize for their internal input object. That alignment requirement bubbles up to the output section, so explicit alignment here should not be necessary.
The symbol _etext below should be 512 aligned as in arch/arm/lib/crt0_aarch64_efi.S we have set FileAlignment = 0x200.
Ah, that definitely needs to be fixed since the .rela.* sections might not have the 512 alignment. I've updated my local branch, though this also needs to be addressed in the current master branch.
Cheers, Sam
[1]: https://github.com/llvm/llvm-project/blob/acce2a315945e386a7be6f014ebe90c2a2...
This makes the output ELF friendlier to llvm-objcopy, which otherwise complains about discarding .dynsym when it's (potentially) referenced by .rel.dyn.
Since u-boot doesn't actually make use of a dynamic linker, and these sections are only here because we're building --pie, it's best to discard what we don't need.
Signed-off-by: Sam Edwards CFSworks@gmail.com ---
arch/arm/cpu/u-boot.lds | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index 8cdf08a730..bd4650bd86 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -235,10 +235,13 @@ SECTIONS */ /DISCARD/ : { *(.gnu.version*) }
- .dynsym _image_binary_end : { *(.dynsym) } - .dynbss : { *(.dynbss) } - .dynstr : { *(.dynstr*) } - .dynamic : { *(.dynamic*) } + /* + * Not needed at runtime: relocations are done directly on .rel.dyn, + * and we're self-relocating so we don't need compatibility with an + * external dynamic linker. + */ + /DISCARD/ : { *(.dyn*) } + .plt : { *(.plt*) } .interp : { *(.interp*) } .gnu.hash : { *(.gnu.hash) }
This patch effectively reverts 3ebd1cbc49f0005092d69cf0d9a6e64d7a1c300b.
The approach taken in that commit was to have the section-marking symbols generated into empty sections by the compiler, for the linker script to include at the correct location. The rationale was that at the time, the linker considered linker-assigned symbols to be dynamic when they were in PIC (PIEs or shared libraries), which meant they were represented at runtime by a R_ARM_ABS32 relocation (by symbol name) rather than by M_ARM_RELATIVE.
That commit landed in March 2013, but GNU ld later changed its behavior on 2016-02-23 to default linker-assigned symbols to dynamic only in shared libraries (not PIE), so this approach is unnecessary.
I am removing it, because: 1) It required keeping sections.c in sync with multiple linker scripts. 2) It added complexity to the linker scripts, making them less readable. 3) It added unnecessary sections to the output, which can't be merged because the sections are sometimes of different types. 4) The linker may insert sections not explicitly named in the script somewhere between explicit sections; having the marker symbols outside of the sections they were marking meant the markers could end up with an unintended section inserted within that region.
Signed-off-by: Sam Edwards CFSworks@gmail.com Cc: Albert ARIBAUD albert.u.boot@aribaud.net
---
arch/arm/cpu/armv8/spl_data.c | 4 +- arch/arm/cpu/armv8/u-boot-spl.lds | 26 +++---- arch/arm/cpu/armv8/u-boot.lds | 48 ++++--------- arch/arm/cpu/u-boot.lds | 88 +++++------------------- arch/arm/lib/Makefile | 2 - arch/arm/lib/sections.c | 36 ---------- arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 26 ++----- arch/arm/mach-zynq/u-boot.lds | 73 ++++---------------- 8 files changed, 59 insertions(+), 244 deletions(-) delete mode 100644 arch/arm/lib/sections.c
diff --git a/arch/arm/cpu/armv8/spl_data.c b/arch/arm/cpu/armv8/spl_data.c index 8f1231c86e..017b117183 100644 --- a/arch/arm/cpu/armv8/spl_data.c +++ b/arch/arm/cpu/armv8/spl_data.c @@ -6,8 +6,8 @@ #include <common.h> #include <spl.h>
-char __data_save_start[0] __section(".__data_save_start"); -char __data_save_end[0] __section(".__data_save_end"); +extern char __data_save_start[0]; +extern char __data_save_end[0];
u32 cold_reboot_flag = 1;
diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds index 7cb9d73124..137113a823 100644 --- a/arch/arm/cpu/armv8/u-boot-spl.lds +++ b/arch/arm/cpu/armv8/u-boot-spl.lds @@ -40,9 +40,9 @@ SECTIONS
#ifdef CONFIG_SPL_RECOVER_DATA_SECTION .data_save : { - *(.__data_save_start) + __data_save_start = .; . = SIZEOF(.data); - *(.__data_save_end) + __data_save_end = .; } >.sram #endif
@@ -51,30 +51,20 @@ SECTIONS KEEP(*(SORT(__u_boot_list*))); } >.sram
- .image_copy_end : { - . = ALIGN(8); - *(.__image_copy_end) - } >.sram + . = ALIGN(8);
- .end : { - . = ALIGN(8); - *(.__end) - } >.sram + __image_copy_end = .; + _end = .;
_image_binary_end = .;
- .bss_start (NOLOAD) : { - . = ALIGN(8); - KEEP(*(.__bss_start)); - } >.sdram + . = ALIGN(8);
.bss (NOLOAD) : { + __bss_start = .; *(.bss*) . = ALIGN(8); - } >.sdram - - .bss_end (NOLOAD) : { - KEEP(*(.__bss_end)); + __bss_end = .; } >.sdram
/DISCARD/ : { *(.rela*) } diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index fb6a30c922..c14c3365a8 100644 --- a/arch/arm/cpu/armv8/u-boot.lds +++ b/arch/arm/cpu/armv8/u-boot.lds @@ -23,7 +23,7 @@ SECTIONS . = ALIGN(8); .text : { - *(.__image_copy_start) + __image_copy_start = .; CPUDIR/start.o (.text*) }
@@ -42,13 +42,9 @@ SECTIONS }
#ifdef CONFIG_ARMV8_PSCI - .__secure_start : #ifndef CONFIG_ARMV8_SECURE_BASE - ALIGN(CONSTANT(COMMONPAGESIZE)) + . = ALIGN(CONSTANT(COMMONPAGESIZE)); #endif - { - KEEP(*(.__secure_start)) - }
#ifndef CONFIG_ARMV8_SECURE_BASE #define __ARMV8_SECURE_BASE @@ -57,8 +53,8 @@ SECTIONS #define __ARMV8_SECURE_BASE CONFIG_ARMV8_SECURE_BASE #endif .secure_text __ARMV8_SECURE_BASE : - AT(ADDR(.__secure_start) + SIZEOF(.__secure_start)) { + __secure_start = .; *(._secure.text) . = ALIGN(8); __secure_svc_tbl_start = .; @@ -79,23 +75,20 @@ SECTIONS AT(LOADADDR(.secure_data) + SIZEOF(.secure_data)) #endif { - KEEP(*(.__secure_stack_start)) + __secure_stack_start = .;
. = . + CONFIG_ARMV8_PSCI_NR_CPUS * ARM_PSCI_STACK_SIZE;
. = ALIGN(CONSTANT(COMMONPAGESIZE));
- KEEP(*(.__secure_stack_end)) + __secure_stack_end = .; }
#ifndef __ARMV8_PSCI_STACK_IN_RAM . = LOADADDR(.secure_stack); #endif
- .__secure_end : AT(ADDR(.__secure_end)) { - KEEP(*(.__secure_end)) - LONG(0x1d1071c); /* Must output something to reset LMA */ - } + __secure_end = .; #endif
. = ALIGN(8); @@ -126,42 +119,25 @@ SECTIONS
. = ALIGN(8);
- .image_copy_end : - { - *(.__image_copy_end) - } + __image_copy_end = .;
. = ALIGN(8);
- .rel_dyn_start : - { - *(.__rel_dyn_start) - } - .rela.dyn : { + __rel_dyn_start = .; *(.rela*) - } - - .rel_dyn_end : - { - *(.__rel_dyn_end) + __rel_dyn_end = .; }
_end = .;
. = ALIGN(8);
- .bss_start : { - KEEP(*(.__bss_start)); - } - .bss : { + __bss_start = .; *(.bss*) - . = ALIGN(8); - } - - .bss_end : { - KEEP(*(.__bss_end)); + . = ALIGN(8); + __bss_end = .; }
/DISCARD/ : { *(.dynsym) } diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index bd4650bd86..f0e58f4d72 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -40,24 +40,18 @@ SECTIONS . = ALIGN(4); .text : { - *(.__image_copy_start) + __image_copy_start = .; *(.vectors) CPUDIR/start.o (.text*) }
/* This needs to come before *(.text*) */ - .__efi_runtime_start : { - *(.__efi_runtime_start) - } - .efi_runtime : { + __efi_runtime_start = .; *(.text.efi_runtime*) *(.rodata.efi_runtime*) *(.data.efi_runtime*) - } - - .__efi_runtime_stop : { - *(.__efi_runtime_stop) + __efi_runtime_stop = .; }
.text_rest : @@ -68,13 +62,9 @@ SECTIONS #ifdef CONFIG_ARMV7_NONSEC
/* Align the secure section only if we're going to use it in situ */ - .__secure_start #ifndef CONFIG_ARMV7_SECURE_BASE - ALIGN(CONSTANT(COMMONPAGESIZE)) + . = ALIGN(CONSTANT(COMMONPAGESIZE)); #endif - : { - KEEP(*(.__secure_start)) - }
#ifndef CONFIG_ARMV7_SECURE_BASE #define __ARMV7_SECURE_BASE @@ -84,8 +74,8 @@ SECTIONS #endif
.secure_text __ARMV7_SECURE_BASE : - AT(ADDR(.__secure_start) + SIZEOF(.__secure_start)) { + __secure_start = .; *(._secure.text) }
@@ -103,7 +93,7 @@ SECTIONS AT(LOADADDR(.secure_data) + SIZEOF(.secure_data)) #endif { - KEEP(*(.__secure_stack_start)) + __secure_stack_start = .;
/* Skip addreses for stack */ . = . + CONFIG_ARMV7_PSCI_NR_CPUS * ARM_PSCI_STACK_SIZE; @@ -111,7 +101,7 @@ SECTIONS /* Align end of stack section to page boundary */ . = ALIGN(CONSTANT(COMMONPAGESIZE));
- KEEP(*(.__secure_stack_end)) + __secure_stack_end = .;
#ifdef CONFIG_ARMV7_SECURE_MAX_SIZE /* @@ -133,7 +123,7 @@ SECTIONS #endif
.__secure_end : AT(ADDR(.__secure_end)) { - *(.__secure_end) + __secure_end = .; LONG(0x1d1071c); /* Must output something to reset LMA */ } #endif @@ -146,58 +136,29 @@ SECTIONS *(.data*) }
- . = ALIGN(4); - - . = .; - . = ALIGN(4); __u_boot_list : { KEEP(*(SORT(__u_boot_list*))); }
. = ALIGN(4); - - .efi_runtime_rel_start : - { - *(.__efi_runtime_rel_start) - } - .efi_runtime_rel : { + __efi_runtime_rel_start = .; *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) - } - - .efi_runtime_rel_stop : - { - *(.__efi_runtime_rel_stop) + __efi_runtime_rel_stop = .; }
. = ALIGN(4); - - .image_copy_end : - { - *(.__image_copy_end) - } - - .rel_dyn_start : - { - *(.__rel_dyn_start) - } + __image_copy_end = .;
.rel.dyn : { + __rel_dyn_start = .; *(.rel*) + __rel_dyn_end = .; }
- .rel_dyn_end : - { - *(.__rel_dyn_end) - } - - .end : - { - *(.__end) - } - + _end = .; _image_binary_end = .;
/* @@ -209,24 +170,11 @@ SECTIONS *(.mmutable) }
-/* - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c - * __bss_base and __bss_limit are for linker only (overlay ordering) - */ - - .bss_start __rel_dyn_start (OVERLAY) : { - KEEP(*(.__bss_start)); - __bss_base = .; - } - - .bss __bss_base (OVERLAY) : { + .bss : { + __bss_start = .; *(.bss*) - . = ALIGN(4); - __bss_limit = .; - } - - .bss_end __bss_limit (OVERLAY) : { - KEEP(*(.__bss_end)); + . = ALIGN(4); + __bss_end = .; }
/* diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile index d65dc33f5b..c77e2c1f4b 100644 --- a/arch/arm/lib/Makefile +++ b/arch/arm/lib/Makefile @@ -57,8 +57,6 @@ endif
# obj-$(CONFIG_SAVE_PREV_BL_INITRAMFS_START_ADDR) += save_prev_bl_data.o obj-y += bdinfo.o -obj-y += sections.o -CFLAGS_REMOVE_sections.o := $(LTO_CFLAGS)
obj-y += stack.o ifdef CONFIG_CPU_V7M diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c deleted file mode 100644 index 857879711c..0000000000 --- a/arch/arm/lib/sections.c +++ /dev/null @@ -1,36 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/* - * Copyright 2013 Albert ARIBAUD albert.u.boot@aribaud.net - */ -#include <linux/compiler.h> - -/** - * These two symbols are declared in a C file so that the linker - * uses R_ARM_RELATIVE relocation, rather than the R_ARM_ABS32 one - * it would use if the symbols were defined in the linker file. - * Using only R_ARM_RELATIVE relocation ensures that references to - * the symbols are correct after as well as before relocation. - * - * We need a 0-byte-size type for these symbols, and the compiler - * does not allow defining objects of C type 'void'. Using an empty - * struct is allowed by the compiler, but causes gcc versions 4.4 and - * below to complain about aliasing. Therefore we use the next best - * thing: zero-sized arrays, which are both 0-byte-size and exempt from - * aliasing warnings. - */ - -char __bss_start[0] __section(".__bss_start"); -char __bss_end[0] __section(".__bss_end"); -char __image_copy_start[0] __section(".__image_copy_start"); -char __image_copy_end[0] __section(".__image_copy_end"); -char __rel_dyn_start[0] __section(".__rel_dyn_start"); -char __rel_dyn_end[0] __section(".__rel_dyn_end"); -char __secure_start[0] __section(".__secure_start"); -char __secure_end[0] __section(".__secure_end"); -char __secure_stack_start[0] __section(".__secure_stack_start"); -char __secure_stack_end[0] __section(".__secure_stack_end"); -char __efi_runtime_start[0] __section(".__efi_runtime_start"); -char __efi_runtime_stop[0] __section(".__efi_runtime_stop"); -char __efi_runtime_rel_start[0] __section(".__efi_runtime_rel_start"); -char __efi_runtime_rel_stop[0] __section(".__efi_runtime_rel_stop"); -char _end[0] __section(".__end"); diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds index 74618eba59..0cd6730533 100644 --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds @@ -24,7 +24,7 @@ SECTIONS
.text : { . = ALIGN(8); - *(.__image_copy_start) + __image_copy_start = .; CPUDIR/start.o (.text*) *(.text*) } @@ -44,30 +44,16 @@ SECTIONS KEEP(*(SORT(__u_boot_list*))); }
- .image_copy_end : { - . = ALIGN(8); - *(.__image_copy_end) - } - - .end : { - . = ALIGN(8); - *(.__end) - } + __image_copy_end = .; + _end = .;
_image_binary_end = .;
- .bss_start (NOLOAD) : { - . = ALIGN(8); - KEEP(*(.__bss_start)); - } - .bss (NOLOAD) : { + __bss_start = .; *(.bss*) - . = ALIGN(8); - } - - .bss_end (NOLOAD) : { - KEEP(*(.__bss_end)); + . = ALIGN(8); + __bss_end = .; }
/DISCARD/ : { *(.dynsym) } diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds index 3b7c9d515f..41d0b9471c 100644 --- a/arch/arm/mach-zynq/u-boot.lds +++ b/arch/arm/mach-zynq/u-boot.lds @@ -16,24 +16,18 @@ SECTIONS . = ALIGN(4); .text : { - *(.__image_copy_start) + __image_copy_start = .; *(.vectors) CPUDIR/start.o (.text*) }
/* This needs to come before *(.text*) */ - .__efi_runtime_start : { - *(.__efi_runtime_start) - } - .efi_runtime : { + __efi_runtime_start = .; *(.text.efi_runtime*) *(.rodata.efi_runtime*) *(.data.efi_runtime*) - } - - .__efi_runtime_stop : { - *(.__efi_runtime_stop) + __efi_runtime_stop = .; }
.text_rest : @@ -49,77 +43,36 @@ SECTIONS *(.data*) }
- . = ALIGN(4); - - . = .; - . = ALIGN(4); __u_boot_list : { KEEP(*(SORT(__u_boot_list*))); }
. = ALIGN(4); - - .efi_runtime_rel_start : - { - *(.__efi_runtime_rel_start) - } - .efi_runtime_rel : { + __efi_runtime_rel_start = .; *(.rel*.efi_runtime) *(.rel*.efi_runtime.*) - } - - .efi_runtime_rel_stop : - { - *(.__efi_runtime_rel_stop) + __efi_runtime_rel_stop = .; }
. = ALIGN(8); - .image_copy_end : - { - *(.__image_copy_end) - } - - .rel_dyn_start : - { - *(.__rel_dyn_start) - } + __image_copy_end = .;
.rel.dyn : { + __rel_dyn_start = .; *(.rel*) + __rel_dyn_end = .; }
- .rel_dyn_end : - { - *(.__rel_dyn_end) - } - - .end : - { - *(.__end) - } - + _end = .; _image_binary_end = .;
-/* - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c - * __bss_base and __bss_limit are for linker only (overlay ordering) - */ - - .bss_start __rel_dyn_start (OVERLAY) : { - KEEP(*(.__bss_start)); - __bss_base = .; - } - - .bss __bss_base (OVERLAY) : { + .bss : { + __bss_start = .; *(.bss*) - . = ALIGN(8); - __bss_limit = .; - } - - .bss_end __bss_limit (OVERLAY) : { - KEEP(*(.__bss_end)); + . = ALIGN(8); + __bss_end = .; }
/*
On Sat, May 20, 2023 at 02:55:47PM -0600, Sam Edwards wrote:
This patch effectively reverts 3ebd1cbc49f0005092d69cf0d9a6e64d7a1c300b.
Also 47bd65ef057fb71b02b32741d5cfcaf03e2f0918 ?
The approach taken in that commit was to have the section-marking symbols generated into empty sections by the compiler, for the linker script to include at the correct location. The rationale was that at the time, the linker considered linker-assigned symbols to be dynamic when they were in PIC (PIEs or shared libraries), which meant they were represented at runtime by a R_ARM_ABS32 relocation (by symbol name) rather than by M_ARM_RELATIVE.
That commit landed in March 2013, but GNU ld later changed its behavior on 2016-02-23 to default linker-assigned symbols to dynamic only in shared libraries (not PIE), so this approach is unnecessary.
I am removing it, because:
- It required keeping sections.c in sync with multiple linker scripts.
- It added complexity to the linker scripts, making them less readable.
- It added unnecessary sections to the output, which can't be merged because the sections are sometimes of different types.
- The linker may insert sections not explicitly named in the script somewhere between explicit sections; having the marker symbols outside of the sections they were marking meant the markers could end up with an unintended section inserted within that region.
Signed-off-by: Sam Edwards CFSworks@gmail.com Cc: Albert ARIBAUD albert.u.boot@aribaud.net
Thanks /Ilias
Am 20. Mai 2023 22:55:37 MESZ schrieb Sam Edwards cfsworks@gmail.com:
Here is a series of patches aimed at improving support for the LLVM toolchain (clang, lld, and to a lesser extent, llvm-objcopy) when targeting ARM. This toolchain is a cross-compiler "by default" -- a user generally should not need to install anything more to target their board of choice if they already have Clang installed. Additionally, Clang has a few nice diagnostics that should help appreciably with code quality, if Clang starts to be used regularly by a few more U-Boot developers. ;)
Most of these patches are trivial and as such they should be pretty easy to review, but the later patches in the patchset start making some pretty big changes to the linker scripts. There are no behavioral changes with those (U-Boot should still function the same) but there is always the risk of compatibility with some third-party tool or loader being broken. Fortunately, I foresee any problems making themselves immediately apparent upon testing.
I have tested booting on sunxi/T113, and building for Raspberry Pi (32-bit and 64-bit). The remaining testing efforts should be focused on:
- Exynos
- EFI loader (esp. on Lenovo X13s, which Heinrich Schuchardt had mentioned in a previous commit as being fickle)
- Zynq
- Rockchip TPL
- AArch64 SPL
I'm submitting this as an RFC since this doesn't completely guarantee LLVM toolchain compatibility yet; my focus here is mostly on ensuring that I haven't caused any regressions in GNU-land. Also, I haven't discussed most of these changes before doing them. Perhaps alternate approaches to some of these things can be proposed - I'm all ears.
Outstanding problems are:
- LLD sometimes puts .hash outside of the image binary area, though this
is flagged by binary_size_check
- The main makefile uses --gap-fill=0xff, which is not supported in
llvm-objcopy
- llvm-objcopy also doesn't appear to speak S-Record; the u-boot.srec
target has to be deleted manually
- llvm-objcopy gets upset at some of the EFI code, since the EFI linker
scripts preserve dynamic sections that llvm-objcopy doesn't want to strip off
Cheers, Sam
Hello Sam,
thanks for looking into llvm compatibility.
I guess the documentation and the CI testing would also have to be adjusted.
What about non-ARM architectures?
Could you, please, describe how built with lld so that reviewers can test it.
I find reviewing hard when receiving only 3 out of 10 patches.
Best regards
Heinrich
Sam Edwards (10): makefile: Fix symbol typo in binary_size_check arm: set alignment properly for asm funcs arm: exclude eabi_compat from LTO arm: add __aeabi_memclr in eabi_compat arm: add aligned-memory aliases to eabi_compat arm: discard .gnu.version* sections arm: efi_loader: discard hash, unwind information arm: efi_loader: move .dynamic out of .text in EFI arm: discard all .dyn* sections arm: migrate away from sections.c
Makefile | 2 +- arch/arm/cpu/armv8/spl_data.c | 4 +- arch/arm/cpu/armv8/u-boot-spl.lds | 26 ++---- arch/arm/cpu/armv8/u-boot.lds | 48 +++-------- arch/arm/cpu/u-boot.lds | 103 +++++++---------------- arch/arm/include/asm/linkage.h | 4 +- arch/arm/lib/Makefile | 3 +- arch/arm/lib/eabi_compat.c | 17 ++++ arch/arm/lib/elf_aarch64_efi.lds | 3 +- arch/arm/lib/elf_arm_efi.lds | 3 + arch/arm/lib/sections.c | 36 -------- arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 26 ++---- arch/arm/mach-zynq/u-boot.lds | 73 +++------------- 13 files changed, 96 insertions(+), 252 deletions(-) delete mode 100644 arch/arm/lib/sections.c
On 5/20/23 22:26, Heinrich Schuchardt wrote:
Hello Sam,
Hi Heinrich! Good to hear from you.
I guess the documentation and the CI testing would also have to be adjusted.
Ah, yeah, those are going to be big things for me to look at when this series starts to mature out of the RFC phase. CI is definitely important so that the hard-won compatibility doesn't just decay away. :)
What about non-ARM architectures?
If there's a groundswell of demand for building U-Boot on LLVM, I'd be willing to collaborate with others on getting the other architectures up to parity with GNU. But since the linker scripts, relocation thunks, sections, and whatnot are all arch-specific, I'm only focusing on ARM for now (which is both the arch I need and one of the more common ones).
Is there a particular arch you'd like to see next? It seems everything U-Boot supports is supported by LLVM, except for Microblaze, NIOS2, and SH.
Could you, please, describe how built with lld so that reviewers can test it.
I've been building with:
nice make CC='clang --target=armv7l-none-eabi' \ LTO_FINAL_LDFLAGS=-fuse-ld=lld LD=ld.lld OBJCOPY=llvm-objcopy
...though mostly at this stage I'm just hoping for folks to confirm that this patchset causes no regressions in their existing GNU environments. (Feedback from LLVM-land would be appreciated nonetheless, though!!!)
I find reviewing hard when receiving only 3 out of 10 patches.
Oof sorry about that. I haven't actually joined the U-Boot mailing list yet so the other patches are probably caught in the moderation queue. I guess wait for moderation approval and see if the other 7 arrive then -- I can resend the whole series to you specifically if not.
Cheers, Sam
Hi Sam,
On Sun, 21 May 2023 at 07:59, Sam Edwards cfsworks@gmail.com wrote:
On 5/20/23 22:26, Heinrich Schuchardt wrote:
Hello Sam,
Hi Heinrich! Good to hear from you.
I guess the documentation and the CI testing would also have to be adjusted.
Ah, yeah, those are going to be big things for me to look at when this series starts to mature out of the RFC phase. CI is definitely important so that the hard-won compatibility doesn't just decay away. :)
What about non-ARM architectures?
If there's a groundswell of demand for building U-Boot on LLVM, I'd be willing to collaborate with others on getting the other architectures up to parity with GNU. But since the linker scripts, relocation thunks, sections, and whatnot are all arch-specific, I'm only focusing on ARM for now (which is both the arch I need and one of the more common ones).
I can help clean up the arm architecture even further. I was toying with the idea of having page-aligned sections and eventually map u-boot with proper permissions per section. Right now (at least for the majority of arm platforms) we are doing RWX for all the memory, apart from devices that are mapped as RW. I do have an awfully hacky PoC around, but the linker script cleanup is more than welcome.
Is there a particular arch you'd like to see next? It seems everything U-Boot supports is supported by LLVM, except for Microblaze, NIOS2, and SH.
Could you, please, describe how built with lld so that reviewers can test it.
I've been building with:
nice make CC='clang --target=armv7l-none-eabi' \ LTO_FINAL_LDFLAGS=-fuse-ld=lld LD=ld.lld OBJCOPY=llvm-objcopy
...though mostly at this stage I'm just hoping for folks to confirm that this patchset causes no regressions in their existing GNU environments. (Feedback from LLVM-land would be appreciated nonetheless, though!!!)
I find reviewing hard when receiving only 3 out of 10 patches.
Oof sorry about that. I haven't actually joined the U-Boot mailing list yet so the other patches are probably caught in the moderation queue. I guess wait for moderation approval and see if the other 7 arrive then -- I can resend the whole series to you specifically if not.
It's probably not a mailing list issue. I only got the efi related patches on my mailbox. The recipients were generated with get_maintainers.pl? Heinirch and I only received the efi* portions as we maintain that subsystem
Thanks! /Ilias
Cheers, Sam
Hi Ilias,
On 5/22/23 00:52, Ilias Apalodimas wrote:
I can help clean up the arm architecture even further. I was toying with the idea of having page-aligned sections and eventually map u-boot with proper permissions per section. Right now (at least for the majority of arm platforms) we are doing RWX for all the memory, apart from devices that are mapped as RW. I do have an awfully hacky PoC around, but the linker script cleanup is more than welcome.
Glad to hear it (and excited by the idea of proper W^X)! The linker script cleanup (i.e. deleting those pesky `sections.c` files and going back to linker-assigned symbols) can really happen whenever; it won't cause a problem on any version of GNU ld from <7 years ago. Perhaps a series of patches (one per arch) doing that should be landed first?
It's probably not a mailing list issue. I only got the efi related patches on my mailbox. The recipients were generated with get_maintainers.pl? Heinirch and I only received the efi* portions as we maintain that subsystem
Well, it's true that you and Heinrich weren't Cc: on every email in the series. I just went with patman's default behavior.
But every patch was sent To: the u-boot list, and I do see the whole series showing up on the archive. Did you not even receive the other patches in the series via the list?
Cheers, Sam
Am 22. Mai 2023 21:37:26 MESZ schrieb Sam Edwards cfsworks@gmail.com:
Hi Ilias,
On 5/22/23 00:52, Ilias Apalodimas wrote:
I can help clean up the arm architecture even further. I was toying with the idea of having page-aligned sections and eventually map u-boot with proper permissions per section. Right now (at least for the majority of arm platforms) we are doing RWX for all the memory, apart from devices that are mapped as RW. I do have an awfully hacky PoC around, but the linker script cleanup is more than welcome.
Glad to hear it (and excited by the idea of proper W^X)! The linker script cleanup (i.e. deleting those pesky `sections.c` files and going back to linker-assigned symbols) can really happen whenever; it won't cause a problem on any version of GNU ld from <7 years ago. Perhaps a series of patches (one per arch) doing that should be landed first?
It's probably not a mailing list issue. I only got the efi related patches on my mailbox. The recipients were generated with get_maintainers.pl? Heinirch and I only received the efi* portions as we maintain that subsystem
Well, it's true that you and Heinrich weren't Cc: on every email in the series. I just went with patman's default behavior.
But every patch was sent To: the u-boot list, and I do see the whole series showing up on the archive. Did you not even receive the other patches in the series via the list?
The series can be retrieved from patchwork. I personally dislike patman for this eclectic behavior. Git send-email is doing the expected.
Best regards
Heinrich
Cheers, Sam
On Mon, May 22, 2023 at 01:37:26PM -0600, Sam Edwards wrote:
Hi Ilias,
On 5/22/23 00:52, Ilias Apalodimas wrote:
I can help clean up the arm architecture even further. I was toying with the idea of having page-aligned sections and eventually map u-boot with proper permissions per section. Right now (at least for the majority of arm platforms) we are doing RWX for all the memory, apart from devices that are mapped as RW. I do have an awfully hacky PoC around, but the linker script cleanup is more than welcome.
Glad to hear it (and excited by the idea of proper W^X)!
Yes that's my end goal here. Looking around the code we have in U-Boot regarding the mmu configuration, it's a lot easier (and cleaner) to fix the linker script, add symbols for ro_start, rw_start etc and layout the binary in a way we can easily map it, instead of leaving it as is and try to fix the mapping in c code.
The linker script cleanup (i.e. deleting those pesky `sections.c` files and going back to linker-assigned symbols) can really happen whenever; it won't cause a problem on any version of GNU ld from <7 years ago. Perhaps a series of patches (one per arch) doing that should be landed first?
Yes probably, because I specifically remember digging through the history of why the sections were defined like that in the first place.
It's probably not a mailing list issue. I only got the efi related patches on my mailbox. The recipients were generated with get_maintainers.pl? Heinirch and I only received the efi* portions as we maintain that subsystem
Well, it's true that you and Heinrich weren't Cc: on every email in the series. I just went with patman's default behavior.
But every patch was sent To: the u-boot list, and I do see the whole series showing up on the archive. Did you not even receive the other patches in the series via the list?
Cheers, Sam
Cheers /Ilias
Hi
On 5/21/23 06:59, Sam Edwards wrote:
On 5/20/23 22:26, Heinrich Schuchardt wrote:
Hello Sam,
Hi Heinrich! Good to hear from you.
I guess the documentation and the CI testing would also have to be adjusted.
Ah, yeah, those are going to be big things for me to look at when this series starts to mature out of the RFC phase. CI is definitely important so that the hard-won compatibility doesn't just decay away. :)
What about non-ARM architectures?
If there's a groundswell of demand for building U-Boot on LLVM, I'd be willing to collaborate with others on getting the other architectures up to parity with GNU. But since the linker scripts, relocation thunks, sections, and whatnot are all arch-specific, I'm only focusing on ARM for now (which is both the arch I need and one of the more common ones).
Is there a particular arch you'd like to see next? It seems everything U-Boot supports is supported by LLVM, except for Microblaze, NIOS2, and SH.
Could you, please, describe how built with lld so that reviewers can test it.
I've been building with:
nice make CC='clang --target=armv7l-none-eabi' \ LTO_FINAL_LDFLAGS=-fuse-ld=lld LD=ld.lld OBJCOPY=llvm-objcopy
...though mostly at this stage I'm just hoping for folks to confirm that this patchset causes no regressions in their existing GNU environments. (Feedback from LLVM-land would be appreciated nonetheless, though!!!)
Dockerfile in repo as I see is using 3 toolchain categories. 1. llvm deb repo 2. kernel.org 3. others - xtensa/arc
For CI loop you should pretty much provide a way how to get toolchain. That's why would be good to figure it out and then I am happy to take a look at changed you have done for Zynq. Definitely nice to see this happening and I expect more warnings will be visible and they should be fixed.
Thanks, Michal
On Mon, May 22, 2023 at 12:39:04PM +0200, Michal Simek wrote:
Hi
On 5/21/23 06:59, Sam Edwards wrote:
On 5/20/23 22:26, Heinrich Schuchardt wrote:
Hello Sam,
Hi Heinrich! Good to hear from you.
I guess the documentation and the CI testing would also have to be adjusted.
Ah, yeah, those are going to be big things for me to look at when this series starts to mature out of the RFC phase. CI is definitely important so that the hard-won compatibility doesn't just decay away. :)
What about non-ARM architectures?
If there's a groundswell of demand for building U-Boot on LLVM, I'd be willing to collaborate with others on getting the other architectures up to parity with GNU. But since the linker scripts, relocation thunks, sections, and whatnot are all arch-specific, I'm only focusing on ARM for now (which is both the arch I need and one of the more common ones).
Is there a particular arch you'd like to see next? It seems everything U-Boot supports is supported by LLVM, except for Microblaze, NIOS2, and SH.
Could you, please, describe how built with lld so that reviewers can test it.
I've been building with:
nice make CC='clang --target=armv7l-none-eabi' \ LTO_FINAL_LDFLAGS=-fuse-ld=lld LD=ld.lld OBJCOPY=llvm-objcopy
...though mostly at this stage I'm just hoping for folks to confirm that this patchset causes no regressions in their existing GNU environments. (Feedback from LLVM-land would be appreciated nonetheless, though!!!)
Dockerfile in repo as I see is using 3 toolchain categories.
- llvm deb repo
- kernel.org
- others - xtensa/arc
For CI loop you should pretty much provide a way how to get toolchain. That's why would be good to figure it out and then I am happy to take a look at changed you have done for Zynq. Definitely nice to see this happening and I expect more warnings will be visible and they should be fixed.
So, we can trivially add lld to the Dockerfile, it's just listing lld-16 in the install list. I think objcopy is a bit of a stretch at this point and it's not clear from the above if you're also making use of the assembler. We might also want to look at backporting scripts/Makefile.clang from the current kernel build system and then adapting the "guess the --target argument" logic based on CONFIG_$ARCH rather than ARCH= (which we don't use). That would also solve the LTO problem as that's a result of us missing some flags that the kernel has as LLVM+LTO (logically) requires LLVM LD not GNU LD.
At that point, and once the EFI guid_t warning is resolved to everyones satisfaction we can put qemu_arm* + clang in the CI loop, to catch new warnings there. I've already got clang + Pi in my CI loop, but that doesn't fail on warnings.
Hi Tom,
On 5/22/23 09:30, Tom Rini wrote:
I think objcopy is a bit of a stretch at this point and it's not clear from the above if you're also making use of the assembler.
I agree, since getting llvm-objcopy to play nice with this currently requires that I make a handful of small hack edits to the makefiles. It does offer a great advantage over GNU's objcopy in that it doesn't balk at ELFs from a foreign arch, but I'm only supporting it "opportunistically" right now.
I do make use of LLVM's assembler, but LLVM bundles its assembler inside the Clang binary (`clang -cc1as` and/or just pass .S files to Clang) rather than installing a separate program. This is not to be confused with `llvm-as` which is for bitcode/IR manipulation only. But in general, if you're using Clang, you're also using the LLVM assembler.
We might also want to look at backporting scripts/Makefile.clang from the current kernel build system and then adapting the "guess the --target argument" logic based on CONFIG_$ARCH rather than ARCH= (which we don't use). That would also solve the LTO problem as that's a result of us missing some flags that the kernel has as LLVM+LTO (logically) requires LLVM LD not GNU LD.
Having something like Linux's `LLVM=1` to enable LLVM would be ideal, I think. I probably won't be doing that backporting in this patch series since my goal for now is just to fill in some of the pitfalls for people like me who are too stubborn to install a GNU toolchain.
At that point, and once the EFI guid_t warning is resolved to everyones satisfaction we can put qemu_arm* + clang in the CI loop, to catch new warnings there. I've already got clang + Pi in my CI loop, but that doesn't fail on warnings.
Do you mean, on the current master branch, and only Clang (no LLD)? I'm in favor, but since a few of the patches in this series (#3-5) are to support some of the libcalls that LLVM's codegen likes to emit, I'd be surprised if that worked on all targets. Feel free to pull whatever necessary patches of mine here into your own series though. :)
Cheers, Sam
On Mon, May 22, 2023 at 01:59:33PM -0600, Sam Edwards wrote:
Hi Tom,
On 5/22/23 09:30, Tom Rini wrote:
I think objcopy is a bit of a stretch at this point and it's not clear from the above if you're also making use of the assembler.
I agree, since getting llvm-objcopy to play nice with this currently requires that I make a handful of small hack edits to the makefiles. It does offer a great advantage over GNU's objcopy in that it doesn't balk at ELFs from a foreign arch, but I'm only supporting it "opportunistically" right now.
I do make use of LLVM's assembler, but LLVM bundles its assembler inside the Clang binary (`clang -cc1as` and/or just pass .S files to Clang) rather than installing a separate program. This is not to be confused with `llvm-as` which is for bitcode/IR manipulation only. But in general, if you're using Clang, you're also using the LLVM assembler.
Ah, ok.
We might also want to look at backporting scripts/Makefile.clang from the current kernel build system and then adapting the "guess the --target argument" logic based on CONFIG_$ARCH rather than ARCH= (which we don't use). That would also solve the LTO problem as that's a result of us missing some flags that the kernel has as LLVM+LTO (logically) requires LLVM LD not GNU LD.
Having something like Linux's `LLVM=1` to enable LLVM would be ideal, I think. I probably won't be doing that backporting in this patch series since my goal for now is just to fill in some of the pitfalls for people like me who are too stubborn to install a GNU toolchain.
It honestly shouldn't be too much work to just backport that file (and move/remove some of the logic we have scattered elsewhere instead).
At that point, and once the EFI guid_t warning is resolved to everyones satisfaction we can put qemu_arm* + clang in the CI loop, to catch new warnings there. I've already got clang + Pi in my CI loop, but that doesn't fail on warnings.
Do you mean, on the current master branch, and only Clang (no LLD)? I'm in favor, but since a few of the patches in this series (#3-5) are to support some of the libcalls that LLVM's codegen likes to emit, I'd be surprised if that worked on all targets. Feel free to pull whatever necessary patches of mine here into your own series though. :)
The reason it's not in CI right now is that there's compiler warnings, due to the EFI guid_t alignment thing that's being discussed in another thread. I am running (and so booting and running pytest) in my CI loop clang-built U-Boot for Pi 3 (32bit and 64bit) and a few other targets too. But there's also certainly room to improve and I think some of the issues you've addressed here are why other targets for example are too large to boot.
participants (5)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Michal Simek -
Sam Edwards -
Tom Rini