Page MenuHomeFreeBSD

Fix initial exec TLS mode for dynamically loaded shared objects.
ClosedPublic

Authored by kib on Feb 3 2019, 9:11 PM.

Details

Summary

If the DF_STATIC_TLS dynamic flag is set, rtld tries to allocate TLS in static space. If there is no space left, the dlopen(3) fails. If space if allocated, initial content from PT_TLS segment is distributed to all threads' pcbs.

See https://www.redhat.com/archives/phil-list/2003-February/msg00077.html for plain language explanation.
I am unsure about https://reviews.llvm.org/D33041, I did observed that lld 6.0.1 does not set DF_STATIC_TLS. IMO this is a bug.

The test case is https://github.com/dumbbell/test-tls-initial-exec. I only compiled and somewhat tested patch on amd64, and patch have untested bits for i386.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Feb 3 2019, 9:11 PM

Should we encourage lld to revive the https://reviews.llvm.org/D33041 patch?

kib added a comment.Feb 3 2019, 10:25 PM

Should we encourage lld to revive the https://reviews.llvm.org/D33041 patch?

Lets discuss it. They did not stated why DF_STATIC_TLS was considered as 'not needed'.

emaste added a comment.Feb 4 2019, 9:56 PM
In D19072#407799, @kib wrote:

Lets discuss it. They did not stated why DF_STATIC_TLS was considered as 'not needed'.

See the ongoing discussion in the LLVM thread - glibc and musl do not use the flag

glibc used to use it, but doesn't since 2002
(2430d57a13f4f10312e13c58962cd9104e6428fd).

As far as I can tell glibc originally used the flag only to signal that the .so should not be opened by dlopen, and the referenced commit changed it to checking explicitly for TLS use. Later changes for dlopen w/ static TLS did not check the flag.

kib updated this revision to Diff 53726.Feb 9 2019, 11:42 AM

Fix quite a bug in the loading of depended objects.
Restructure to avoid coding current thread offset calculation twice.

kib updated this revision to Diff 53919.Feb 14 2019, 2:17 PM

Provide implementations for all arches.

llvm commit in https://reviews.llvm.org/D57821, @dim reasonable to backport in your opinion?

llvm commit in https://reviews.llvm.org/D57821, @dim reasonable to backport in your opinion?

and https://reviews.llvm.org/D57749

dim added a comment.Feb 19 2019, 9:30 PM

llvm commit in https://reviews.llvm.org/D57821, @dim reasonable to backport in your opinion?

and https://reviews.llvm.org/D57749

Yes, those should be fine; can be imported independently of this revision.

ian added a subscriber: ian.Fri, Mar 29, 3:13 PM
This comment was removed by ian.
ian added a comment.Fri, Mar 29, 3:14 PM

I haven't reviewed this change, but I did run-test it (on armv7 only). I updated to r345693 then applied the patch from this review, and ran the test code from https://github.com/emaste/test-tls-initial-exec. The test failed before patching and worked afterwards.

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Mar 29, 5:53 PM
This revision was automatically updated to reflect the committed changes.