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

Stabilize raw_dylib (#[link(kind = "raw-dylib")]) #104218

Closed
wants to merge 1 commit into from

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Nov 9, 2022

This stabilizes the raw-dylib and link_ordinal features (#58713) for the remaining architecture: x86.

@rustbot
Copy link
Collaborator

rustbot commented Nov 9, 2022

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

Please see the contribution instructions for more information.

@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 Nov 9, 2022
@tbu- tbu- force-pushed the pr_stabilize_raw_dylib branch from af71c84 to 97dc3f9 Compare November 9, 2022 21:53
@tbu-
Copy link
Contributor Author

tbu- commented Nov 9, 2022

Stabilization proposed in #58713 (comment).

The remaining mentions of raw_dylib are in src/tools/rust-analyzer. I assumed that I should not modify that because rust-analyzer has its own repository.

@dpaoliello
Copy link
Contributor

r? @michaelwoerister

@rustbot rustbot assigned michaelwoerister and unassigned ehuss Nov 9, 2022
@rust-log-analyzer

This comment has been minimized.

This stabilizes the raw-dylib and link_ordinal features (rust-lang#58713) for the
remaining architecture: x86.
@tbu- tbu- force-pushed the pr_stabilize_raw_dylib branch from e981b46 to ec4d15d Compare November 10, 2022 00:37
@crlf0710 crlf0710 added the F-raw_dylib `#![feature(raw_dylib)]` label Nov 10, 2022
@michaelwoerister
Copy link
Member

michaelwoerister commented Nov 11, 2022

Thanks for the PR, @tbu-! I'll take a closer look on Monday -- I'll need some time to get up-to-speed with the current state of things.

@michaelwoerister
Copy link
Member

Thanks for the PR, @tbu-!

I have some comments:

@dpaoliello
Copy link
Contributor

One more issue blocking stabilization for x86 (I'm currently looking into it): #104453

@kennykerr
Copy link
Contributor

Cross-compilation would also block windows-rs from going beyond experimental support. #103939

@mati865
Copy link
Contributor

mati865 commented Nov 15, 2022

@michaelwoerister I think there is one more problem related to the mentioned issue that maybe even should have blocked stabilization on 64-bit platforms.
rustc has dlltool flag to override path to dlltool binary which could have been used as a workaround to this issue. Sound cool but that flag is unstable (-Z dlltool) so on windows-gnu users can override things like linker but are stuck with first dlltool binary that is found in PATH with no ability to change it.
Both current dlltool hack and this flag are "temporary" until Rust can declare minimal supported Binutils version for windows-gnu recent enough it contains fix allowing it to link against short import libraries created by LLVM (this fix will be included in upcoming 2.40 version). Then the hack will become obsolete since all targets would use LLVM to generate import libs and and the flag will become obsolete.

@michaelwoerister
Copy link
Member

Yes, good catch @mati865! I came across that too while taking a look at #103939.

As far as I can tell, the plan has always been to also stabilize the -C dlltool commandline option (unless we come up with a foolproof way of always finding the correct dlltool -- which we didn't 😉). When the option was introduced in #90782, I asked on Zulip what the procedure would be here. That feedback was that we can just make the flag part of the stabilization FCP. So, I think we just have to make it visible in the stabilization report, that -Z dlltool becomes -C dlltool.

But one open question there is if rustc should always use the specified dlltool if the option is explicitly set, even when it would normally go through LLVM. I think the answer is yes, because otherwise it would be confusing.

@bjorn3
Copy link
Member

bjorn3 commented Nov 17, 2022

I think -Cdlltool should only be respected if rustc itself can't do it. This is similar to how we used to invoke an external archiver that you could specify using -Car, but have since deprecated this option and started ignoring it's value since rustc can do it on it's own now.

@michaelwoerister
Copy link
Member

I think that would be confusing, no? If I explicitly add -C dlltool to my rustc invocation, I would expect it to use the tool I specified unconditionally (as opposed to "just some of the time under conditions that are undocumented implementation details"). My expectations would be different if the commandline option was called something like -C fallback-dlltool.

@bjorn3
Copy link
Member

bjorn3 commented Nov 17, 2022

My suggestion is to stabilize -Cdlltool for now, but deprecate and ignore it once rustc has builtin support. This won't be the first case where rustc temporarily used an external tool until we gained builtin support. Keeping the external tool support only harms maintainability without any advantages that I know of.

@michaelwoerister
Copy link
Member

OK, I have no strong opinion here. Being able to remove the entire dlltool logic at some point does seem like an advantage. We just have to make sure to clearly document what the commandline option really does.

@bors
Copy link
Contributor

bors commented Nov 28, 2022

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

@apiraino
Copy link
Contributor

apiraino commented Jan 5, 2023

hello, checking for progress. With regard to this comment and perhaps also this comment, can the update situation be posted here?

Namely if there is some more design/discussion needed or the review con move forward. I'll switch the review status to author for a feedback. @tbu- feel free to request a review with @rustbot ready, thanks!

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2023
@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 5, 2023
@Dylan-DPC
Copy link
Member

Closing this as inactive. Feel free to create a new PR if interested in continuing with this.

@Dylan-DPC Dylan-DPC closed this Mar 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2023
Fix cross-compiling with dlltool for raw-dylib

Fix for rust-lang#103939

Issue Details:
When attempting to cross-compile using the `raw-dylib` feature and the GNU toolchain, rustc would attempt to find a cross-compiling version of dlltool (e.g., `i686-w64-mingw32-dlltool`). The has two issues 1) on Windows dlltool is always `dlltool` (no cross-compiling named versions exist) and 2) it only supported compiling to i686 and x86_64 resulting in ARM 32 and 64 compiling as x86_64.

Fix Details:
* On Windows always use the normal `dlltool` binary.
* Add the ARM64 cross-compiling dlltool name (support for this is coming: https://sourceware.org/bugzilla/show_bug.cgi?id=29964)
* Provide the `-m` argument to dlltool to indicate the target machine type.

(This is the first of two PRs to fix the remaining issues for the `raw-dylib` feature (rust-lang#58713) that is blocking stabilization (rust-lang#104218))
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 1, 2023
…ister,wesleywiser

Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool

This stabilizes the `raw-dylib` feature (rust-lang#58713) for all architectures (i.e., `x86` as it is already stable for all other architectures).

Changes:
* Permit the use of the `raw-dylib` link kind for x86, the `link_ordinal` attribute and the `import_name_type` key for the `link` attribute.
* Mark the `raw_dylib` feature as stable.
* Stabilized the `-Zdlltool` argument as `-Cdlltool`.
* Note the path to `dlltool` if invoking it failed (we don't need to do this if `dlltool` returns an error since it prints its path in the error message).
* Adds tests for `-Cdlltool`.
* Adds tests for being unable to find the dlltool executable, and dlltool failing.
* Fixes a bug where we were checking the exit code of dlltool to see if it failed, but dlltool always returns 0 (indicating success), so instead we need to check if anything was written to `stderr`.

NOTE: As previously noted (rust-lang#104218 (comment)) using dlltool within rustc is temporary, but this is not the first time that Rust has added a temporary tool use and argument: rust-lang#104218 (comment)

Big thanks to `@tbu-` for the first version of this PR (rust-lang#104218)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 2, 2023
…ister,wesleywiser

Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool

This stabilizes the `raw-dylib` feature (rust-lang#58713) for all architectures (i.e., `x86` as it is already stable for all other architectures).

Changes:
* Permit the use of the `raw-dylib` link kind for x86, the `link_ordinal` attribute and the `import_name_type` key for the `link` attribute.
* Mark the `raw_dylib` feature as stable.
* Stabilized the `-Zdlltool` argument as `-Cdlltool`.
* Note the path to `dlltool` if invoking it failed (we don't need to do this if `dlltool` returns an error since it prints its path in the error message).
* Adds tests for `-Cdlltool`.
* Adds tests for being unable to find the dlltool executable, and dlltool failing.
* Fixes a bug where we were checking the exit code of dlltool to see if it failed, but dlltool always returns 0 (indicating success), so instead we need to check if anything was written to `stderr`.

NOTE: As previously noted (rust-lang#104218 (comment)) using dlltool within rustc is temporary, but this is not the first time that Rust has added a temporary tool use and argument: rust-lang#104218 (comment)

Big thanks to ``@tbu-`` for the first version of this PR (rust-lang#104218)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 3, 2023
…ister,wesleywiser

Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool

This stabilizes the `raw-dylib` feature (rust-lang#58713) for all architectures (i.e., `x86` as it is already stable for all other architectures).

Changes:
* Permit the use of the `raw-dylib` link kind for x86, the `link_ordinal` attribute and the `import_name_type` key for the `link` attribute.
* Mark the `raw_dylib` feature as stable.
* Stabilized the `-Zdlltool` argument as `-Cdlltool`.
* Note the path to `dlltool` if invoking it failed (we don't need to do this if `dlltool` returns an error since it prints its path in the error message).
* Adds tests for `-Cdlltool`.
* Adds tests for being unable to find the dlltool executable, and dlltool failing.
* Fixes a bug where we were checking the exit code of dlltool to see if it failed, but dlltool always returns 0 (indicating success), so instead we need to check if anything was written to `stderr`.

NOTE: As previously noted (rust-lang#104218 (comment)) using dlltool within rustc is temporary, but this is not the first time that Rust has added a temporary tool use and argument: rust-lang#104218 (comment)

Big thanks to ```@tbu-``` for the first version of this PR (rust-lang#104218)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 3, 2023
…ister,wesleywiser

Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool

This stabilizes the `raw-dylib` feature (rust-lang#58713) for all architectures (i.e., `x86` as it is already stable for all other architectures).

Changes:
* Permit the use of the `raw-dylib` link kind for x86, the `link_ordinal` attribute and the `import_name_type` key for the `link` attribute.
* Mark the `raw_dylib` feature as stable.
* Stabilized the `-Zdlltool` argument as `-Cdlltool`.
* Note the path to `dlltool` if invoking it failed (we don't need to do this if `dlltool` returns an error since it prints its path in the error message).
* Adds tests for `-Cdlltool`.
* Adds tests for being unable to find the dlltool executable, and dlltool failing.
* Fixes a bug where we were checking the exit code of dlltool to see if it failed, but dlltool always returns 0 (indicating success), so instead we need to check if anything was written to `stderr`.

NOTE: As previously noted (rust-lang#104218 (comment)) using dlltool within rustc is temporary, but this is not the first time that Rust has added a temporary tool use and argument: rust-lang#104218 (comment)

Big thanks to ````@tbu-```` for the first version of this PR (rust-lang#104218)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 3, 2023
…ister,wesleywiser

Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool

This stabilizes the `raw-dylib` feature (rust-lang#58713) for all architectures (i.e., `x86` as it is already stable for all other architectures).

Changes:
* Permit the use of the `raw-dylib` link kind for x86, the `link_ordinal` attribute and the `import_name_type` key for the `link` attribute.
* Mark the `raw_dylib` feature as stable.
* Stabilized the `-Zdlltool` argument as `-Cdlltool`.
* Note the path to `dlltool` if invoking it failed (we don't need to do this if `dlltool` returns an error since it prints its path in the error message).
* Adds tests for `-Cdlltool`.
* Adds tests for being unable to find the dlltool executable, and dlltool failing.
* Fixes a bug where we were checking the exit code of dlltool to see if it failed, but dlltool always returns 0 (indicating success), so instead we need to check if anything was written to `stderr`.

NOTE: As previously noted (rust-lang#104218 (comment)) using dlltool within rustc is temporary, but this is not the first time that Rust has added a temporary tool use and argument: rust-lang#104218 (comment)

Big thanks to `````@tbu-````` for the first version of this PR (rust-lang#104218)
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 6, 2023
…ister,wesleywiser

Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool

This stabilizes the `raw-dylib` feature (rust-lang#58713) for all architectures (i.e., `x86` as it is already stable for all other architectures).

Changes:
* Permit the use of the `raw-dylib` link kind for x86, the `link_ordinal` attribute and the `import_name_type` key for the `link` attribute.
* Mark the `raw_dylib` feature as stable.
* Stabilized the `-Zdlltool` argument as `-Cdlltool`.
* Note the path to `dlltool` if invoking it failed (we don't need to do this if `dlltool` returns an error since it prints its path in the error message).
* Adds tests for `-Cdlltool`.
* Adds tests for being unable to find the dlltool executable, and dlltool failing.
* Fixes a bug where we were checking the exit code of dlltool to see if it failed, but dlltool always returns 0 (indicating success), so instead we need to check if anything was written to `stderr`.

NOTE: As previously noted (rust-lang#104218 (comment)) using dlltool within rustc is temporary, but this is not the first time that Rust has added a temporary tool use and argument: rust-lang#104218 (comment)

Big thanks to ``````@tbu-`````` for the first version of this PR (rust-lang#104218)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-raw_dylib `#![feature(raw_dylib)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.