Page MenuHomeFreeBSD

Add support for R_AARCH64_TLSDESC with a symbol, and R_AARCH64_TLS_TPREL64
ClosedPublic

Authored by andrew on Mar 31 2015, 5:03 PM.

Details

Summary

These are needed to allocate memory, and when accessing thread local data
across different ELF files.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

andrew updated this revision to Diff 4549.Mar 31 2015, 5:03 PM
andrew retitled this revision from to Add support for R_AARCH64_TLSDESC with a symbol, and R_AARCH64_TLS_TPREL64.
andrew updated this object.
andrew edited the test plan for this revision. (Show Details)
andrew added reviewers: kib, emaste.
kib added inline comments.Mar 31 2015, 7:07 PM
libexec/rtld-elf/aarch64/reloc.c
136 ↗(On Diff #4549)

Use xmalloc() wrapper to avoid manuall testing the allocation failure and provide standard error message.

175 ↗(On Diff #4549)

Please use _rtld_error().

177 ↗(On Diff #4549)

Could you, please, clarify the rules for sharing the tls_data items between threads ? Is it thread-private, or could it be that two threads try to perform rtld_tlsdesc_handle() simultaneously on the same tlsdesc ?

I am mostly wondering about read-lock, is it enough or the case needs write-lock due to modifications to tlsdesc.

Note that it is possible that symbol lookup returns different results for separate invocations.

256 ↗(On Diff #4549)

Hmm, I think you should pass lockstate down to tld_tlsdesc_handle() and then for find_symdef(), instead of recursively locking the bind lock. In fact,what you do could deadlock.

andrew added inline comments.Apr 2 2015, 4:39 PM
libexec/rtld-elf/aarch64/reloc.c
175 ↗(On Diff #4549)

I think we want to print the message set by _rtld_error in find_symdef and exit, something like die, but that's a static function in rtld.c.

177 ↗(On Diff #4549)

We need to look up the symbol to find it's offset from the thread pointer. The details are in [1], and in this case we are interested in TPREL(S+A) on page 23.

I will need to add the offset of the objects tls data. Currently it returns the offset within the objects data, but this will lead two objects pointing to the same data.

Two threads could call this if they both access the same thread local data concurrently. I'll change it to use a write lock to handle this case. Is the reloc_jmpslots case safe if I use the lock provided, or will I need to upgrade it to a write lock?

[1] http://infocenter.arm.com/help/topic/com.arm.doc.ihi0056c/IHI0056C_beta_aaelf64.pdf

205 ↗(On Diff #4549)

I think this will need to change to:
where[1] = obj->tlsindex + rela->r_addend;

256 ↗(On Diff #4549)

Ok

emaste added inline comments.Apr 2 2015, 4:52 PM
libexec/rtld-elf/aarch64/reloc.c
175 ↗(On Diff #4549)

I think I wanted something like die() in MIPS reloc.c too.

kib added inline comments.Apr 2 2015, 6:24 PM
libexec/rtld-elf/aarch64/reloc.c
175 ↗(On Diff #4549)

AFAIR, in MIPS case the use of die() is bogus.

Indeed, https://reviews.freebsd.org/D661

As usual, MIPS people do not care.

177 ↗(On Diff #4549)

Re-using the provided lockstate should be fine. reloc_jmpslots() is called either by _rtld(), when the process is guaranteed to be single-threaded, or from the dlopen(). In the later case, rtld_bind_lock is write-locked.

What worries me somewhat, is the use of rtld_tlsdesc_handle() from rtld_tlsdesc_dynamic(). I suppose you would need one more wrapper.

emaste added inline comments.Apr 2 2015, 6:33 PM
libexec/rtld-elf/aarch64/reloc.c
175 ↗(On Diff #4549)

Ah, indeed that is the change I was thinking about.

andrew added a comment.Apr 2 2015, 7:24 PM

I have an update that should fix all the issues, but need to handle the symbol lookup failure case before pushing it here.

libexec/rtld-elf/aarch64/reloc.c
175 ↗(On Diff #4549)

Is the above patch going in or do I need to do something similar here?

177 ↗(On Diff #4549)

I have a fix for this, but an unable to push it as I need to decide how best to handle the case above where I need something like die().

kib added inline comments.Apr 2 2015, 8:08 PM
libexec/rtld-elf/aarch64/reloc.c
175 ↗(On Diff #4549)

I think you need to expose die(). Now, with the -fvisibility=hidden being used on x86, I am much less sensitive to making functions non-static.

You could consider setting hidden for aarch64.

emaste added inline comments.Apr 2 2015, 8:17 PM
libexec/rtld-elf/aarch64/reloc.c
175 ↗(On Diff #4549)

I still think there's some value in renaming it _rtld_die too, from a grepability perspective. NetBSD's rtld for example calls it _rtld_die.

andrew updated this revision to Diff 4617.Apr 2 2015, 8:18 PM

Update based on comments

emaste edited edge metadata.Apr 2 2015, 8:34 PM

I'm fine with this change but would like to have @kostikbel's sign-off.

kib edited edge metadata.Apr 2 2015, 8:35 PM

No, please just expose die, probably as rtld_die(), without #ifdef hacks. If doing this, please make it a separate commit, which should be merged to stable/10 and 9.

That said, what I do not like right now is that dlopen() of somewhat incorrect dso would kill the process, instead of returning an error.

andrew added a comment.Apr 2 2015, 8:48 PM
In D2183#16, @kostikbel wrote:

That said, what I do not like right now is that dlopen() of somewhat incorrect dso would kill the process, instead of returning an error.

How? We only look up the symbol when either we try to access it through _rtld_tlsdesc_dynamic, or when LD_BIND_NOW is set.

kib added a comment.Apr 2 2015, 9:14 PM
In D2183#17, @andrew wrote:
In D2183#16, @kostikbel wrote:

That said, what I do not like right now is that dlopen() of somewhat incorrect dso would kill the process, instead of returning an error.

How? We only look up the symbol when either we try to access it through _rtld_tlsdesc_dynamic, or when LD_BIND_NOW is set.

Never mind, I was confused.

andrew updated this revision to Diff 4625.Apr 2 2015, 9:42 PM
andrew edited edge metadata.

Update for r281005

kib accepted this revision.Apr 2 2015, 10:06 PM
kib edited edge metadata.
This revision is now accepted and ready to land.Apr 2 2015, 10:06 PM
andrew closed this revision.Apr 3 2015, 9:36 AM
andrew updated this revision to Diff 4629.

Closed by commit rS281014 (authored by @andrew).