[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250211234944.GD10433@brightrain.aerifal.cx>
Date: Tue, 11 Feb 2025 18:49:44 -0500
From: Rich Felker <dalias@...c.org>
To: Daniil Kovalev <dkovalev@...esssoftek.com>
Cc: "musl@...ts.openwall.com" <musl@...ts.openwall.com>
Subject: Re: RE: [EXTERNAL] Re: [PATCH] aarch64: handle TLSDESC
relocs against undef weak symbols
On Tue, Feb 11, 2025 at 10:21:45PM +0000, Daniil Kovalev wrote:
> > -----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.
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
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.
