Opened 7 years ago

Closed 5 years ago

#52585 closed enhancement (fixed)

libcxxabi -- attempts to add thread-local-storage (TLS) to <10.7

Reported by: ken-cunningham-webuse Owned by: kencu (Ken)
Priority: Normal Milestone:
Component: ports Version: 2.3.4
Keywords: Cc:
Port: libcxxabi

Description (last modified by kencu (Ken))

As per the mailing list discussion, a few ports are requesting TLS which is not available on systems prior to 10.7. This functionality is implemented in libcxxabi, and there are current efforts to provide a fallback mechanism for this detailed on the llvm website's libcxxxabi pages. < https://reviews.llvm.org/D21803>.

This ticket hopes to follow the attempts to get this to work.

First thing I notice is that the patches above appear to apply to libcxxabi @3.9.0, and we're currently running @3.7.0. I first tried to build @3.9.0 by updating the portfile, but ran into some attempts to rebuild clang-3.7 which I assume to be the dependency cycle mentioned in the libcxxabi portfile. I could drop back to clang-3.4 and try to build it, I presume -- but rather than do that, I attempted to backport the changes into libcxxabi @3.7.0 instead.

Almost all the changes are in one file, src/cxa_thread_atexit.cpp, which is basically completely rewritten. I was unable to apply the patch from the website cleanly due to changes in that file from @3.7.0, so I did it manually, and hopefully without error. The changes cxa_thread_atexit.cpp file is attached.

Sadly, when building the replacement libcxxabi that is supposed to provide a fallback for TLS, I now get the error saying to build the file, the system requires TLS -- which of course is sort of a catch-22.

So, working on that idea further while I post progress so far.

Attachments (3)

cxa_thread_atexit.cpp (4.6 KB) - added by ken-cunningham-webuse 7 years ago.
replacement file for src/cxa_thread_atexit.cpp
cxa_thread_atexit-TLS.diff (4.9 KB) - added by ken-cunningham-webuse 7 years ago.
diff from existing @3.7.0 src/cxa_thread_atexit.cpp
main.log (63.7 KB) - added by ken-cunningham-webuse 7 years ago.
build log (failure) -- on 10.6 with libc++ upgrade

Download all attachments as: .zip

Change History (16)

Changed 7 years ago by ken-cunningham-webuse

Attachment: cxa_thread_atexit.cpp added

replacement file for src/cxa_thread_atexit.cpp

Changed 7 years ago by ken-cunningham-webuse

Attachment: cxa_thread_atexit-TLS.diff added

diff from existing @3.7.0 src/cxa_thread_atexit.cpp

Changed 7 years ago by ken-cunningham-webuse

Attachment: main.log added

build log (failure) -- on 10.6 with libc++ upgrade

comment:1 Changed 7 years ago by larryv (Lawrence Velázquez)

As the fallback implementation has already been committed to libc++abi trunk and will be released at some point (probably in 4.0), I think backporting it would be a waste of time. It would be more useful to sort out the build issues with 3.9, since those will have to be addressed for 4.0 anyway.

comment:2 Changed 7 years ago by ken-cunningham-webuse

Hey -- they committed the patch 6 hours ago! You're on top of things!

If jeremy is about to update libcxxabi anytime soon, then I'll wait for that. I didn't think that was on his (exceptionally full) dance card at the moment, though, and he had suggested to see if I could do it this way. -- K

Last edited 7 years ago by ken-cunningham-webuse (previous) (diff)

comment:3 Changed 7 years ago by ken-cunningham-webuse

Thinking about it 10 minutes more (which I tell myself I always should do before replying, of course) .. perhaps I will take your suggestion then, and devote the effort to building 3.9.0 -- you're quite right, it would be a more efficient use of time. And I'm not sure that it will work in @3.7.0 anyway, as there are many changes since then. Thanks!

comment:4 Changed 7 years ago by jeremyhu (Jeremy Huddleston Sequoia)

I haven’t spent too much time trying to update libcxxabi and libcxx to newer builds, but I agree that is a better approach than backporting. IIRC, 3.8.0 had an issue when I tried it and I just never had time to dig into it.

comment:5 Changed 7 years ago by kencu (Ken)

so far I've had no luck with this using libcxx with 10.6; something I saw in the comments on the referenced page above suggested it may not be possible.

You'll have to guard it against all the platforms that don't support TLS. Darwin 10.6 is one of them.

Perhaps of some interest, libgcc's stdlibc++ does appear to support TLS on 10.6 (successfully building and running the software I was trying use, glbinding, so there is something to ponder if one really needs TLS and is prepared to go through a few hoops.

Last edited 7 years ago by kencu (Ken) (previous) (diff)

comment:6 Changed 6 years ago by kencu (Ken)

I think we have this fixed now. Some modifications to clang-5.0 appear to have done the trick, and the thread_local stuff looks like it's working on 10.6.8 now (should also work on 10.4 and 10.5):

$ clang++ --version
clang version 5.0.1 (tags/RELEASE_501/final)
Target: x86_64-apple-darwin10.8.0
Thread model: posix
InstalledDir: /opt/local/libexec/llvm-5.0/bin
$ cat thread.cpp
#include <iostream>
#include <string>
#include <thread>
#include <mutex>

thread_local unsigned int rage = 1; 

std::mutex cout_mutex;
 
void increase_rage(const std::string& thread_name)
{
    ++rage; // modifying outside a lock is okay; this is a thread-local variable
    std::lock_guard<std::mutex> lock(cout_mutex);
    std::cout << "Rage counter for " << thread_name << ": " << rage << '\n';
}
 
int main()
{
    std::thread a(increase_rage, "a"), b(increase_rage, "b");
 
    {
        std::lock_guard<std::mutex> lock(cout_mutex);
        std::cout << "Rage counter for main: " << rage << '\n';
    }
 
    a.join();
    b.join();
}
$ clang++ -std=c++11 -o thread thread.cpp
$ ./thread
Rage counter for main: 1
Rage counter for a: 2
Rage counter for b: 2

comment:7 Changed 6 years ago by kencu (Ken)

Well better, but not perfect. Sometimes, even though we're using -femulated-tls, a _tlv_atexit or _tlv_init still slips into the code, and doesn't find a link.

I'm not sure why this happens inconsistently just now, but I wonder if it might have something to do with constructs such as this, in llvm-5.0.1.src/tools/clang/lib/CodeGen/ItaniumCXXABI.cpp:

const char *Name = "__cxa_atexit";
  if (TLS) {
    const llvm::Triple &T = CGF.getTarget().getTriple();
    Name = T.isOSDarwin() ?  "_tlv_atexit" : "__cxa_thread_atexit";
  }

It looks like on Darwin, _tlv_* codes might be assumed in certain places...

comment:8 Changed 6 years ago by kencu (Ken)

I think I can see where this comes in. If you have thead_local variables that have non-trivial destructors, then we're still missing some functionality in 10.6.8 and lower.

e.g. building FileZilla

this:

thread_local std::map<std::wstring, std::unique_ptr<wxCSConv>> converters_;
thread_local std::vector<char> toServerBuffer_;
thread_local std::vector<wchar_t> toLocalBuffer_;

generates a link-time error:

Undefined symbols for architecture x86_64:
  "__tlv_atexit", referenced from:
      ___tls_init in filezilla-encoding_converter.o
ld: symbol(s) not found for architecture x86_64

changing it to this as an experiement:

__thread std::map<std::wstring, std::unique_ptr<wxCSConv>> converters_;
__thread std::vector<char> toServerBuffer_;
__thread std::vector<wchar_t> toLocalBuffer_;

suggests what the issue might be:

encoding_converter.cpp:8:60: error: type of thread-local variable has non-trivial destruction
__thread std::map<std::wstring, std::unique_ptr<wxCSConv>> converters_;
                                                           ^
encoding_converter.cpp:8:60: note: use 'thread_local' to allow this
encoding_converter.cpp:9:28: error: type of thread-local variable has non-trivial destruction
__thread std::vector<char> toServerBuffer_;
                           ^
encoding_converter.cpp:9:28: note: use 'thread_local' to allow this
encoding_converter.cpp:10:31: error: type of thread-local variable has non-trivial destruction
__thread std::vector<wchar_t> toLocalBuffer_;
                              ^
encoding_converter.cpp:10:31: note: use 'thread_local' to allow this

We'll need cxa_thread_atexit I believe, and then fix up the call

Name = T.isOSDarwin() ?  "_tlv_atexit" : "__cxa_thread_atexit";

to point to it.

However cxa_thread_atexit is not presently compiled into libcxx on Darwin, and trying to add it to the build runs into some issues, as it requires a _thread-capable compiler, and presently won't build with clang-5.0 due to other issues.

Probably have to build libcxx with gcc6 or gcc7 to make this work.

comment:9 Changed 6 years ago by kencu (Ken)

Description: modified (diff)
Summary: libcxxabi -- attempts to add thread-local-storage (TLS) to <10.9libcxxabi -- attempts to add thread-local-storage (TLS) to <10.7

comment:10 Changed 6 years ago by kencu (Ken)

Hmm. Another possible approach...I see there is a clang option -fno-use-cxa-atexit that might help here. See <http://clang-developers.42468.n3.nabble.com/where-should-target-specific-flags-be-set-tt4035181.html#none> and <http://lists.llvm.org/pipermail/llvm-bugs/2011-February/016717.html>.

Assuming non-trivial C++ destructors still somehow work with this flag added, and if this works to fix the problem, it could be made the default easily enough.

comment:11 Changed 6 years ago by kencu (Ken)

Happy to report this looks to be working now.

I rebuilt libcxx adding in the cxa_thread_atexit file using the thread-enabled version of clang-5.0, and then once that was done, I rebuilt clang-5.0 with this modification:

Name = (T.isOSDarwin() && !T.isMacOSXVersionLT(10, 7)) ?  "_tlv_atexit" : "__cxa_thread_atexit";

and using that, everything builds nicely, no link errors found, and filezilla builds through to completion and seems to be working as it should.

Need to do some thorough testing, and then perhaps make this available to everyone.

comment:12 Changed 5 years ago by kencu (Ken)

Cc: jeremyhu removed
Owner: changed from macports-tickets@… to kencu
Status: newassigned
Type: defectenhancement

comment:13 Changed 5 years ago by kencu (Ken)

Resolution: fixed
Status: assignedclosed

Two years ... not terribly trivial, but we have this working now.

Note: See TracTickets for help on using tickets.