[musl] [PATCH] aarch64: handle TLSDESC relocs against undef weak symbols

[musl] [PATCH] aarch64: handle TLSDESC relocs against undef weak symbols

[Daniil Kovalev]

if a weak symbol is undefined, it's address must be null. when accessing
a thread-local symbol via tlsdesc, the value of TPIDR_EL0 is added to the
return value of the tls descriptor function. so, in case of an undefined
symbol, the descriptor function should return -TPIDR_EL0 which will give
null after adding TPIDR_EL0.
---
 arch/aarch64/reloc.h       | 2 ++
 ldso/dynlink.c             | 6 ++++++
 src/internal/dynlink.h     | 3 +++
 src/ldso/aarch64/tlsdesc.s | 9 +++++++++
 4 files changed, 20 insertions(+)

diff --git a/arch/aarch64/reloc.h b/arch/aarch64/reloc.h
index b1b68c72..a8229982 100644
--- a/arch/aarch64/reloc.h
+++ b/arch/aarch64/reloc.h
@@ -20,5 +20,7 @@
 #define REL_TPOFF       R_AARCH64_TLS_TPREL64
 #define REL_TLSDESC     R_AARCH64_TLSDESC
 
+#define TLSDESC_UNDEF_WEAK_SUPPORT 1
+
 #define CRTJMP(pc,sp) __asm__ __volatile__( \
 	"mov sp,%1 ; br %0" : : "r"(pc), "r"(sp) : "memory" )
diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index 3b57c07f..4b19ef0d 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -515,6 +515,12 @@ static void do_relocs(struct dso *dso, size_t *rel, size_t rel_size, size_t stri
 #endif
 		case REL_TLSDESC:
 			if (stride<3) addend = reloc_addr[!TLSDESC_BACKWARDS];
+#ifdef TLSDESC_UNDEF_WEAK_SUPPORT
+			if (sym && sym->st_info>>4 == STB_WEAK && sym->st_shndx == SHN_UNDEF) {
+				reloc_addr[0] = (size_t)__tlsdesc_undef_weak;
+				reloc_addr[1] = 0;
+			} else
+#endif
 			if (def.dso->tls_id > static_tls_cnt) {
 				struct td_index *new = malloc(sizeof *new);
 				if (!new) {
diff --git a/src/internal/dynlink.h b/src/internal/dynlink.h
index 40c743e2..588458f3 100644
--- a/src/internal/dynlink.h
+++ b/src/internal/dynlink.h
@@ -112,6 +112,9 @@ hidden int __dl_invalid_handle(void *);
 hidden void __dl_vseterr(const char *, va_list);
 
 hidden ptrdiff_t __tlsdesc_static(), __tlsdesc_dynamic();
+#ifdef TLSDESC_UNDEF_WEAK_SUPPORT
+hidden ptrdiff_t __tlsdesc_undef_weak();
+#endif
 
 hidden extern int __malloc_replaced;
 hidden extern int __aligned_alloc_replaced;
diff --git a/src/ldso/aarch64/tlsdesc.s b/src/ldso/aarch64/tlsdesc.s
index c6c685b3..e4b0f9a6 100644
--- a/src/ldso/aarch64/tlsdesc.s
+++ b/src/ldso/aarch64/tlsdesc.s
@@ -29,3 +29,12 @@ __tlsdesc_dynamic:
 	add x0,x1,x2          // dtv[p->modidx] + p->off - tp
 	ldp x1,x2,[sp],#16
 	ret
+
+.global __tlsdesc_undef_weak
+.hidden __tlsdesc_undef_weak
+.type __tlsdesc_undef_weak,@function
+__tlsdesc_undef_weak:
+	mov x0,#0
+	mrs x8,TPIDR_EL0
+	sub x0,x0,x8
+	ret
-- 
2.48.1

Re: [musl] [PATCH] aarch64: handle TLSDESC relocs against undef weak symbols

[Rich Felker]

On Tue, Feb 11, 2025 at 07:50:40PM +0000, Daniil Kovalev wrote:
> if a weak symbol is undefined, it's address must be null. when accessing
> a thread-local symbol via tlsdesc, the value of TPIDR_EL0 is added to the
> return value of the tls descriptor function. so, in case of an undefined
> symbol, the descriptor function should return -TPIDR_EL0 which will give
> null after adding TPIDR_EL0.
> ---
>  arch/aarch64/reloc.h       | 2 ++
>  ldso/dynlink.c             | 6 ++++++
>  src/internal/dynlink.h     | 3 +++
>  src/ldso/aarch64/tlsdesc.s | 9 +++++++++
>  4 files changed, 20 insertions(+)
> 
> diff --git a/arch/aarch64/reloc.h b/arch/aarch64/reloc.h
> index b1b68c72..a8229982 100644
> --- a/arch/aarch64/reloc.h
> +++ b/arch/aarch64/reloc.h
> @@ -20,5 +20,7 @@
>  #define REL_TPOFF       R_AARCH64_TLS_TPREL64
>  #define REL_TLSDESC     R_AARCH64_TLSDESC
>  
> +#define TLSDESC_UNDEF_WEAK_SUPPORT 1
> +
>  #define CRTJMP(pc,sp) __asm__ __volatile__( \
>  	"mov sp,%1 ; br %0" : : "r"(pc), "r"(sp) : "memory" )
> diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> index 3b57c07f..4b19ef0d 100644
> --- a/ldso/dynlink.c
> +++ b/ldso/dynlink.c
> @@ -515,6 +515,12 @@ static void do_relocs(struct dso *dso, size_t *rel, size_t rel_size, size_t stri
>  #endif
>  		case REL_TLSDESC:
>  			if (stride<3) addend = reloc_addr[!TLSDESC_BACKWARDS];
> +#ifdef TLSDESC_UNDEF_WEAK_SUPPORT
> +			if (sym && sym->st_info>>4 == STB_WEAK && sym->st_shndx == SHN_UNDEF) {
> +				reloc_addr[0] = (size_t)__tlsdesc_undef_weak;
> +				reloc_addr[1] = 0;
> +			} else
> +#endif
>  			if (def.dso->tls_id > static_tls_cnt) {
>  				struct td_index *new = malloc(sizeof *new);
>  				if (!new) {
> diff --git a/src/internal/dynlink.h b/src/internal/dynlink.h
> index 40c743e2..588458f3 100644
> --- a/src/internal/dynlink.h
> +++ b/src/internal/dynlink.h
> @@ -112,6 +112,9 @@ hidden int __dl_invalid_handle(void *);
>  hidden void __dl_vseterr(const char *, va_list);
>  
>  hidden ptrdiff_t __tlsdesc_static(), __tlsdesc_dynamic();
> +#ifdef TLSDESC_UNDEF_WEAK_SUPPORT
> +hidden ptrdiff_t __tlsdesc_undef_weak();
> +#endif
>  
>  hidden extern int __malloc_replaced;
>  hidden extern int __aligned_alloc_replaced;
> diff --git a/src/ldso/aarch64/tlsdesc.s b/src/ldso/aarch64/tlsdesc.s
> index c6c685b3..e4b0f9a6 100644
> --- a/src/ldso/aarch64/tlsdesc.s
> +++ b/src/ldso/aarch64/tlsdesc.s
> @@ -29,3 +29,12 @@ __tlsdesc_dynamic:
>  	add x0,x1,x2          // dtv[p->modidx] + p->off - tp
>  	ldp x1,x2,[sp],#16
>  	ret
> +
> +.global __tlsdesc_undef_weak
> +.hidden __tlsdesc_undef_weak
> +.type __tlsdesc_undef_weak,@function
> +__tlsdesc_undef_weak:
> +	mov x0,#0
> +	mrs x8,TPIDR_EL0
> +	sub x0,x0,x8
> +	ret
> -- 
> 2.48.1
> 

Thanks for bringing this up. Undefined weak TLS has been something
that's just been unsupported for a long time, with a vague idea that
might change at some point in the future. If we do adopt support
though:

1. It should be for all archs not just aarch64. I don't think this is
   hard and I can help with the asm for others if needed.

2. I'm not sure right now what happens with non-TLSDESC accesses.
   Really they should work too as long as non-TLSDESC is not
   deprecated, and I don't think they work at present. In order not to
   pessimize __tls_get_addr with new code paths, making it work
   probably requires adding a dummy DTV index (modid) with 0 as its
   base address, so that undef-weak symbols can resolve to that modid.

In addressing (2), it might make sense to go ahead and incorporate -tp
into the dtv values too. This would slightly trim down the cost of
TLSDESC lookups at the cost of adding an insn to __tls_get_addr.
Either way, I think the TLSDESC undef-weak case could then use this
slot the same way __tls_get_addr does, so that a dedicated
__tlsdesc_undef_weak wouldn't be necessary (but it could still be
there as an optimization if desired).

Rich

[musl] RE: [EXTERNAL] Re: [musl] [PATCH] aarch64: handle TLSDESC relocs against undef weak symbols

[Daniil Kovalev]

> -----Original Message-----
> From: Rich Felker <dalias@libc.org>
> Sent: Wednesday, February 12, 2025 00:09
> To: Daniil Kovalev <dkovalev@accesssoftek.com>
> Cc: musl@lists.openwall.com
> Subject: [EXTERNAL] Re: [musl] [PATCH] aarch64: handle TLSDESC relocs against
> undef weak symbols
> 
> On Tue, Feb 11, 2025 at 07:50:40PM +0000, Daniil Kovalev wrote:
> > if a weak symbol is undefined, it's address must be null. when accessing
> > a thread-local symbol via tlsdesc, the value of TPIDR_EL0 is added to the
> > return value of the tls descriptor function. so, in case of an undefined
> > symbol, the descriptor function should return -TPIDR_EL0 which will give
> > null after adding TPIDR_EL0.
> > ---
> >  arch/aarch64/reloc.h       | 2 ++
> >  ldso/dynlink.c             | 6 ++++++
> >  src/internal/dynlink.h     | 3 +++
> >  src/ldso/aarch64/tlsdesc.s | 9 +++++++++
> >  4 files changed, 20 insertions(+)
> >
> > diff --git a/arch/aarch64/reloc.h b/arch/aarch64/reloc.h
> > index b1b68c72..a8229982 100644
> > --- a/arch/aarch64/reloc.h
> > +++ b/arch/aarch64/reloc.h
> > @@ -20,5 +20,7 @@
> >  #define REL_TPOFF       R_AARCH64_TLS_TPREL64
> >  #define REL_TLSDESC     R_AARCH64_TLSDESC
> >
> > +#define TLSDESC_UNDEF_WEAK_SUPPORT 1
> > +
> >  #define CRTJMP(pc,sp) __asm__ __volatile__( \
> >       "mov sp,%1 ; br %0" : : "r"(pc), "r"(sp) : "memory" )
> > diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> > index 3b57c07f..4b19ef0d 100644
> > --- a/ldso/dynlink.c
> > +++ b/ldso/dynlink.c
> > @@ -515,6 +515,12 @@ static void do_relocs(struct dso *dso, size_t *rel,
> size_t rel_size, size_t stri
> >  #endif
> >               case REL_TLSDESC:
> >                       if (stride<3) addend = reloc_addr[!TLSDESC_BACKWARDS];
> > +#ifdef TLSDESC_UNDEF_WEAK_SUPPORT
> > +                     if (sym && sym->st_info>>4 == STB_WEAK && sym->st_shndx
> == SHN_UNDEF) {
> > +                             reloc_addr[0] = (size_t)__tlsdesc_undef_weak;
> > +                             reloc_addr[1] = 0;
> > +                     } else
> > +#endif
> >                       if (def.dso->tls_id > static_tls_cnt) {
> >                               struct td_index *new = malloc(sizeof *new);
> >                               if (!new) {
> > diff --git a/src/internal/dynlink.h b/src/internal/dynlink.h
> > index 40c743e2..588458f3 100644
> > --- a/src/internal/dynlink.h
> > +++ b/src/internal/dynlink.h
> > @@ -112,6 +112,9 @@ hidden int __dl_invalid_handle(void *);
> >  hidden void __dl_vseterr(const char *, va_list);
> >
> >  hidden ptrdiff_t __tlsdesc_static(), __tlsdesc_dynamic();
> > +#ifdef TLSDESC_UNDEF_WEAK_SUPPORT
> > +hidden ptrdiff_t __tlsdesc_undef_weak();
> > +#endif
> >
> >  hidden extern int __malloc_replaced;
> >  hidden extern int __aligned_alloc_replaced;
> > diff --git a/src/ldso/aarch64/tlsdesc.s b/src/ldso/aarch64/tlsdesc.s
> > index c6c685b3..e4b0f9a6 100644
> > --- a/src/ldso/aarch64/tlsdesc.s
> > +++ b/src/ldso/aarch64/tlsdesc.s
> > @@ -29,3 +29,12 @@ __tlsdesc_dynamic:
> >       add x0,x1,x2          // dtv[p->modidx] + p->off - tp
> >       ldp x1,x2,[sp],#16
> >       ret
> > +
> > +.global __tlsdesc_undef_weak
> > +.hidden __tlsdesc_undef_weak
> > +.type __tlsdesc_undef_weak,@function
> > +__tlsdesc_undef_weak:
> > +     mov x0,#0
> > +     mrs x8,TPIDR_EL0
> > +     sub x0,x0,x8
> > +     ret
> > --
> > 2.48.1
> >
> 
> Thanks for bringing this up. Undefined weak TLS has been something
> that's just been unsupported for a long time, with a vague idea that
> might change at some point in the future. If we do adopt support
> though:
> 
> 1. It should be for all archs not just aarch64. I don't think this is
>    hard and I can help with the asm for others if needed.
> 
> 2. I'm not sure right now what happens with non-TLSDESC accesses.
>    Really they should work too as long as non-TLSDESC is not
>    deprecated, and I don't think they work at present. In order not to
>    pessimize __tls_get_addr with new code paths, making it work
>    probably requires adding a dummy DTV index (modid) with 0 as its
>    base address, so that undef-weak symbols can resolve to that modid.
> 
> In addressing (2), it might make sense to go ahead and incorporate -tp
> into the dtv values too. This would slightly trim down the cost of
> TLSDESC lookups at the cost of adding an insn to __tls_get_addr.
> Either way, I think the TLSDESC undef-weak case could then use this
> slot the same way __tls_get_addr does, so that a dedicated
> __tlsdesc_undef_weak wouldn't be necessary (but it could still be
> there as an optimization if desired).
> 
> Rich

Thanks Rich for your feedback!

> 1. It should be for all archs not just aarch64. I don't think this is
>    hard and I can help with the asm for others if needed.

I'll do that, thanks. The reason why I've limited the changes to aarch64
is the fact that I have an environment where I can test that. But it should
not be that hard to set up Qemu in order to test the functionality for
other architectures. Thanks for suggestion of helping with asm - if there
are some issues I'm unable to resolve by myself, I'll let you know.

BTW, is there some test suite for the dynamic loader? I know that
http://nsz.repo.hu/git/?p=libc-test exists, but it looks like that it only
aims to test the standard library itself and not the ldso. I'll anyway
implement some local tests to ensure the correctness of the changes,
so, it would be nice to submit the tests somewhere as well.

> 2. I'm not sure right now what happens with non-TLSDESC accesses.

Yes, I agree that this should also be handled. I'll investigate this
after finishing the tlsdesc part.

--
Best regards, Daniil Kovalev

Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] aarch64: handle TLSDESC relocs against undef weak symbols

[Rich Felker]

On Tue, Feb 11, 2025 at 10:21:45PM +0000, Daniil Kovalev wrote:
> > -----Original Message-----
> > From: Rich Felker <dalias@libc.org>
> > Sent: Wednesday, February 12, 2025 00:09
> > To: Daniil Kovalev <dkovalev@accesssoftek.com>
> > Cc: musl@lists.openwall.com
> > Subject: [EXTERNAL] Re: [musl] [PATCH] aarch64: handle TLSDESC relocs against
> > undef weak symbols
> > 
> > On Tue, Feb 11, 2025 at 07:50:40PM +0000, Daniil Kovalev wrote:
> > > if a weak symbol is undefined, it's address must be null. when accessing
> > > a thread-local symbol via tlsdesc, the value of TPIDR_EL0 is added to the
> > > return value of the tls descriptor function. so, in case of an undefined
> > > symbol, the descriptor function should return -TPIDR_EL0 which will give
> > > null after adding TPIDR_EL0.
> > > ---
> > >  arch/aarch64/reloc.h       | 2 ++
> > >  ldso/dynlink.c             | 6 ++++++
> > >  src/internal/dynlink.h     | 3 +++
> > >  src/ldso/aarch64/tlsdesc.s | 9 +++++++++
> > >  4 files changed, 20 insertions(+)
> > >
> > > diff --git a/arch/aarch64/reloc.h b/arch/aarch64/reloc.h
> > > index b1b68c72..a8229982 100644
> > > --- a/arch/aarch64/reloc.h
> > > +++ b/arch/aarch64/reloc.h
> > > @@ -20,5 +20,7 @@
> > >  #define REL_TPOFF       R_AARCH64_TLS_TPREL64
> > >  #define REL_TLSDESC     R_AARCH64_TLSDESC
> > >
> > > +#define TLSDESC_UNDEF_WEAK_SUPPORT 1
> > > +
> > >  #define CRTJMP(pc,sp) __asm__ __volatile__( \
> > >       "mov sp,%1 ; br %0" : : "r"(pc), "r"(sp) : "memory" )
> > > diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> > > index 3b57c07f..4b19ef0d 100644
> > > --- a/ldso/dynlink.c
> > > +++ b/ldso/dynlink.c
> > > @@ -515,6 +515,12 @@ static void do_relocs(struct dso *dso, size_t *rel,
> > size_t rel_size, size_t stri
> > >  #endif
> > >               case REL_TLSDESC:
> > >                       if (stride<3) addend = reloc_addr[!TLSDESC_BACKWARDS];
> > > +#ifdef TLSDESC_UNDEF_WEAK_SUPPORT
> > > +                     if (sym && sym->st_info>>4 == STB_WEAK && sym->st_shndx
> > == SHN_UNDEF) {
> > > +                             reloc_addr[0] = (size_t)__tlsdesc_undef_weak;
> > > +                             reloc_addr[1] = 0;
> > > +                     } else
> > > +#endif
> > >                       if (def.dso->tls_id > static_tls_cnt) {
> > >                               struct td_index *new = malloc(sizeof *new);
> > >                               if (!new) {
> > > diff --git a/src/internal/dynlink.h b/src/internal/dynlink.h
> > > index 40c743e2..588458f3 100644
> > > --- a/src/internal/dynlink.h
> > > +++ b/src/internal/dynlink.h
> > > @@ -112,6 +112,9 @@ hidden int __dl_invalid_handle(void *);
> > >  hidden void __dl_vseterr(const char *, va_list);
> > >
> > >  hidden ptrdiff_t __tlsdesc_static(), __tlsdesc_dynamic();
> > > +#ifdef TLSDESC_UNDEF_WEAK_SUPPORT
> > > +hidden ptrdiff_t __tlsdesc_undef_weak();
> > > +#endif
> > >
> > >  hidden extern int __malloc_replaced;
> > >  hidden extern int __aligned_alloc_replaced;
> > > diff --git a/src/ldso/aarch64/tlsdesc.s b/src/ldso/aarch64/tlsdesc.s
> > > index c6c685b3..e4b0f9a6 100644
> > > --- a/src/ldso/aarch64/tlsdesc.s
> > > +++ b/src/ldso/aarch64/tlsdesc.s
> > > @@ -29,3 +29,12 @@ __tlsdesc_dynamic:
> > >       add x0,x1,x2          // dtv[p->modidx] + p->off - tp
> > >       ldp x1,x2,[sp],#16
> > >       ret
> > > +
> > > +.global __tlsdesc_undef_weak
> > > +.hidden __tlsdesc_undef_weak
> > > +.type __tlsdesc_undef_weak,@function
> > > +__tlsdesc_undef_weak:
> > > +     mov x0,#0
> > > +     mrs x8,TPIDR_EL0
> > > +     sub x0,x0,x8
> > > +     ret
> > > --
> > > 2.48.1
> > >
> > 
> > Thanks for bringing this up. Undefined weak TLS has been something
> > that's just been unsupported for a long time, with a vague idea that
> > might change at some point in the future. If we do adopt support
> > though:
> > 
> > 1. It should be for all archs not just aarch64. I don't think this is
> >    hard and I can help with the asm for others if needed.
> > 
> > 2. I'm not sure right now what happens with non-TLSDESC accesses.
> >    Really they should work too as long as non-TLSDESC is not
> >    deprecated, and I don't think they work at present. In order not to
> >    pessimize __tls_get_addr with new code paths, making it work
> >    probably requires adding a dummy DTV index (modid) with 0 as its
> >    base address, so that undef-weak symbols can resolve to that modid.
> > 
> > In addressing (2), it might make sense to go ahead and incorporate -tp
> > into the dtv values too. This would slightly trim down the cost of
> > TLSDESC lookups at the cost of adding an insn to __tls_get_addr.
> > Either way, I think the TLSDESC undef-weak case could then use this
> > slot the same way __tls_get_addr does, so that a dedicated
> > __tlsdesc_undef_weak wouldn't be necessary (but it could still be
> > there as an optimization if desired).
> > 
> > Rich
> 
> Thanks Rich for your feedback!
> 
> > 1. It should be for all archs not just aarch64. I don't think this is
> >    hard and I can help with the asm for others if needed.
> 
> I'll do that, thanks. The reason why I've limited the changes to aarch64
> is the fact that I have an environment where I can test that. But it should
> not be that hard to set up Qemu in order to test the functionality for
> other architectures. Thanks for suggestion of helping with asm - if there
> are some issues I'm unable to resolve by myself, I'll let you know.
> 
> BTW, is there some test suite for the dynamic loader? I know that
> http://nsz.repo.hu/git/?p=libc-test exists, but it looks like that it only
> aims to test the standard library itself and not the ldso. I'll anyway
> implement some local tests to ensure the correctness of the changes,
> so, it would be nice to submit the tests somewhere as well.

I don't think we really have good coverage for the loader itself
anywhere, although libc-test has some dlopen tests that could be
expanded to cover some of these things.

> > 2. I'm not sure right now what happens with non-TLSDESC accesses.
> 
> Yes, I agree that this should also be handled. I'll investigate this
> after finishing the tlsdesc part.

It might make sense to look at it first, since I think making it work
for non-TLSDESC automatically makes it work for TLSDESC too on all
archs without having to write any new asm.

Ricj