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.

Sign up for GitHub

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

Jump to bottom

Use ar_archive_writer for writing COFF import libs on all backends #129164

Merged
merged 1 commit into from
Aug 18, 2024
Merged

Use ar_archive_writer for writing COFF import libs on all backends #129164

merged 1 commit into from
Aug 18, 2024

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Aug 16, 2024 •
edited by jieyouxu
Loading

This is mostly the same as the llvm backend but with the cranelift version copy/pasted in place of the LLVM library.

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: aarch64-gnu
try-job: aarch64-apple
try-job: test-various
try-job: armhf-gnu

@rustbot
Copy link
Collaborator

rustbot commented Aug 16, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 16, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 16, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@rust-log-analyzer

This comment has been minimized.

Sign in to view
jieyouxu
jieyouxu approved these changes Aug 16, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me. Do we want any tests (even just a sanity check) for this (in terms of integration w/ ar_archive_writer)? Or does ar_archive_writer have sufficient tests?

compiler/rustc_codegen_ssa/src/back/archive.rs Outdated Show resolved Hide resolved
@jieyouxu
Copy link
Member

Since this PR would also close the linked issue, is it possible to have a regression test?

@ChrisDenton
Copy link
Member Author

Do we want any tests (even just a sanity check) for this (in terms of integration w/ ar_archive_writer)?

I think this should be covered by our existing raw-dylib tests. They should fail if anything goes wrong.

Since this PR would also close the linked issue, is it possible to have a regression test?

I can try.

@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Aug 16, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 16, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@rust-log-analyzer

This comment has been minimized.

Sign in to view
jieyouxu
jieyouxu approved these changes Aug 17, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jieyouxu
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 17, 2024

📌 Commit c9451ac has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2024
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 17, 2024
@tgross35
Rollup merge of rust-lang#129164 - ChrisDenton:comdat, r=jieyouxu
533d508 
Use `ar_archive_writer` for writing COFF import libs on all backends

This is mostly the same as the llvm backend but with the cranelift version copy/pasted in place of the LLVM library.

Also updates ar_archive_writer to 0.4.0 which fixes rust-lang#129020
@tgross35 tgross35 mentioned this pull request Aug 17, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 17, 2024
@bors
Auto merge of rust-lang#129198 - tgross35:rollup-bxc17se, r=tgross35
150bacd 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#128786 (Detect multiple crate versions on method not found)
 - rust-lang#128982 (Re-enable more debuginfo tests on Windows)
 - rust-lang#128989 (Emit an error for invalid use of the linkage attribute)
 - rust-lang#129115 (Re-enable `dump-ice-to-disk` for Windows)
 - rust-lang#129164 (Use `ar_archive_writer` for writing COFF import libs on all backends)
 - rust-lang#129167 (mir/pretty: use `Option` instead of `Either<Once, Empty>`)
 - rust-lang#129168 (Return correct HirId when finding body owner in diagnostics)
 - rust-lang#129173 (Fix `is_val_statically_known` for floats)
 - rust-lang#129185 (Port `run-make/libtest-json/validate_json.py` to Rust)

r? `@ghost`
`@rustbot` modify labels: rollup
@ChrisDenton
Copy link
Member Author

Failed in rollup #129198 (comment)

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 17, 2024
@ChrisDenton
Copy link
Member Author

While I figure out what went wrong, let's make sure x86_64 passes

@bors try

@bors
Copy link
Contributor

bors commented Aug 17, 2024

⌛ Trying commit c9451ac with merge eb9ec80...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 17, 2024
@bors
Auto merge of rust-lang#129164 - ChrisDenton:comdat, r=<try>
eb9ec80 
Use `ar_archive_writer` for writing COFF import libs on all backends

This is mostly the same as the llvm backend but with the cranelift version copy/pasted in place of the LLVM library.

Also updates ar_archive_writer to 0.4.0 which fixes rust-lang#129020

try-job: x86_64-msvc
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Aug 17, 2024

💔 Test failed - checks-actions

@jieyouxu
Copy link
Member

... weird

@ChrisDenton
Copy link
Member Author

Hm, it seems to work on x86_64 with MSVC's link.exe but it seems there's problems with LLVM and i686. It might be that the need the .idata$3 object file to be just so. It might still be possible to workaround that limitation but not easily.

So I think I'll edit this to disable the comdat option and just land the unification of COFF writing for now. It might be a good idea to land them separately anyway.

@jieyouxu
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Aug 18, 2024

⌛ Trying commit 0156eb5 with merge f1672b9...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 18, 2024
@bors
Auto merge of rust-lang#129164 - ChrisDenton:comdat, r=<try>
f1672b9 
Use `ar_archive_writer` for writing COFF import libs on all backends

This is mostly the same as the llvm backend but with the cranelift version copy/pasted in place of the LLVM library.

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: aarch64-gnu
try-job: aarch64-apple
try-job: test-various
try-job: armhf-gnu
@bors
Copy link
Contributor

bors commented Aug 18, 2024

☀️ Try build successful - checks-actions
Build commit: f1672b9 (f1672b955d70942cc9a145a28e1a61cbf003b88b)

@jieyouxu
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 18, 2024

📌 Commit 0156eb5 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 18, 2024
@jieyouxu jieyouxu mentioned this pull request Aug 18, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 18, 2024
@bors
Auto merge of rust-lang#129225 - jieyouxu:rollup-xwtkwgr, r=jieyouxu
7521bda 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#129164 (Use `ar_archive_writer` for writing COFF import libs on all backends)
 - rust-lang#129173 (Fix `is_val_statically_known` for floats)
 - rust-lang#129185 (Port `run-make/libtest-json/validate_json.py` to Rust)
 - rust-lang#129203 (Use cnum for extern crate data key)
 - rust-lang#129221 (Remove JohnTitor from review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c4cf437 into rust-lang:master Aug 18, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 18, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 18, 2024
@rust-timer
Unrolled build for rust-lang#129164
b8aec82 
Rollup merge of rust-lang#129164 - ChrisDenton:comdat, r=jieyouxu

Use `ar_archive_writer` for writing COFF import libs on all backends

This is mostly the same as the llvm backend but with the cranelift version copy/pasted in place of the LLVM library.

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: aarch64-gnu
try-job: aarch64-apple
try-job: test-various
try-job: armhf-gnu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.82.0
Development

Successfully merging this pull request may close these issues.
6 participants
@ChrisDenton @rustbot @rust-log-analyzer @jieyouxu @bors @bjorn3