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

Link to Windows debug CRTs when crt-debug target_feature is present. #1433

Closed
wants to merge 1 commit into from

Conversation

jdm
Copy link

@jdm jdm commented Jul 9, 2019

Relies on rust-lang/rust#62534 in order to fix rust-lang/rust#39016.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (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.

@jdm
Copy link
Author

jdm commented Jul 9, 2019

This makes it possible to link a Rust binding crate against a C library that was compiled with /MTd or /MDd.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

What happens if the standard library is built to link to one, but this crate from crates.io is set to link to the other ?

What happens if a #[no_std] binary only uses this crate without using the standard library.

Also, this needs tests that show that this works.

@jdm
Copy link
Author

jdm commented Jul 9, 2019

@gnzlbg What happens in those cases when crt-static is used?

@jdm
Copy link
Author

jdm commented Jul 9, 2019

It's true; this change may not be enough because of the issue of building the standard library. I tried building https://github.com/servo/mozjs with a Cargo.toml patch for libc that replaced msvcrt with msvcrtd, and I still got the same error that I'm used to seeing:

 = note: lld-link: error: /failifmismatch: mismatch detected: 0 and 2 for key _ITERATOR_DEBUG_LEVEL

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

@jdm note that libc is built as part of libstd, and linked there, applying certain link flags, and that when using it from crates.io, how things actually should get linked will depend, on whether the binary is #[no_std] or not. If the binary links libstd, trying to override link flags here will probably cause incompatibilities. I'd recommend you to open an issue in rust-lang/rust to discuss whether this is the right way to enable this feature - or does such an issue already exist ?

@jdm
Copy link
Author

jdm commented Jul 9, 2019

I mentioned it in the original PR description: rust-lang/rust#39016

@ollie27
Copy link
Member

ollie27 commented Jul 9, 2019

@gnzlbg when using this crate from crates.io nothing get linked:

#[cfg(all(target_env = "msvc", feature = "rustc-dep-of-std"))] // " if "

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

@ollie27 so if one builds a #[no_std] binary, and adds the libc crate as a dependency, no C standard library will be linked ? That sounds like a bug.

@ollie27
Copy link
Member

ollie27 commented Jul 9, 2019

so if one builds a #[no_std] binary, and adds the libc crate as a dependency, no C standard library will be linked ?

Correct. I don't know if it's a bug or not but it looks like the same is true of musl:

libc/src/unix/mod.rs

Lines 299 to 305 in 6307a0b

} else if #[cfg(target_env = "musl")] {
#[cfg_attr(feature = "rustc-dep-of-std",
link(name = "c", kind = "static",
cfg(target_feature = "crt-static")))]
#[cfg_attr(feature = "rustc-dep-of-std",
link(name = "c", cfg(not(target_feature = "crt-static"))))]
extern {}

I guess this is just how crt-static works at the moment.

@alexcrichton
Copy link
Member

It looks like this was realized in the last few comments, but replying to this cc this PR won't have the desired effect until something like rust-lang/rust#62534 is also added.

@bors
Copy link
Contributor

bors commented Sep 13, 2019

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

@Bromeon
Copy link

Bromeon commented Aug 30, 2020

Has there been progress towards linking to MSVCRTD (debug version), as described in rust-lang/rust#39016?

If not, are there meanwhile any known workarounds?

@jdm
Copy link
Author

jdm commented Aug 31, 2020

No.

@petrochenkov
Copy link
Contributor

Update from the compiler side - libc shouldn't need any additional compiler support for this (rust-lang/rust#39016 (comment)), this PR should work as is.
It can even keep target_feature = "crt-debug" as a switch in theory.

@jdm
Copy link
Author

jdm commented Oct 1, 2020

I don't have access to a Windows development environment any more, so I won't be pushing forward on this work.

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.

rustc always links against non-debug Windows runtime
8 participants