[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<BN0SPR01MB0002A606571110DEA27A6C5EB9FD2@BN0SPR01MB0002.namprd20.prod.outlook.com>
Date: Tue, 11 Feb 2025 22:21:45 +0000
From: Daniil Kovalev <dkovalev@...esssoftek.com>
To: Rich Felker <dalias@...c.org>
CC: "musl@...ts.openwall.com" <musl@...ts.openwall.com>
Subject: RE: [EXTERNAL] Re: [PATCH] aarch64: handle TLSDESC relocs
against undef weak symbols
> -----Original Message-----
> From: Rich Felker <dalias@...c.org>
> Sent: Wednesday, February 12, 2025 00:09
> To: Daniil Kovalev <dkovalev@...esssoftek.com>
> Cc: musl@...ts.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
