diff mbox series

[uclibc-ng-devel,5/5] xtensa: add static pie support

Message ID 20220909183009.4111393-6-jcmvbkbc@gmail.com
State Superseded
Headers show
Series add static PIE support for xtensa | expand

Commit Message

Max Filippov Sept. 9, 2022, 6:30 p.m. UTC 
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 extra/Configs/Config.in                |  4 +++-
 ldso/ldso/xtensa/dl-startup.h          |  2 ++
 libc/misc/internals/reloc_static_pie.c |  2 +-
 libc/sysdeps/linux/xtensa/crt1.S       | 27 ++++++++++++++++++++++++++
 4 files changed, 33 insertions(+), 2 deletions(-)

Comments

linted Sept. 14, 2022, 7:23 p.m. UTC | #1 
There is a lot to unpack here, so please forgive me while I try to format
this in a readable and concise way.

[Patch 1/5]
I thought this was already submitted and merged in? It looks identical to
"[PATCH v2] static pie: building static PDE", or am I mistaken?

[Patch 2/5]
I'm going to be honest with you, I left that check out because it only
affects xtensia, so it's funny to me that it would bite me this quickly.
That said, reading the code, I'm not sure why xtensia decided to call
elf_machine_relative in PERFORM_BOOTSTRAP_GOT and not use the default code
path with ELF_MACHINE_PLTREL_OVERLAP set to ensure only 1 iteration. I am
not very familiar with xtensia, so there may be some nuance that I'm not
aware of, but I would like your opinion on if we should change xtensia to
conform to how every other architecture seems to perform that relocation.

If we are going to keep the code the way it is, I would like to know
everyone's opinion on whether or not we should change that preprocessor
directive to be explicit on which architectures shouldn't do that section
of code? Currently it's in an allow list format with seemingly only xtensia
having a false condition, but that isn't very clear to me from an initial
read. Obviously that change would belong to a separate patch and shouldn't
hold this up.

[Patch 3/5]
looks good to me.

[Patch 4/5]
I think this removal further implies that we should move the
elf_machine_relative call out of PERFORM_BOOTSTRAP_GOT as I suggested above.

[Patch 5/5]
I am getting segfaults while still in __uClibc_main when using buildroot's
qemu_xtensa_lx60_defconfig and qemu-xtensa.  I'm not sure the exact cause,
but I would guess that some functions aren't being relocated correctly or
the TLS wasn't updated correctly. It could also possibly be a problem with
the quick gcc patch I wrote to compile with -static-pie. If you have a gcc
patch that you think will fix this as well, I would be glad to test it.

Here is the stack trace up to where it calls 0x00:
#0  0x3efe58e9 in __h_errno_location () at libpthread/nptl/herrno.c:33
#1  0x3efe12aa in __uClibc_main (main=0x3efddfbc <main>, argc=0x1,
argv=0x3efdb314, app_init=0x3efdde14 <_init>, app_fini=0x3eff5864 <_fini>,
rtld_fini=0x0, stack_end=0x3efdb310) at
libc/misc/internals/__uClibc_main.c:514
#2  0x3efdde6c in _start ()


On Fri, Sep 9, 2022 at 2:30 PM Max Filippov <jcmvbkbc@gmail.com> wrote:

> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  extra/Configs/Config.in                |  4 +++-
>  ldso/ldso/xtensa/dl-startup.h          |  2 ++
>  libc/misc/internals/reloc_static_pie.c |  2 +-
>  libc/sysdeps/linux/xtensa/crt1.S       | 27 ++++++++++++++++++++++++++
>  4 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/extra/Configs/Config.in b/extra/Configs/Config.in
> index 43c04fd0a271..dd1beaadcee3 100644
> --- a/extra/Configs/Config.in
> +++ b/extra/Configs/Config.in
> @@ -324,7 +324,9 @@ config DOPIC
>  config STATIC_PIE
>         bool "Add support for Static Position Independent Executables
> (PIE)"
>         default n
> -       depends on DOPIC && !UCLIBC_FORMAT_FDPIC_ELF && (TARGET_arm ||
> TARGET_i386 || TARGET_x86_64 || TARGET_aarch64 || TARGET_mips)
> +       depends on DOPIC && !UCLIBC_FORMAT_FDPIC_ELF && \
> +               (TARGET_arm || TARGET_i386 || TARGET_x86_64 ||
> TARGET_aarch64 || \
> +                TARGET_mips || TARGET_xtensa)
>
>  config ARCH_HAS_NO_SHARED
>         bool
> diff --git a/ldso/ldso/xtensa/dl-startup.h b/ldso/ldso/xtensa/dl-startup.h
> index 19955ffc77a5..1babaccea7e8 100644
> --- a/ldso/ldso/xtensa/dl-startup.h
> +++ b/ldso/ldso/xtensa/dl-startup.h
> @@ -7,6 +7,7 @@
>   * Parts taken from glibc/sysdeps/xtensa/dl-machine.h.
>   */
>
> +#ifndef L_rcrt1
>  __asm__ (
>      "  .text\n"
>      "  .align  4\n"
> @@ -81,6 +82,7 @@ __asm__ (
>      "  addi    a5, a5, 8\n"
>      "  bnez    a6, 3b\n"
>      "  j      .Lfixup_stack_ret");
> +#endif
>
>  /* Get a pointer to the argv value.  */
>  #define GET_ARGV(ARGVP, ARGS) ARGVP = (((unsigned long *) ARGS) + 1)
> diff --git a/libc/misc/internals/reloc_static_pie.c
> b/libc/misc/internals/reloc_static_pie.c
> index 9f5366cc6ab7..81b235a51cb3 100644
> --- a/libc/misc/internals/reloc_static_pie.c
> +++ b/libc/misc/internals/reloc_static_pie.c
> @@ -21,7 +21,7 @@
>  #include <dl-elf.h>
>
>  #include <ldso.h>
> -#ifdef __mips__
> +#if defined(__mips__) || defined(__xtensa__)
>  #include <dl-startup.h>
>  #endif
>
> diff --git a/libc/sysdeps/linux/xtensa/crt1.S
> b/libc/sysdeps/linux/xtensa/crt1.S
> index efbe264c03f0..3fa14ae583a9 100644
> --- a/libc/sysdeps/linux/xtensa/crt1.S
> +++ b/libc/sysdeps/linux/xtensa/crt1.S
> @@ -76,9 +76,26 @@
>         .global _start
>         .type   _start, @function
>  _start:
> +#ifdef L_rcrt1
> +       .begin  no-transform
> +       call0   1f
> +.Lret_addr:
> +       .end    no-transform
> +       .align  4
> +1:
> +#endif
>  #if defined(__XTENSA_WINDOWED_ABI__)
> +#ifdef L_rcrt1
> +       movi    a6, .Lret_addr
> +       sub     a6, a0, a6
> +       movi    a0, 0
> +       movi    a4, reloc_static_pie
> +       add     a4, a4, a6
> +       callx4  a4
> +#else
>         /* Clear a0 to obviously mark the outermost frame.  */
>         movi    a0, 0
> +#endif
>
>         /* Load up the user's main function.  */
>         movi    a6, main
> @@ -106,8 +123,18 @@ _start:
>         movi    a4, __uClibc_main
>         callx4  a4
>  #elif defined(__XTENSA_CALL0_ABI__)
> +#ifdef L_rcrt1
> +       mov     a12, a2
> +       movi    a2, .Lret_addr
> +       sub     a2, a0, a2
> +       movi    a0, reloc_static_pie
> +       add     a0, a0, a2
> +       callx0  a0
> +       mov     a7, a12
> +#else
>         /* Setup the shared library termination function.  */
>         mov     a7, a2
> +#endif
>
>         /* Load up the user's main function.  */
>         movi    a2, main
> --
> 2.30.2
>
>
Max Filippov Sept. 14, 2022, 7:48 p.m. UTC | #2 
Hi linted,

thank you for taking a look.

On Wed, Sep 14, 2022 at 12:23 PM linted <linted90@gmail.com> wrote:
> [Patch 1/5]
> I thought this was already submitted and merged in? It looks identical to
> "[PATCH v2] static pie: building static PDE", or am I mistaken?

Yes, it's the exact same patch. I'm looking at this uClibc-ng repository:

https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/

and it's not there, so I included it for completeness.

> [Patch 2/5]
> I'm going to be honest with you, I left that check out because it only affects
> xtensia, so it's funny to me that it would bite me this quickly. That said,
> reading the code, I'm not sure why xtensia decided to call elf_machine_relative
> in PERFORM_BOOTSTRAP_GOT and not use the default code path with
> ELF_MACHINE_PLTREL_OVERLAP set to ensure only 1 iteration.
> I am not very familiar with xtensia, so there may be some nuance that I'm
> not aware of, but I would like your opinion on if we should change xtensia
> to conform to how every other architecture seems to perform that relocation.

Yes, I stumbled here too and the same thought crossed my mind. I guess
I should do another pass, try to understand why it's done that way and
clean it up.

> [Patch 3/5]
> looks good to me.
>
> [Patch 4/5]
> I think this removal further implies that we should move the
> elf_machine_relative call out of PERFORM_BOOTSTRAP_GOT
> as I suggested above.
>
> [Patch 5/5]
> I am getting segfaults while still in __uClibc_main when using
> buildroot's qemu_xtensa_lx60_defconfig and qemu-xtensa.
> I'm not sure the exact cause, but I would guess that some functions
> aren't being relocated correctly or the TLS wasn't updated correctly.
> It could also possibly be a problem with the quick gcc patch I wrote to
> compile with -static-pie. If you have a gcc patch that you think will fix
> this as well, I would be glad to test it.

You're right, the following recent binutils and gcc changes are
required for generation of correct relocations and recognition of
the -static-pie option:

https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=658ba81aef5e85a08d67eb211a43c6db775a36b3
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9e0c2696724d4d004ea189a69f15781c7baa68e1
Lance Fredrickson Sept. 14, 2022, 8:15 p.m. UTC | #3 
On 9/14/2022 1:23 PM, linted wrote:
> [Patch 5/5]
> I am getting segfaults while still in __uClibc_main when using 
> buildroot's qemu_xtensa_lx60_defconfig and qemu-xtensa.  I'm not sure 
> the exact cause, but I would guess that some functions aren't being 
> relocated correctly or the TLS wasn't updated correctly. It could also 
> possibly be a problem with the quick gcc patch I wrote to compile with 
> -static-pie. If you have a gcc patch that you think will fix this as 
> well, I would be glad to test it.
I've had -static-pie binaries segfault with qemu while they ran fine on 
actual hardware. I was using Debian 10 so maybe that's because it's an 
old version of qemu, and maybe newer versions can handle this better?

Lance
linted Sept. 15, 2022, 5:43 p.m. UTC | #4 
>> [Patch 5/5]
>> I am getting segfaults while still in __uClibc_main when using
>> buildroot's qemu_xtensa_lx60_defconfig and qemu-xtensa.  I'm not sure
>> the exact cause, but I would guess that some functions aren't being
>> relocated correctly or the TLS wasn't updated correctly. It could also
>> possibly be a problem with the quick gcc patch I wrote to compile with
>> -static-pie. If you have a gcc patch that you think will fix this as
>> well, I would be glad to test it.

> You're right, the following recent binutils and gcc changes are
> required for generation of correct relocations and recognition of
> the -static-pie option:
>
https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=658ba81aef5e85a08d67eb211a43c6db775a36b3
<https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=658ba81aef5e85a08d67eb211a43c6db775a36b3>
>
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9e0c2696724d4d004ea189a69f15781c7baa68e1
<https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9e0c2696724d4d004ea189a69f15781c7baa68e1>

The binutils patch fixed the problems! I was able to make a xtensa
toolchain which worked with my test programs for C and C++.


On Wed, Sep 14, 2022 at 4:15 PM Lance Fredrickson <lancethepants@gmail.com>
wrote:

>
>
> On 9/14/2022 1:23 PM, linted wrote:
> > [Patch 5/5]
> > I am getting segfaults while still in __uClibc_main when using
> > buildroot's qemu_xtensa_lx60_defconfig and qemu-xtensa.  I'm not sure
> > the exact cause, but I would guess that some functions aren't being
> > relocated correctly or the TLS wasn't updated correctly. It could also
> > possibly be a problem with the quick gcc patch I wrote to compile with
> > -static-pie. If you have a gcc patch that you think will fix this as
> > well, I would be glad to test it.
> I've had -static-pie binaries segfault with qemu while they ran fine on
> actual hardware. I was using Debian 10 so maybe that's because it's an
> old version of qemu, and maybe newer versions can handle this better?
>
> Lance
> _______________________________________________
> devel mailing list -- devel@uclibc-ng.org
> To unsubscribe send an email to devel-leave@uclibc-ng.org
>
diff mbox series

Patch

diff --git a/extra/Configs/Config.in b/extra/Configs/Config.in
index 43c04fd0a271..dd1beaadcee3 100644
--- a/extra/Configs/Config.in
+++ b/extra/Configs/Config.in
@@ -324,7 +324,9 @@  config DOPIC
 config STATIC_PIE
 	bool "Add support for Static Position Independent Executables (PIE)"
 	default n
-	depends on DOPIC && !UCLIBC_FORMAT_FDPIC_ELF && (TARGET_arm || TARGET_i386 || TARGET_x86_64 || TARGET_aarch64 || TARGET_mips)
+	depends on DOPIC && !UCLIBC_FORMAT_FDPIC_ELF && \
+		(TARGET_arm || TARGET_i386 || TARGET_x86_64 || TARGET_aarch64 || \
+		 TARGET_mips || TARGET_xtensa)
 
 config ARCH_HAS_NO_SHARED
 	bool
diff --git a/ldso/ldso/xtensa/dl-startup.h b/ldso/ldso/xtensa/dl-startup.h
index 19955ffc77a5..1babaccea7e8 100644
--- a/ldso/ldso/xtensa/dl-startup.h
+++ b/ldso/ldso/xtensa/dl-startup.h
@@ -7,6 +7,7 @@ 
  * Parts taken from glibc/sysdeps/xtensa/dl-machine.h.
  */
 
+#ifndef L_rcrt1
 __asm__ (
     "	.text\n"
     "	.align  4\n"
@@ -81,6 +82,7 @@  __asm__ (
     "	addi    a5, a5, 8\n"
     "	bnez    a6, 3b\n"
     "	j      .Lfixup_stack_ret");
+#endif
 
 /* Get a pointer to the argv value.  */
 #define GET_ARGV(ARGVP, ARGS) ARGVP = (((unsigned long *) ARGS) + 1)
diff --git a/libc/misc/internals/reloc_static_pie.c b/libc/misc/internals/reloc_static_pie.c
index 9f5366cc6ab7..81b235a51cb3 100644
--- a/libc/misc/internals/reloc_static_pie.c
+++ b/libc/misc/internals/reloc_static_pie.c
@@ -21,7 +21,7 @@ 
 #include <dl-elf.h>
 
 #include <ldso.h>
-#ifdef __mips__
+#if defined(__mips__) || defined(__xtensa__)
 #include <dl-startup.h>
 #endif
 
diff --git a/libc/sysdeps/linux/xtensa/crt1.S b/libc/sysdeps/linux/xtensa/crt1.S
index efbe264c03f0..3fa14ae583a9 100644
--- a/libc/sysdeps/linux/xtensa/crt1.S
+++ b/libc/sysdeps/linux/xtensa/crt1.S
@@ -76,9 +76,26 @@ 
 	.global	_start
 	.type	_start, @function
 _start:
+#ifdef L_rcrt1
+	.begin	no-transform
+	call0	1f
+.Lret_addr:
+	.end	no-transform
+	.align	4
+1:
+#endif
 #if defined(__XTENSA_WINDOWED_ABI__)
+#ifdef L_rcrt1
+	movi	a6, .Lret_addr
+	sub	a6, a0, a6
+	movi	a0, 0
+	movi	a4, reloc_static_pie
+	add	a4, a4, a6
+	callx4	a4
+#else
 	/* Clear a0 to obviously mark the outermost frame.  */
 	movi	a0, 0
+#endif
 
 	/* Load up the user's main function.  */
 	movi	a6, main
@@ -106,8 +123,18 @@  _start:
 	movi	a4, __uClibc_main
 	callx4	a4
 #elif defined(__XTENSA_CALL0_ABI__)
+#ifdef L_rcrt1
+	mov	a12, a2
+	movi	a2, .Lret_addr
+	sub	a2, a0, a2
+	movi	a0, reloc_static_pie
+	add	a0, a0, a2
+	callx0	a0
+	mov	a7, a12
+#else
 	/* Setup the shared library termination function.  */
 	mov	a7, a2
+#endif
 
 	/* Load up the user's main function.  */
 	movi	a2, main