-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Enable LLVM zlib #72696
Enable LLVM zlib #72696
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
src/bootstrap/native.rs
Outdated
@@ -175,6 +174,12 @@ impl Step for Llvm { | |||
} | |||
} | |||
|
|||
if builder.config.lld_enabled { | |||
cfg.define("LLVM_ENABLE_ZLIB", "ON"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me if this setting has repercussions beyond LLD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you think of a reason we wouldn't want to support zlib even if lld isn't enabled? I imagine it's more broadly useful and not measurably slower to compile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the git history and couldn't find a reason for it being off. It may just have been a case of "we don't use it, so why enabled it?"
src/bootstrap/native.rs
Outdated
@@ -175,6 +174,12 @@ impl Step for Llvm { | |||
} | |||
} | |||
|
|||
if builder.config.lld_enabled { | |||
cfg.define("LLVM_ENABLE_ZLIB", "ON"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you think of a reason we wouldn't want to support zlib even if lld isn't enabled? I imagine it's more broadly useful and not measurably slower to compile.
@bors r+ |
📌 Commit 8541213c95ef77bd91f73c75771652710940fc9d has been approved by |
@bors r+ I think we raced there :) |
📌 Commit 44b5f53 has been approved by |
…acrum Enable LLVM zlib Compilers may generate ELF objects with compressed sections (although rustc currently doesn't do this). Currently, when linking these with `rust-lld`, you'll get this error: `rust-lld: error: ...: contains a compressed section, but zlib is not available` This enables zlib when building LLVM.
I think this is responsible for the failure in #72809 (comment)
|
@bors rollup=never |
Out of curiosity, can you elaborate a bit further on this? How can a program compiled with EDIT: this is from the SGX toolchain I guess, but can you elaborate on which parts of it that end up generating compressed sections? |
You're right the Rust compiler doesn't do this. But C compilers do, for example I believe GCC does this by default on Ubuntu 18.04. So if you're trying to link native code and you're using |
Who is the NetBSD maintainer? @gandro ? How would we install zlib in the NetBSD build container? |
cc @MartinHusemann @jakllsch @bgermann @Niacat since you had a NetBSD PR somewhat recently |
I do not think the problem is that zlib is not installed. The check for the header says: "Looking for zlib.h - found". Maybe it is a missing -lz linker flag. |
rust maintainer for NetBSD is @he32. i'm responsible for repackaging rust binary releases, however. zlib is part of the NetBSD base system so you shouldn't need to install anything. instead the problem seems to be that the -lz linker flag is missing. semi-relatedly from a NetBSD packaging point of view I'm wondering why rust binaries for NetBSD aren't linked statically, or at a minimum with --static-libstdc++ |
So which libraries need to be linked is determined by @Niacat Looks like NetBSD is actually specifically excluded from statically linking libstdc++. Although there's some other netbsd-specific stuff at https://github.com/rust-lang/rust/blob/master/src/librustc_llvm/build.rs#L270 . |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors r+ |
📌 Commit 8caa14f has been approved by |
Uhhm, this is all very cryptic and totally undebuggable (from my POV). To make it possible to file a bug against llvm upstream, what invocation of llvm-config is supposed to output -lz (and why?) |
It's not LLVM bug. rust/src/librustc_llvm/build.rs Lines 184 to 186 in 0a675c5
|
To be clear I'm happy to (in the future) accept a PR fixing this and changing things, but I don't want to block progress for fixes to tier-2 platforms that seem non trivial. |
According to the git history, passing |
@bors p=1 letting rollup=never PRs flush out |
☀️ Test successful - checks-azure |
(Rust now has a dynamic library dependence on zlib. (see rust-lang/rust#72696))
(Rust now has a dynamic library dependence on zlib. (see rust-lang/rust#72696))
PR rust-lang#72696 enabled the option LLVM_ENABLE_ZLIB for the LLVM builds. On Haiku, zlib is linked as a shared library. When cross-compiling LLVM, rustbuild should be instructed to explicitly linking to libz.
…ulacrum Disable zlib in LLVM on Haiku PR rust-lang#72696 enabled the option LLVM_ENABLE_ZLIB for the LLVM builds. Like NetBSD and aarch64-apple-darwin (see PR rust-lang#75500), the LLVM build system not explicitly linking to libz on these platforms cause issues. For Haiku, this meant the runtime loader complaining about undefined symbols..
PR rust-lang#72696 enabled the option LLVM_ENABLE_ZLIB for the LLVM builds. Like NetBSD and aarch64-apple-darwin (see PR rust-lang#75500), the LLVM build system not explicitly linking to libz on these platforms cause issues. For Haiku, this meant the runtime loader complaining about undefined symbols..
@jethrogb I've hit this while futzing around with DragonFly and Solaris builds as well. From what I can tell the issue is that in a cross compiling context (e.g. the NetBSD builds) the host I think it's potentially more appropriate to check for a cross compilation and link to e.g.
diff --git a/compiler/rustc_llvm/build.rs b/compiler/rustc_llvm/build.rs
index b0783d75..2bfc421b 100644
--- a/compiler/rustc_llvm/build.rs
+++ b/compiler/rustc_llvm/build.rs
@@ -251,7 +251,7 @@ fn main() {
} else if target.contains("windows-gnu") {
println!("cargo:rustc-link-lib=shell32");
println!("cargo:rustc-link-lib=uuid");
- } else if target.contains("netbsd") || target.contains("haiku") || target.contains("darwin") {
+ } else if is_crossed {
println!("cargo:rustc-link-lib=z");
}
cmd.args(&components); |
Compilers may generate ELF objects with compressed sections (although rustc currently doesn't do this). Currently, when linking these with
rust-lld
, you'll get this error:rust-lld: error: ...: contains a compressed section, but zlib is not available
This enables zlib when building LLVM.