Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openbsd: link with required system libraries #7380

Merged
merged 2 commits into from
Dec 15, 2020

Conversation

semarie
Copy link
Contributor

@semarie semarie commented Dec 10, 2020

and while compiling zig stage2, use c++ and c++abi too

…ss) when linking

and while compiling zig stage2, use c++ and c++abi too
@semarie
Copy link
Contributor Author

semarie commented Dec 10, 2020

it adds linking for pthread library and compiler_rt (from system) when linking zig executable. compiler_rt is needs (in addition from the version from zig itself) due to __emutls_get_address symbol which isn't provided by zig version.

it also adds c++ and c++abi libraries when compiling stage2

@semarie semarie changed the title openbsd: use -lpthread and system compiler_rt (for __emutls_get_address) when linking openbsd: link with required system libraries Dec 10, 2020
@LemonBoy
Copy link
Contributor

due to __emutls_get_address symbol which isn't provided by zig version.

OpenBSD has no support for native TLS? Colour me surprised!
Is there a technical reason for falling back to emutls?

compiler_rt is needs (in addition from the version from zig itself) due to __emutls_get_address symbol which isn't provided by zig version.

Unconditionally adding a dependency on the native compiler-rt is a big no no, but implementing emutls in pure Zig has a few outstanding problems (see #5921).
We could cut some corner and make the unix implementation (for OpenBSD and apparently also Android) weakly dependent on libc/pthread to avoid further linking problems...

@semarie
Copy link
Contributor Author

semarie commented Dec 10, 2020

I must confess that I have no real idea on why __emutls_get_address is required. From this thread it seems we are using it since LLVM import in base.

I will rework the PR to not include it. the rest of the code will be better upstreamed than in my local repo

@LemonBoy
Copy link
Contributor

So this enables the code. It makes -femulated-tls the default,
otherwise it will generated TLS relocations that we can't handle yet.
It is possible to specify -fno-emulated-tls if you really want to
generate those.

Hopefully the situation improved a bit in the past three years or so, can you check if that's the case?
If you hack ZigLLVMCreateTargetMachine to set this and this to zero in opt Zig will be forced to avoid the emission of emutls calls.

@semarie
Copy link
Contributor Author

semarie commented Dec 11, 2020

Hopefully the situation improved a bit in the past three years or so, can you check if that's the case?

TLS support is still partial. so it might depend of type of relocations used: PT_TLS is supported enough well for Go to use it for example, but STT_TLS support seems to be missing.

If you hack ZigLLVMCreateTargetMachine ...

I am unsure if it is need or not: the LLVM 11 libraries I am using are mostly upstream version (I only backported few required patches): so the default is still -fno-emulated-tls when using them to build something. But the libraries itself were compiled using local clang (with default to -femulated-tls). And others system libraries might need the symbol too. I will look futher.

@semarie
Copy link
Contributor Author

semarie commented Dec 11, 2020

it seems some changes were backported. LLVM 11 upstream version has -femulated-tls by default for OpenBSD. But forcing it to false in ZigLLVMCreateTargetMachine doesn't seems effective. I am rebuilding LLVM for checking.

@LemonBoy
Copy link
Contributor

ExplicitEmulatedTLS makes LLVM ignore the platform default value and take llvm::TargetOptions::EmulatedTLS instead as you can see here.

When the emulated TLS option is on LLVM won't even attempt to use the native relocations and friends, you may not even need the emulation at all.

@semarie
Copy link
Contributor Author

semarie commented Dec 12, 2020

the current support of TLS seems enough at least for simple tests. I am able to build a zig stage1 compiler which generate code without using emutls and which is functional.

does it makes sense to add proper code generation without emutls for OpenBSD in zig ? or should I keep that local for now ?

@semarie
Copy link
Contributor Author

semarie commented Dec 14, 2020

@LemonBoy ping ?

the PR should be fine as it.

eventually I could add an additionnal commit to add explicit no-emulated-tls in stage1, but it isn't strictly necessary:

diff 3f2389d807cd23104b653e88eabbda0884bacecc 13d8a1ac8eb044871e069aa72021d2e15083884e
blob - 9b1ab71e9a11a333a4830580651b3de10e84a9d1
blob + b26db72a77db6409bca190818baf243e614beed4
--- src/zig_llvm.cpp
+++ src/zig_llvm.cpp
@@ -99,7 +99,7 @@ static const bool assertions_on = true;
 static const bool assertions_on = false;
 #endif
 
-LLVMTargetMachineRef ZigLLVMCreateTargetMachine(LLVMTargetRef T, const char *Triple,
+LLVMTargetMachineRef ZigLLVMCreateTargetMachine(LLVMTargetRef T, const char *TripleName,
     const char *CPU, const char *Features, LLVMCodeGenOptLevel Level, LLVMRelocMode Reloc,
     LLVMCodeModel CodeModel, bool function_sections, ZigLLVMABIType float_abi, const char *abi_name)
 {
@@ -164,7 +164,14 @@ LLVMTargetMachineRef ZigLLVMCreateTargetMachine(LLVMTa
         opt.MCOptions.ABIName = abi_name;
     }
 
-    TargetMachine *TM = reinterpret_cast<Target*>(T)->createTargetMachine(Triple, CPU, Features, opt, RM, CM,
+    Triple triple(TripleName);
+    if ((ZigLLVM_OSType)triple.getOS() == ZigLLVM_OpenBSD) {
+	/* force to not use EmulatedTLS on OpenBSD */
+	opt.ExplicitEmulatedTLS = true;
+	opt.EmulatedTLS = false;
+    }
+    
+    TargetMachine *TM = reinterpret_cast<Target*>(T)->createTargetMachine(TripleName, CPU, Features, opt, RM, CM,
             OL, JIT);
     return reinterpret_cast<LLVMTargetMachineRef>(TM);
 }

@LemonBoy
Copy link
Contributor

LGTM.

I think we still need an emutls fallback to allow our compiler-rt to provide the required symbols in case a user is pulling in some object file compiled with system clang.

@andrewrk andrewrk merged commit 7cd6c25 into ziglang:master Dec 15, 2020
@semarie semarie deleted the openbsd-libs branch December 23, 2020 12:41
aarvay pushed a commit to aarvay/zig that referenced this pull request Jan 4, 2021
* openbsd: use -lpthread when linking

and while compiling zig stage2, use c++ and c++abi too
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants