Rework emulated TLS patch for Android #3
Conversation
include/llvm/ADT/Triple.h
Outdated
| @@ -682,7 +682,7 @@ class Triple { | |||
|
|
|||
| /// Tests whether the target uses emulated TLS as default. | |||
| bool hasDefaultEmulatedTLS() const { | |||
| return isAndroid() || isOSOpenBSD() || isWindowsCygwinEnvironment(); | |||
| return /* LDC isAndroid() ||*/ isOSOpenBSD() || isWindowsCygwinEnvironment(); | |||
There was a problem hiding this comment.
Can't we fix this on the LDC side? I noticed that there are llvm::TargetOptions::{EmulatedTLS,ExplicitEmulatedTLS}.
There was a problem hiding this comment.
Yeah, just being lazy in figuring out that mess, :) this is the easiest fix that works. Since we're the only ones using this LLVM, rather just do it this way.
There was a problem hiding this comment.
No, you're right. I was stupidly thinking that I'd have to fix it elsewhere in LLVM, and misread what you wrote as that. Best to fix it in LDC as you said, will do that.
| @@ -3664,6 +3664,9 @@ class TargetLowering : public TargetLoweringBase { | |||
| virtual SDValue LowerToTLSEmulatedModel(const GlobalAddressSDNode *GA, | |||
| SelectionDAG &DAG) const; | |||
|
|
|||
| virtual SDValue LowerToAndroidEmulatedTLSAddress(SDValue Op, SDValue Result, | |||
| SelectionDAG &DAG, bool is64bit) const; // LDC | |||
|
|
|||
There was a problem hiding this comment.
Wouldn't need to be virtual, right?
There was a problem hiding this comment.
Not sure, it's a cut-n-paste of LowerToTLSEmulatedModel above, which doesn't have multiple class implementations either. I have almost zero knowledge of the LLVM class hierarchy, just followed suit.
| @@ -264,6 +265,7 @@ static SectionKind getELFKindForNamedSection(StringRef Name, SectionKind K) { | |||
| return SectionKind::getThreadData(); | |||
|
|
|||
| if (Name == ".tbss" || | |||
| (TargetTriple.isAndroid() && Name == ".tcommon") || // LDC | |||
There was a problem hiding this comment.
I've only found a single occurrence of .tcommon, as section for the _tlsend symbol. Is this line (and the extra param) really only required because of that?
There was a problem hiding this comment.
Yes, it ensures that the final executable always has the _tlsend symbol after all emulated TLS symbols, believe it was a trick Walter used in the original version of this emulated TLS scheme for dmd with OS X, which I copied to ldc.
This reminds me though that I'll likely be taking this out once I finally get emulated TLS working with reading the ELF headers and ditching these delimiter symbols, so this will likely change before the final 1.12 release anyway.
|
Lgtm. I'd suggest force-pushing directly onto the |
|
I thought about removing the old version, but on the off chance someone already pulled this branch, I figured better not mess with the git history. We can always do that on a later branch. |
|
Alright. A short summary as commit msg would be nice, based on the druntime comment in
|
|
Sure, rebuilding LLVM now with the changes you suggested. |
The Bionic C library ignores thread-local data stored in the normal .tbss/.tdata ELF sections, which are marked with the SHF_TLS/STT_TLS flags. LDC rolls its own emulated TLS scheme for Android instead, by keeping TLS data in the .tdata/.tbss sections but removing the SHF_TLS/STT_TLS flags, and replacing direct access to these globals by a call to __tls_get_addr() (implemented in druntime's rt.sections_android). The function is expected to translate an address in the TLS static data to the corresponding address in the actual TLS dynamic per-thread data.
|
Made the two changes you suggested, third is pending getting rid of section delimiters later, and expanded the commit message. Running through all the stdlib tests again, so far so good. |
Switches all four Android targets- ARM, AArch64, x86, and x64- to the same approach for inserting
__tls_get_addrused by Dan for AArch64. Tested by cross-compiling from linux/x64 and natively running the stdlib tests on ARM and AArch64 and for x86 and x64 in an Anbox Android/x64 container, also natively built on Android/AArch64.This meant I tried out Android/x64 for the first time, pull forthcoming for druntime and will enable that Termux package with 1.12, for those running Termux in an x64 Chromebook.
LLVM 7 enables the built-in gcc-alike scheme for emulated TLS for Android by default, rather than having language frontends like clang or ldc enable it, so I
had to disable that firstnow disable it from ldc, pull forthcoming.