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
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 22500

Event Timeline

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'.

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.

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

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

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

This comment was removed by ian.

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.Mar 29 2019, 5:53 PM
This revision was automatically updated to reflect the committed changes.