[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190516074850.GD16415@port70.net>
Date: Thu, 16 May 2019 09:48:50 +0200
From: Szabolcs Nagy <nsz@...t70.net>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for
TLS_ABOVE_TP
* Rich Felker <dalias@...c.org> [2019-05-15 20:20:51 -0400]:
> On Tue, May 14, 2019 at 04:01:31AM +0200, Szabolcs Nagy wrote:
> > this came up because lld changed tls alignment on aarch64 as a
> > workaround for a bionic abi issue https://reviews.llvm.org/D53906
> > but lld does not handle p_vaddr%p_align!=0 right so it broke on glibc
> > https://reviews.llvm.org/D61824
> >
> > the patch is untested (bfd linker cannot seem to create problematic
> > elf objects), but at least there are no regressions with libc-test.
>
> > >From 8c94fcbc9faeb8b07132506757c3d3973652420e Mon Sep 17 00:00:00 2001
> > From: Szabolcs Nagy <nsz@...t70.net>
> > Date: Mon, 13 May 2019 18:47:11 +0000
> > Subject: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP
> >
> > currently the bfd linker does not seem to create tls segments where
> > p_vaddr%p_align != 0, but this is valid in ELF and then the runtime
> > computed tls offset must satisfy
> >
> > offset%p_align == (base+p_vaddr)%p_align
> >
> > and in case of local exec tls (main executable) the smallest such
> > offset must be used (otherwise it is incompatible with the offset
> > computed by the static linker). the !TLS_ABOVE_TP case handled this
> > correctly (the offset is negative then in the formula).
> >
> > the ldso code for TLS_ABOVE_TP is changed so the static tls offset
> > of each module satisfies the formula and tls_offset always points
> > to the end of the allocated static tls area (and not aligned up to
> > tls_align or MIN_TLS_ALIGN).
>
> I guess this saves some wasted memory?
>
> > the tls_offset computation was wrong
> > when multiple modules were loaded with static tls and in some the
> > tls segment p_memsz%p_align != 0.
>
> I don't understand this part. Are you saying we're currently
> misaligning TLS for some libraries now?
any time there are 'alignment gaps' in the static tls area
for shared libs, since tls_offset += dso.tls.size is the
only update to tls_offset which can be misaligned, it is
not updated when dso.tls.offset is aligned up.
the only exception is tls in the application: tls_offset
is aligned up after the app tls. (which my introduce
bigger gap than necessary)
i can make the tls_offset fix a separate change from
the p_vaddr%p_align!=0 fix.
> > ---
> > ldso/dynlink.c | 13 ++++++-------
> > src/env/__init_tls.c | 3 ++-
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> > index 42a5470d..6dc39483 100644
> > --- a/ldso/dynlink.c
> > +++ b/ldso/dynlink.c
> > @@ -1126,9 +1126,9 @@ static struct dso *load_library(const char *name, struct dso *needed_by)
> > p->tls_id = ++tls_cnt;
> > tls_align = MAXP2(tls_align, p->tls.align);
> > #ifdef TLS_ABOVE_TP
> > - p->tls.offset = tls_offset + ( (tls_align-1) &
> > - -(tls_offset + (uintptr_t)p->tls.image) );
> > - tls_offset += p->tls.size;
> > + p->tls.offset = tls_offset + ( (p->tls.align-1) &
> > + (-tls_offset + (uintptr_t)p->tls.image) );
> > + tls_offset = p->tls.offset + p->tls.size;
>
> Is there a motivation for the seemingly independent change from use of
> tls_align to use of p->tls.align here?
to 'satisfy the formula' which uses p_align, using the max
alignment seemed unnecessary overalignment.
if all modules follow the same formula and static linking
too then you only have to prove that one thing to work.
> > #else
> > tls_offset += p->tls.size + p->tls.align - 1;
> > tls_offset -= (tls_offset + (uintptr_t)p->tls.image)
> > @@ -1797,10 +1797,9 @@ _Noreturn void __dls3(size_t *sp)
> > app.tls_id = tls_cnt = 1;
> > #ifdef TLS_ABOVE_TP
> > app.tls.offset = GAP_ABOVE_TP;
> > - app.tls.offset += -GAP_ABOVE_TP & (app.tls.align-1);
> > - tls_offset = app.tls.offset + app.tls.size
> > - + ( -((uintptr_t)app.tls.image + app.tls.size)
> > - & (app.tls.align-1) );
> > + app.tls.offset += (-GAP_ABOVE_TP + (uintptr_t)app.tls.image)
> > + & (app.tls.align-1);
> > + tls_offset = app.tls.offset + app.tls.size;
> > #else
> > tls_offset = app.tls.offset = app.tls.size
> > + ( -((uintptr_t)app.tls.image + app.tls.size)
> > diff --git a/src/env/__init_tls.c b/src/env/__init_tls.c
> > index 5f12500c..772baba3 100644
> > --- a/src/env/__init_tls.c
> > +++ b/src/env/__init_tls.c
> > @@ -115,7 +115,8 @@ static void static_init_tls(size_t *aux)
> > & (main_tls.align-1);
> > #ifdef TLS_ABOVE_TP
> > main_tls.offset = GAP_ABOVE_TP;
> > - main_tls.offset += -GAP_ABOVE_TP & (main_tls.align-1);
> > + main_tls.offset += (-GAP_ABOVE_TP + (uintptr_t)main_tls.image)
> > + & (main_tls.align-1);
> > #else
> > main_tls.offset = main_tls.size;
> > #endif
> > --
> > 2.21.0
> >
>
> I think you're probably right about all these things and I want to
> apply this, but I also want to understand it a bit better first.
>
> Rich
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.
