-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
[experiment] Support linking from a .rlink file #68487
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
r? @tmandry |
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 |
src/librustc_codegen_llvm/lib.rs
Outdated
|
||
// Now that we won't touch anything in the incremental compilation directory | ||
// any more, we can finalize it (which involves renaming it) | ||
rustc_incremental::finalize_session_directory(sess, codegen_results.crate_hash); |
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.
This is already performed when creating the .rlink
file. I think it can be moved before linking for join_codegen_and_link
too.
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.
Will do once #68601 is merged.
src/librustc_session/config.rs
Outdated
match *self { | ||
Input::File(ref ifile) => { | ||
if let Some(extension) = ifile.extension() { | ||
extension == RLINK_EXT |
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.
This is technically a breaking change, as a source file may have .rlink
as extension. Checking if the file starts with {
or magic bytes (eg b"rlink", should be prepended before writing it) will fix this.
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'm not sure if this is still necessary as the feature is now gated by -Z link-only
.
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 think automatically detecting the .rlink
file will be desired when we stabilize this feature; however, we will likely change the .rlink
file to a binary at that time. So I prefer not to include automatic file format detection at this stage.
src/librustc_driver/lib.rs
Outdated
@@ -593,6 +594,19 @@ fn show_content_with_pager(content: &String) { | |||
} | |||
|
|||
impl RustcDefaultCalls { | |||
pub fn process_rlink(sess: &Session, compiler: &interface::Compiler) -> Compilation { |
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.
This isn't feature gated. With a hand made json file it is possible to use this.
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.
Added new flag -Z link-only
☔ The latest upstream changes (presumably #68601) made this pull request unmergeable. Please resolve the merge conflicts. |
Rebased so that #68601 is included. |
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.
This doesn't support #![crate_type]
and #![crate_name]
with seperate linking. Because they are not used very often, I think it would be fine to fix it in a follow-up, or maybe deperecate them instead.
This is likely the last round of review from me.
@bors delegate=0dvictor |
Flag `-Z no-link` was previously introduced, which allows creating an `.rlink` file to perform compilation without linking. This change enables linking from an `.rlink` file.
I've squashed git history and all four checks passed. This PR is ready to go. @bors r+ |
@0dvictor: 🔑 Insufficient privileges: Not in reviewers |
@tmandry I think the correct syntax is |
@bjorn3 both syntax are correct but bors had downtime yesterday. |
@bors r+ |
📌 Commit a47fdb9 has been approved by |
[experiment] Support linking from a .rlink file Flag `-Z no-link` was previously introduced, which allows creating an `.rlink` file to perform compilation without linking. This change enables linking from an `.rlink` file. Part of Issue rust-lang#64191
Rollup of 11 pull requests Successful merges: - #67695 (Added dyn and true keyword docs) - #68487 ([experiment] Support linking from a .rlink file) - #68554 (Split lang_items to crates `rustc_hir` and `rustc_passes`.) - #68937 (Test failure of unchecked arithmetic intrinsics in const eval) - #68947 (Python script PEP8 style guide space formatting and minor Python source cleanup) - #68999 (remove dependency on itertools) - #69026 (Remove common usage pattern from `AllocRef`) - #69027 (Add missing `_zeroed` varants to `AllocRef`) - #69058 (Preparation for allocator aware `Box`) - #69070 (Add self to .mailmap) - #69077 (Fix outdated doc comment.) Failed merges: r? @ghost
Hooray! Next step: cargo support. |
Flag
-Z no-link
was previously introduced, which allows creating an.rlink
file to perform compilation without linking. This change enables linking from an.rlink
file.Part of Issue #64191