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

Mirror clang's link stage on Windows. #866

Closed
wants to merge 2 commits into from

Conversation

dot-asm
Copy link
Contributor

@dot-asm dot-asm commented Sep 20, 2023

Fixes problem discussed in #811.

@ChrisDenton
Copy link
Member

Hm, unsure about the implementation of this. Since we're forcing this library to be included, we need to be careful that it doesn't override anything the user has implemented themselves. clang does this using -defaultlib:oldnames.lib to give the library lower priority than non-default libs.

@dot-asm
Copy link
Contributor Author

dot-asm commented Sep 24, 2023

cargo:rustc-link-arg=-defaultib:oldnames then. Deeper dive into -vvv logs for a multi-file project revealed that it's also a wrong spot for the link arguments. [As well as --target duplication thing.]

src/lib.rs Outdated Show resolved Hide resolved
@ChrisDenton
Copy link
Member

This could really use a test to make sure it works and isn't accidentally broken in the future.

@ChrisDenton
Copy link
Member

Also cc @thomcc as using rustc-link-arg would require bumping the MSRV to >=1.56 (October 2021).

@thomcc
Copy link
Member

thomcc commented Sep 25, 2023

That MSRV bump doesn't bother me, but I would really like someone to double-check this works. My recollection is that there's some issue with using rustc-link-arg like this.

@dot-asm
Copy link
Contributor Author

dot-asm commented Sep 25, 2023

Also cc @thomcc as using rustc-link-arg would require bumping the MSRV to >=1.56 (October 2021).

Hmm, that would probably be undesirable... Two points in the context. One should keep in mind that the suggested modification affects only "fringe" use cases, an MSVC target with $CC set to clang, as opposed to clang-cl, which by itself is error-prone in the most general case. With this in mind, secondly, since the directive is ignored by an older version, then it wouldn't pose a problem for users who use the older version.

@dot-asm
Copy link
Contributor Author

dot-asm commented Sep 27, 2023

Putting together a test case was challenging because one has to mix object files generated by cl and clang in the same executable, with the former smuggling in oldnames.lib through the linker directive. -Zl prevents it from doing this and thus shifts the burden of linking in its entirety on rustc. You can convince yourself that the test is affirmative by commenting the line that adds -defaultlib:oldnames link flag and noting that the cc-test fails to link with "unresolved external symbol strdup" message. Well, this is provided that you have clang on the %PATH%. Just in case, this is the case on Github Actions, which can be confirmed by noting test msvcrt_here ... ok in the log files.

Fixes problem discussed in rust-lang#811.

[Also deduplicate clang --target argument on Windows, because it's
passed later on in the "// Target flags" section.]
@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 3, 2023

Re-based just in case.

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.

3 participants