diff mbox series

[2/7,v2] arm: clean up v7 and v8 linker scripts for bss_start/end

Message ID 20240312140832.1968215-3-ilias.apalodimas@linaro.org
State New
Headers show
Series Clean up arm linker scripts | expand

Commit Message

Ilias Apalodimas March 12, 2024, 2:08 p.m. UTC 
commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated")
and
commit f84a7b8f54db ("ARM: Fix __bss_start and __bss_end in linker scripts")
were moving the bss_start/end on c generated variables that were
injected in their own sections. The reason was that we needed relative
relocations for position independent code and linker bugs back then
prevented us from doing so [0].

However, the linker documentation pages states that symbols that are
defined within a section definition will create a relocatable type with
the value being a fixed offset from the base of a section [1].
So let's start cleaning this up starting with the bss_start and bss_end
variables. Convert them into symbols within the .bss section definition.

[0] binutils commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object")
[1] https://sourceware.org/binutils/docs/ld/Expression-Section.html

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Tested-by: Caleb Connolly <caleb.connolly@linaro.org> # Qualcomm sdm845
---
 arch/arm/cpu/armv8/u-boot-spl.lds        | 14 +++-----------
 arch/arm/cpu/armv8/u-boot.lds            | 15 +++------------
 arch/arm/cpu/u-boot.lds                  | 21 ++++++---------------
 arch/arm/lib/sections.c                  |  2 --
 arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 14 +++-----------
 arch/arm/mach-zynq/u-boot.lds            | 21 ++++++---------------
 6 files changed, 21 insertions(+), 66 deletions(-)

--
2.37.2

Comments

Richard Henderson March 12, 2024, 3:51 p.m. UTC | #1 
On 3/12/24 04:08, Ilias Apalodimas wrote:
> -	.bss_start (NOLOAD) : {
> -		. = ALIGN(8);

This alignment got lost.

> -		KEEP(*(.__bss_start));
> -	} >.sdram
> -
> -	.bss (NOLOAD) : {
> +	.bss : {

If it is required, the best replacement would be here on the output section definition.

> -	. = ALIGN(8);
> -
> -	.bss_start : {
> -		KEEP(*(.__bss_start));
> -	}
> -
> -	.bss : {
> +	.bss ALIGN(8): {

Like you did here.


r~
Ilias Apalodimas March 12, 2024, 5:31 p.m. UTC | #2 
Hi Richard,

Thanks for taking a look

On Tue, 12 Mar 2024 at 17:51, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/12/24 04:08, Ilias Apalodimas wrote:
> > -     .bss_start (NOLOAD) : {
> > -             . = ALIGN(8);
>
> This alignment got lost.
>
> > -             KEEP(*(.__bss_start));
> > -     } >.sdram
> > -
> > -     .bss (NOLOAD) : {
> > +     .bss : {
>
> If it is required, the best replacement would be here on the output section definition.

As you mentioned in patch #7 we should preserve some alignments or fix
the underlying assembly. But in any case, I'll preserve the .bss
alignment here since it will be easy to bisect/read the commit logs &
patches in the future

>
> > -     . = ALIGN(8);
> > -
> > -     .bss_start : {
> > -             KEEP(*(.__bss_start));
> > -     }
> > -
> > -     .bss : {
> > +     .bss ALIGN(8): {
>
> Like you did here.

Thanks!
/Ilias
>
>
> r~
diff mbox series

Patch

diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds
index 7cb9d731246d..16fddb46e9cb 100644
--- a/arch/arm/cpu/armv8/u-boot-spl.lds
+++ b/arch/arm/cpu/armv8/u-boot-spl.lds
@@ -63,18 +63,10 @@  SECTIONS

 	_image_binary_end = .;

-	.bss_start (NOLOAD) : {
-		. = ALIGN(8);
-		KEEP(*(.__bss_start));
-	} >.sdram
-
-	.bss (NOLOAD) : {
+	.bss : {
+		__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 fb6a30c922f7..c4ee10ebc3ff 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -149,19 +149,10 @@  SECTIONS

 	_end = .;

-	. = ALIGN(8);
-
-	.bss_start : {
-		KEEP(*(.__bss_start));
-	}
-
-	.bss : {
+	.bss ALIGN(8): {
+		__bss_start = .;
 		*(.bss*)
-		 . = ALIGN(8);
-	}
-
-	.bss_end : {
-		KEEP(*(.__bss_end));
+		__bss_end = .;
 	}

 	/DISCARD/ : { *(.dynsym) }
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 7724c9332c3b..01accb109fad 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -207,23 +207,14 @@  SECTIONS
 	}

 /*
- * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
- * __bss_base and __bss_limit are for linker only (overlay ordering)
+ * These sections occupy the same memory, but their lifetimes do
+ * not overlap: U-Boot initializes .bss only after applying dynamic
+ * relocations and therefore after it doesn't need .rel.dyn any more.
  */
-
-	.bss_start __rel_dyn_start (OVERLAY) : {
-		KEEP(*(.__bss_start));
-		__bss_base = .;
-	}
-
-	.bss __bss_base (OVERLAY) : {
+	.bss ADDR(.rel.dyn) (OVERLAY): {
+		__bss_start = .;
 		*(.bss*)
-		 . = ALIGN(4);
-		 __bss_limit = .;
-	}
-
-	.bss_end __bss_limit (OVERLAY) : {
-		KEEP(*(.__bss_end));
+		__bss_end = .;
 	}

 	.dynsym _image_binary_end : { *(.dynsym) }
diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
index 857879711c6a..8e8bd5797e16 100644
--- a/arch/arm/lib/sections.c
+++ b/arch/arm/lib/sections.c
@@ -19,8 +19,6 @@ 
  * 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");
diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
index 74618eba591b..b7887194026e 100644
--- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
+++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds
@@ -56,18 +56,10 @@  SECTIONS

 	_image_binary_end = .;

-	.bss_start (NOLOAD) : {
-		. = ALIGN(8);
-		KEEP(*(.__bss_start));
-	}
-
-	.bss (NOLOAD) : {
+	.bss ALIGN(8) : {
+		__bss_start = .;
 		*(.bss*)
-		 . = ALIGN(8);
-	}
-
-	.bss_end (NOLOAD) : {
-		KEEP(*(.__bss_end));
+		__bss_end = .;
 	}

 	/DISCARD/ : { *(.dynsym) }
diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
index 3b7c9d515f8b..25c81b21018b 100644
--- a/arch/arm/mach-zynq/u-boot.lds
+++ b/arch/arm/mach-zynq/u-boot.lds
@@ -103,23 +103,14 @@  SECTIONS
 	_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)
+ * These sections occupy the same memory, but their lifetimes do
+ * not overlap: U-Boot initializes .bss only after applying dynamic
+ * relocations and therefore after it doesn't need .rel.dyn any more.
  */
-
-	.bss_start __rel_dyn_start (OVERLAY) : {
-		KEEP(*(.__bss_start));
-		__bss_base = .;
-	}
-
-	.bss __bss_base (OVERLAY) : {
+	.bss ADDR(.rel.dyn) (OVERLAY): {
+		__bss_start = .;
 		*(.bss*)
-		 . = ALIGN(8);
-		 __bss_limit = .;
-	}
-
-	.bss_end __bss_limit (OVERLAY) : {
-		KEEP(*(.__bss_end));
+		__bss_end = .;
 	}

 	/*