Fix #463 ; Debug build mode fixing in Windows #506
Conversation
| @@ -22,6 +22,11 @@ cfg_if! { | |||
| println!("cargo:rustc-link-search=native={}", &node_lib_path.display()); | |||
| println!("cargo:rustc-link-lib={}", &node_lib_file_path.file_stem().unwrap().to_str().unwrap()); | |||
|
|
|||
| if debug | |||
| { | |||
| println!("cargo:rustc-link-lib=msvcrtd"); | |||
There was a problem hiding this comment.
This doesn't appear to be impacting other compilers (tests are passing), but, just to be safe, can you add a cfg to target just msvc?
There was a problem hiding this comment.
👍 Yes I agree to your suggestion. I'll try to the fixup. Thank you.
There was a problem hiding this comment.
What is msvcrtd? Is that the debug mode of Microsoft's C runtime library?
|
Were you able to reproduce the integer overflow from the linked issue without this fix? |
|
@kjvalencik Ah... unfortunately(or maybe a good news), I could not reproduce the But, the fix works fine with these examples. And it's debuggable with msvc-debugger and lldb. Lines, variables, dynamic links, registers and step execution in a rust codes are looks good to me. I think maybe, that overflow error occered from the implementation difference of a memory allocation between msvcrt.lib and msvcrtd.lib . I'll happy if the original reporter @anurbol would be share the repro code. |
|
Hi @usagi, you've done great work, wow. I am sorry, I can neither share the code, because I forgot everything about this problem, and where the code is actually, nor I think it could be useful because the code was pretty huge and I never knew which exact part caused the problem. |
|
Any more thoughts on this? We're eagerly awaiting debug builds on Windows! |
|
Thank you for this submission @usagi! I think we should merge it, even if we aren't 100% sure all the issues are gone since this is only for debug builds. If those issues start to surface again we can just try to fix them, but they shouldn't break production systems. |
|
@usagi After we merge this PR and publish a new Neon version, do people still need to use any of the manual steps in #463 (comment) or can they just do |
|
@dherman Just do |
There was a problem hiding this comment.
This looks like pure win to me! The worst that can happen is there might still be bugs in the Windows debug builds, but we can deal with those as we come across them, and there seems to be no risk for production builds.
Thank you so much for the contribution, @usagi! ❤️
|
@usagi In retrospect, it seems worthwhile to add a comment to the line you added that links msvcrtd, since it's otherwise fairly mysterious. Would you be up for a quick followup PR to add the comment? |
|
@dherman I will PR in near future about what is the trick if I could find the time. |
|
This seems incorrect. Rust always uses the release CRT, so linking the debug CRT would conflict with that. Why was this solution preferred over simply building the C code in release on Windows as I did in my PR from a year ago that was ignored? |
|
Oh wow, thank you @retep998, and my apologies for overlooking your contribution. That never feels good. We do our best to keep on top of things but we don't always live up to our intentions. Maybe what we could do is, I can write a backout PR for this and then that should fix the merge conflicts for #400. That way we don't lose the history or attribution. @retep998 Just to make sure I understand: how would the conflicts manifest -- would it result in link-time or runtime errors? Any idea why @usagi's tests didn't run into any errors? Also, do you happen to have any links to info where I can learn about how and why Rust uses the release CRT in Windows? As always, I really appreciate you sharing your expertise in Windows and Rust! |
|
Depending on the order of inputs to the linker, this may result in all linked rust code using only the debug CRT, only the release CRT, or a mixture of both which could cause link errors or runtime UB. If the user depends on any other crates that also link to C code, then that C code may end up linking against the debug CRT despite having been built against the release CRT which could cause link errors or runtime UB. I know even if it doesn't cause a linker error, the linker will usually spew warnings, however Rust currently does not print linker output unless linking fails so there is no way to see those warnings. In the future Rust may gain the option to link to the debug CRT: rust-lang/rust#39016 |
|
Thanks @retep998! You saved us from some likely extremely hard to diagnose bugs down the road. I’ll submit a backout PR and then see if your PR can be merged. |
See #506 (comment) for context.
Related: #463 (comment)