Autodetect the type of allocator crate used #44133
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (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. |
arielb1
added
the
S-waiting-on-review
|
Thanks for the PR! I wonder if we might reorganize this a little slightly though? I'd imagine that we'd select an crate to be an allocator in which case we'd have a |
|
Thank you for the answer. I guess that makes sense and I'll look into it. Some small questions, though:
|
|
These may be ok with the same prefix, I forget at this point :(. If the tests pass then should be good! And yeah sure squashing commits and whatnot is fine |
|
The prefixes are needed. It seems sometimes both kinds are linked in sometimes :-|. I don't feel like changing that. Anyway, I made some more changes here, tried to reorder the code a bit. There's still a small bit I'm not sure about how to do ‒ can I get |
src/liballoc_system/lib.rs
Outdated
| #![cfg_attr(any(unix, target_os = "redox"), feature(libc))] | ||
| #![alloc_kind = "lib"] |
There was a problem hiding this comment.
Could this perhaps be called "rustc_alloc_kind" to clearly deliniate that it's rustc-specific?
src/librustc_metadata/creader.rs
Outdated
| } | ||
|
|
||
| None => { | ||
| // TODO: Is there a way to get the metadata for krate and unify this with the |
There was a problem hiding this comment.
AFAIK this can't be done, but this probably isn't that important to solve regardless?
|
Looks like some tests may be failing? |
|
Hmm. But they passed locally. I'll have to experiment a bit to see what's wrong :-|. Maybe, can the |
|
Hm. Apparently not caused by the name of the flag. |
|
CI's still failing? |
|
Yes, there must be some bug. I didn't find the time to look into it properly yet, I didn't even manage to reproduce it locally (does Anyway, I'll have a look once I get some time. |
|
Yes |
|
☔ The latest upstream changes (presumably #44418) made this pull request unmergeable. Please resolve the merge conflicts. |
Annotate the allocator crates (allocator_system, allocator_jemalloc) by the type of allocator they are. If one is requested as an exe allocator, detect its type by the flags. This has the effect that using this (de jure wrong) configuration in the target spec works instead of producing a really unhelpful and arcane linker error: "exe-allocation-crate": "alloc_system" Fixes #43524.
|
I'm trying something, if the checks pass. No need to review yet, I have a theory I want to confirm first. |
|
Good, the tests passed, so I think it is ready for another round of review. I apologize for rewriting the whole branch, so the latest changes aren't easily seen, but it kind of happened during solving the conflicts. The important change is in I'm not sure why that happens on the build bot and why it doesn't happen to me locally, but it seems the build bot sometimes links with the libraries before modification. Is that possible? Anyway, the fact that switching it around would explain why it failed previously with the error it did and why it doesn't fail now. |
|
@bors: r+ Looks good to me, thanks! |
|
📌 Commit 94297c6 has been approved by |
Autodetect the type of allocator crate used Annotate the allocator crates (allocator_system, allocator_jemalloc) by the type of allocator they are. If one is requested as an exe allocator, detect its type by the flags. This has the effect that using this (de jure wrong) configuration in the target spec works instead of producing a really unhelpful and arcane linker error: "exe-allocation-crate": "alloc_system" Fixes #43524. There are two yet unsolved FIXME's, I'll be glad for some advice on what to do with them.
|
☀️ Test successful - status-appveyor, status-travis |
Annotate the allocator crates (allocator_system, allocator_jemalloc) by the type of allocator they are. If one is requested as an exe allocator, detect its type by the flags.
This has the effect that using this (de jure wrong) configuration in the target spec works instead of producing a really unhelpful and arcane linker error:
"exe-allocation-crate": "alloc_system"
Fixes #43524.
There are two yet unsolved FIXME's, I'll be glad for some advice on what to do with them.