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

std: link to liballoc_system if compiled without the jemalloc feature #37975

Closed
wants to merge 2 commits into from

Conversation

japaric
Copy link
Member

@japaric japaric commented Nov 24, 2016

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@japaric
Copy link
Member Author

japaric commented Nov 24, 2016

With this and #37974, I can CROSS compile std using Cargo and the following Cargo.toml:

[package]

[dependencies.alloc_system]
path = "$RUST_SRC/src/liballoc_system"

[dependencies.panic_abort]
path = "$RUST_SRC/src/libpanic_abort"

[dependencies.std]
path = "$RUST_SRC/src/libstd"

[profile.dev]
panic = "abort"

[profile.release]
panic = "abort"

without any change to rust-src.

@alexcrichton
Copy link
Member

@bors: r+

Awesome progress!

@bors
Copy link
Contributor

bors commented Nov 24, 2016

📌 Commit eb78c71 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 25, 2016

⌛ Testing commit eb78c71 with merge 39577d0...

@bors
Copy link
Contributor

bors commented Nov 25, 2016

💔 Test failed - auto-mac-32-opt

@japaric
Copy link
Member Author

japaric commented Nov 30, 2016

This is blocking Xargo from being able to compile the std crate and I have no idea why those tests don't pass on 32-bit mac. (They pass on Linux)

@alexcrichton
Copy link
Member

Maybe this is affected by how tests are run? Maybe nondeterministic? I'm not sure :(

@japaric
Copy link
Member Author

japaric commented Dec 1, 2016

Maybe nondeterministic?

Want to retry?

(or let's 🔥jem🔥lloc🔥)

@alexcrichton
Copy link
Member

Hm if it is nondeterministic I'd be wary of landing b/c then it'd just turn into a problem I'd have to debug later... If you try running a few times this surely never reproduces on Linux?

@alexcrichton
Copy link
Member

Note that the nondeterminism is just a wild guess, I don't actually know what the problem is.

@HybridEidolon
Copy link

I built this locally for x86_64-apple-darwin and encountered the issue where it links alloc_system where it shouldn't in run-pass/allocator-override.rs and run-pass/allocator-default.rs, as it appears in the bors build error, so it is not a nondeterministic issue.

@HybridEidolon
Copy link

HybridEidolon commented Dec 23, 2016

This only fails on makefiles build for me; the tests build and run successfully using rustbuild, contingent on stage2 std being build with --cfg 'feature="jmalloc"'. Makefiles seem to not pass that argument to std build step but only on darwin targets. Since it's not there, libstd is built with a dependency on alloc_system with these changes, resulting in the multiple allocator crates panic. Why it does that I'm not sure, I'm no compiler hacker. Seems like there is a pre-existing inconsistency in the build systems.

Here is a patch (a simple one-liner) that fixes the test failures under darwin targets by setting RUSTFLAGS_std if CFG_DISABLE_JEMALLOC is not set, as it does for RUSTFLAGS_rustc_back. crates-patch.txt

@japaric
Copy link
Member Author

japaric commented Dec 23, 2016

That patch totally makes sense. Thanks for investigating, @HybridEidolon!

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Dec 23, 2016

📌 Commit fa10ef8 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Dec 23, 2016

⌛ Testing commit fa10ef8 with merge 52f48fe...

@bors
Copy link
Contributor

bors commented Dec 23, 2016

💔 Test failed - auto-win-msvc-64-opt

@HybridEidolon
Copy link

This isn't going to work on targets that do actually use alloc_system. libstd will have an alloc_system dependency and crates that override the default allocator and are not no_std will result in an allocator collision during compile. I am not sure this is the right approach anymore.

@aidanhs
Copy link
Member

aidanhs commented Jan 15, 2017

The new logic seems wrong. As @HybridEidolon says, if you apply this patch disabling jemalloc means you can never drop in another allocator crate because std has a hardcoded dependency on liballoc_system. That's a bit sad.

Ideally I think it'd be nice to specify an allocator preference in Cargo.toml/cargo command line (used when an allocator must be picked i.e. dylibs, not rlibs), but since that's not possible we can make do with a force_alloc_system feature (which, as before, locks you into a single allocator for stage0). Crucially, this feature is distinct from the jemalloc feature - we don't want someone to be forced into alloc_system just for disabling jemalloc!

See #39086

@alexcrichton
Copy link
Member

I'm going to close this due to inactivity, but it'd be great to land a fix for this still!

bors added a commit that referenced this pull request Jan 21, 2017
…crichton

Make rustbuild force_alloc_system rather than relying on stage0

This 'fixes' jemalloc-less local rebuilds, where we tell cargo that we're actually stage1 (this only fixes the rustbuild path, since I wasn't enthusiastic to dive into the makefiles).

There should be one effect from this PR: `--enable-local-rebuild --disable-jemalloc` will successfully build a stage0 std (rather than erroring). Ideally I think it'd be nice to specify an allocator preference in Cargo.toml/cargo command line (used when an allocator must be picked i.e. dylibs, not rlibs), but since that's not possible we can make do with a force_alloc_system feature. Sadly this locks you into a single allocator in the build libstd, making any eventual implementation of #38575 not quite right in this edge case, but clearly not many people exercise the combination of these two flags.

This PR is also a substitute for #37975 I think. The crucial difference is that the feature name here is distinct from the jemalloc feature (reused in the previous PR) - we don't want someone to be forced into alloc_system just for disabling jemalloc!

Fixes #39054

r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error cargo building std as a dylib
6 participants