[PATCH] riscv: Fix breakage caused by linker relaxation

[PATCH] riscv: Fix breakage caused by linker relaxation

[Sean Anderson]

Due to the two-instruction sequence needed to access arbitrary memory
locations, the RISC-V linker aggressively optimises memory accesses and
jumps at link-time. This is called "linker relaxation," and is discussed
in this SiFive article
<https://www.sifive.com/blog/all-aboard-part-3-linker-relaxation-in-riscv-toolchain>.
One of the optimizations in place is to assume that the __global_pointer
symbol is placed in the gp register. To quote the article:

"...The magic __global_pointer$ symbol is defined to point 0x800 bytes
past the start of the .sdata section. The 0x800 magic number allows
signed 12-bit offsets from __global_pointer$ to address symbols at the
start of the .sdata section. The linker assumes that if this symbol is
defined, then the gp register contains that value, which it can then use
to relax accesses to global symbols within that 12-bit range. The
compiler treats the gp register as a constant so it doesn't need to be
saved or restored, which means it is generally only written by _start,
the ELF entry point."

However, U-Boot instead keeps the global data pointer in gp. This causes
memory accesses and jumps optimized to use the gp pointer to fail. To
fix this problem, we undefine the __global_pointer symbol.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
 arch/riscv/cpu/u-boot.lds | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/riscv/cpu/u-boot.lds b/arch/riscv/cpu/u-boot.lds
index 838a844399..c00d17c736 100644
--- a/arch/riscv/cpu/u-boot.lds
+++ b/arch/riscv/cpu/u-boot.lds
@@ -32,7 +32,6 @@ SECTIONS

 	. = ALIGN(4);
 	.data : {
-		__global_pointer$ = . + 0x800;
 		*(.data*)
 	}
 	. = ALIGN(4);
-- 
2.23.0

[PATCH] riscv: Fix breakage caused by linker relaxation

[Bin Meng]

On Wed, Dec 18, 2019 at 8:09 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> Due to the two-instruction sequence needed to access arbitrary memory
> locations, the RISC-V linker aggressively optimises memory accesses and
> jumps at link-time. This is called "linker relaxation," and is discussed
> in this SiFive article
> <https://www.sifive.com/blog/all-aboard-part-3-linker-relaxation-in-riscv-toolchain>.
> One of the optimizations in place is to assume that the __global_pointer
> symbol is placed in the gp register. To quote the article:
>
> "...The magic __global_pointer$ symbol is defined to point 0x800 bytes
> past the start of the .sdata section. The 0x800 magic number allows
> signed 12-bit offsets from __global_pointer$ to address symbols at the
> start of the .sdata section. The linker assumes that if this symbol is
> defined, then the gp register contains that value, which it can then use
> to relax accesses to global symbols within that 12-bit range. The
> compiler treats the gp register as a constant so it doesn't need to be
> saved or restored, which means it is generally only written by _start,
> the ELF entry point."
>
> However, U-Boot instead keeps the global data pointer in gp. This causes
> memory accesses and jumps optimized to use the gp pointer to fail. To
> fix this problem, we undefine the __global_pointer symbol.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>  arch/riscv/cpu/u-boot.lds | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/arch/riscv/cpu/u-boot.lds b/arch/riscv/cpu/u-boot.lds
> index 838a844399..c00d17c736 100644
> --- a/arch/riscv/cpu/u-boot.lds
> +++ b/arch/riscv/cpu/u-boot.lds
> @@ -32,7 +32,6 @@ SECTIONS
>
>         . = ALIGN(4);
>         .data : {
> -               __global_pointer$ = . + 0x800;
>                 *(.data*)
>         }
>         . = ALIGN(4);
> --

Good catch!

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

But I wonder how U-Boot managed to work on RISC-V till today?

Regards,
Bin

[PATCH] riscv: Fix breakage caused by linker relaxation

[Sean Anderson]

> But I wonder how U-Boot managed to work on RISC-V till today?

Hm. I think that linker relaxations may also have been disabled by
linking with -pie. When I discovered this bug, I had disabled the flag
since I was having a hard time getting my compiler to build with
--enable-shared. When recompiling with -pie and with __global_pointer
defined, the problem assembly code I noticed now uses the
two-instruction auipc/lw sequence.

	--Sean

[PATCH] riscv: Fix breakage caused by linker relaxation

[Rick Chen]

> > From: Sean Anderson [mailto:seanga2 at gmail.com]
> > Sent: Wednesday, December 18, 2019 10:36 AM
> > To: u-boot at lists.denx.de; Rick Jian-Zhi Chen(陳建志)
> > Subject: [PATCH] riscv: Fix breakage caused by linker relaxation
> >
> > Due to the two-instruction sequence needed to access arbitrary memory
> > locations, the RISC-V linker aggressively optimises memory accesses and jumps
> > at link-time. This is called "linker relaxation," and is discussed in this SiFive
> > article
> > <https://www.sifive.com/blog/all-aboard-part-3-linker-relaxation-in-riscv-tool
> > chain>.
> > One of the optimizations in place is to assume that the __global_pointer
> > symbol is placed in the gp register. To quote the article:
> >
> > "...The magic __global_pointer$ symbol is defined to point 0x800 bytes past
> > the start of the .sdata section. The 0x800 magic number allows signed 12-bit
> > offsets from __global_pointer$ to address symbols at the start of the .sdata
> > section. The linker assumes that if this symbol is defined, then the gp register
> > contains that value, which it can then use to relax accesses to global symbols
> > within that 12-bit range. The compiler treats the gp register as a constant so it
> > doesn't need to be saved or restored, which means it is generally only written
> > by _start, the ELF entry point."
> >
> > However, U-Boot instead keeps the global data pointer in gp. This causes
> > memory accesses and jumps optimized to use the gp pointer to fail. To fix this
> > problem, we undefine the __global_pointer symbol.
> >
> > Signed-off-by: Sean Anderson <seanga2@gmail.com>
> > ---
> >  arch/riscv/cpu/u-boot.lds | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/arch/riscv/cpu/u-boot.lds b/arch/riscv/cpu/u-boot.lds index
> > 838a844399..c00d17c736 100644
> > --- a/arch/riscv/cpu/u-boot.lds
> > +++ b/arch/riscv/cpu/u-boot.lds
> > @@ -32,7 +32,6 @@ SECTIONS
> >
> >       . = ALIGN(4);
> >       .data : {
> > -             __global_pointer$ = . + 0x800;
> >               *(.data*)
> >       }
> >       . = ALIGN(4);

Reviewed-by: Rick Chen <rick@andestech.com>