Disable zlib in LLVM on Haiku

[nielx]

PR #72696 enabled the option LLVM_ENABLE_ZLIB for the LLVM builds. Like NetBSD and aarch64-apple-darwin (see PR #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..

[rust-highfive]

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

Wow, okay, at this point I'm leaning towards disabling zlib entirely until we can provide a workaround, this is just too many platforms which are broken when enabling it.

Given that the original use case is other compilers generating compressed artifacts, I'm inclined to say that we should just revert the original enable.

@jethrogb - do you think that's reasonable? Do we know if those compilers generating compressed artifacts can be made to not do so?

I'm also nominating for T-compiler, to see what thoughts on this are with a larger set of people (though maybe we'll find some solution by Thursday anyway).

[jethrogb]

Do we know if those compilers generating compressed artifacts can be made to not do so?

For gcc I know you can pass a flag at compile time to avoid this, but the default behavior on some OSes (e.g. Ubuntu) is to generate the compressed sections. Not having a zlib-enabled lld results in poor user experience when trying to use code from multiple compilers in your projects.

I'm leaning towards disabling zlib entirely until we can provide a workaround, this is just too many platforms which are broken when enabling it.

The problems with zlib seem to mostly (only?) affect tier 2 and lower platforms. The problem also seems limited to however rustbuild includes LLVM/zlib in the link, because clearly zlib should “just work” on most platforms. I think we should take a tiered approach here with the goal to support zlib as much as possible. In particular, I don't think tier 1 platforms should have reduced functionality because tier 2/3 platforms don't have their rustbuild configured correctly.

Proposal:

Tier 1: Keep zlib and guarantee that it will link correctly on tier 1 platforms.
Tier 2: Ideally the NetBSD situation (the only outlier for tier 2) gets fixed and we offer the same guarantees as tier 1.
Tier 3: It's the platform maintainer's responsibility to fix rustbuild. Ideally this means they change rustbuild to properly link zlib. Someone with a good understanding of the platform should be able to get rustbuild working fairly quickly. Disabling zlib could be used as a temporary measure instead.

[mati865]

AFAICT doing something like https://github.com/rust-lang/rust/pull/75625/files#diff-b0767d8366aa7e0a83a847d53032dfd4R200 should fix that. I'll try with netbsd.

[nielx]

The problems with zlib seem to mostly (only?) affect tier 2 and lower platforms. The problem also seems limited to however rustbuild includes LLVM/zlib in the link, because clearly zlib should “just work” on most platforms. I think we should take a tiered approach here with the goal to support zlib as much as possible. In particular, I don't think tier 1 platforms should have reduced functionality because tier 2/3 platforms don't have their rustbuild configured correctly.

Proposal:

Tier 1: Keep zlib and guarantee that it will link correctly on tier 1 platforms.
Tier 2: Ideally the NetBSD situation (the only outlier for tier 2) gets fixed and we offer the same guarantees as tier 1.
Tier 3: It's the platform maintainer's responsibility to fix rustbuild. Ideally this means they change rustbuild to properly link zlib. Someone with a good understanding of the platform should be able to get rustbuild working fairly quickly. Disabling zlib could be used as a temporary measure instead.

So I am aware Haiku very much is a tier 3 platform. Like most platforms (if not all except windows), we do ship libz as a default. Patching linking to libz in src/librustc_llvm/build.rs like @mati865 links to sounds fine to me. For Haiku it is indeed a runtime issue, and a link command will probably fix that. I will write a fresh patch and build that overnight to see whether that will resolve the issue.

[nielx]

Like @mati865 suggested, I altered the build.rs for librust_llvm, and it works. The patch wass rather crude:

diff --git a/src/librustc_llvm/build.rs b/src/librustc_llvm/build.rs
index 21b8080714c..d8a2d302168 100644
--- a/src/librustc_llvm/build.rs
+++ b/src/librustc_llvm/build.rs
@@ -189,6 +189,10 @@ fn main() {

     if !is_crossed {
         cmd.arg("--system-libs");
+    } else if target.contains("haiku") {
+        // LLVM is built with LLVM_ENABLE_ZLIB as on, which means it should
+        // link to libz.so on Haiku.
+        println!("cargo:rustc-link-lib=z");
     }
     cmd.args(&components);

I am wondering whether:

1. Linking to libz should be made dependent on the variable LLVM_ENABLE_ZLIB actually being turned on.
2. Whether this will cause issues when cross-compiling with an out of tree build of LLVM (i.e. I don't know enough about rustbuild to determine whether this script is called and the variables are run the same way)

[mati865]

LLVM_ENABLE_ZLIB is passed directly to CMake so you cannot easily check if it was enabled right now.

[spastorino]

This was discussed during the last compiler meeting, removing the nomination.

[pnkfelix]

The specific outcome from the meeting can be summarized like so:

simulacrum: Anyway I think the main question for this meeting is whether we're okay with diverging behavior between tier 1 and 2 (and 3) without bugs filed to fix that

wesleywiser: This seems ok to me provided we document tier 1 support requires zlib support.

[nielx]

Good that we can fix it, but it is unclear to me whether the preference is to fix this by turning off LLVM_ENABLE_ZLIB for Haiku, or by patching build.rs (my preference is the latter).

More fundamentally, I wonder to what extend the problem appears because Haiku is always cross-compiled. And whether this is an issue for all cross-compiled rustc instances.

[Mark-Simulacrum]

@nielx

Linking to libz should be made dependent on the variable LLVM_ENABLE_ZLIB actually being turned on.

I don't think this is necessary, spuriously linking should be fine for now -- and realistically we shouldn't need to turn it off ever I think?

Whether this will cause issues when cross-compiling with an out of tree build of LLVM (i.e. I don't know enough about rustbuild to determine whether this script is called and the variables are run the same way)

I don't think it will. AFAIK, the zlib support in LLVM isn't used by Rust itself, just the LLD we ship -- so even if we are compiling with an LLVM that doesn't support zlib that'll likely just mean that the built rust-lld is less featureful but shouldn't affect Rust itself.

Presumably distros that ship non-zlib supporting LLVM also ship gcc and clang that don't produce zlib-needing binaries for linking, or at least if not, can be convinced to alter one or the other.

I would be happy to r+ the patch you suggested, presuming you've tested it.

[nielx]

@Mark-Simulacrum

Whether this will cause issues when cross-compiling with an out of tree build of LLVM (i.e. I don't know enough about rustbuild to determine whether this script is called and the variables are run the same way)

I don't think it will. AFAIK, the zlib support in LLVM isn't used by Rust itself, just the LLD we ship -- so even if we are compiling with an LLVM that doesn't support zlib that'll likely just mean that the built rust-lld is less featureful but shouldn't affect Rust itself.

Are you sure about this? Because I encountered the issue not during build, but during runtime (running rustc) where there were unresolved compress and uncompress symbols in librustc_driver.so. It looks to me like some of the symbols end up in rustc (and its libraries) itself.

Presumably distros that ship non-zlib supporting LLVM also ship gcc and clang that don't produce zlib-needing binaries for linking, or at least if not, can be convinced to alter one or the other.

I would be happy to r+ the patch you suggested, presuming you've tested it.

I think Haiku does not explicitly use compressed sections in elf libraries (I doubt our toolchain supports it), so we should be good. The patch was manually tested and resolved the issue of not being able to run rustc.

[Mark-Simulacrum]

Are you sure about this? Because I encountered the issue not during build, but during runtime (running rustc) where there were unresolved compress and uncompress symbols in librustc_driver.so. It looks to me like some of the symbols end up in rustc (and its libraries) itself.

Ah, right. Yes, that makes sense. In that case we probably don't want to link it in by default, since presumably it isn't going to be available everywhere, though doing it on particular platforms where it is reasonable to expect it might not be too bad. I'm not sure what platforms would qualify though.

Just to clarify, you think the PR in its current state is sufficient for Haiku? I'm happy to r+ it if so.

[nielx]

I will update this patch once PR #75713 lands.

[bors]

☔ The latest upstream changes (presumably #75713) made this pull request unmergeable. Please resolve the merge conflicts.

[nielx]

Updated patch that uses the same code path as netbsd.

[nielx]

@Mark-Simulacrum This change is ready for review.

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 0f6bcb7 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 0f6bcb7 with merge 80cacd7...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 80cacd7 to master...