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

cc-rs link issue on windows msvc targets when using Clang with GNU cmdline syntax #395

Open
riidefi opened this issue May 26, 2023 · 10 comments

Comments

@riidefi
Copy link

riidefi commented May 26, 2023

In particular, the way Corrosion passes the CMake CC_/CXX_ to rust here causes issues when the rust linker attempts to link the binary with the MSVC linker. This affects libcurl_sys and many other -sys crates that use the C compiler Corrosion specifies.

The only change here is switching the target to cdylib (which forces a complete, not partial, link):

  = note:    Creating library C:/Users/rii/Documents/dev/RiiStudio/ws/./cargo/build\x86_64-pc-windows-msvc\release\deps\riistudio_rs.dll.lib and object C:/Users/rii/Documents/dev/RiiStudio/ws/./cargo/build\x86_64-pc-windows-msvc\release\deps\riistudio_rs.dll.exp
          libcurl_sys-7b6cbe17349096cb.rlib(digest_sspi.o) : error LNK2001: unresolved external symbol strdup
          libcurl_sys-7b6cbe17349096cb.rlib(vauth.o) : error LNK2001: unresolved external symbol strdup
          ...
          libcurl_sys-7b6cbe17349096cb.rlib(rename.o) : error LNK2001: unresolved external symbol strdup
          libcurl_sys-7b6cbe17349096cb.rlib(warnless.o) : error LNK2019: unresolved external symbol read referenced in function curlx_read
          libcurl_sys-7b6cbe17349096cb.rlib(warnless.o) : error LNK2019: unresolved external symbol write referenced in function curlx_write
          ...

A run with this issue can be seen here from this commit.

There are a few workarounds I've found:

  • Switch to clang-cl (not viable)
  • Avoid libraries like libcurl_sys (not viable)
  • Manually pass the MSVC compiler as CC to override CC_ (hacky)
  • Disable passing CC_ altogether (hacky, but works and seen here and fixes the build on CI seen above with this CI commit and CI run)

I believe the least annoying solution would be a flag to disable setting the CC_ rustflag altogether, and let rust use the system compiler directly. Alternatively, it seems if we want to pass CC, we may also need to pass the linker, though that appears less feasible.

Cheers

@riidefi riidefi changed the title cdylib broken on Clang + Windows cdylib broken on Clang + Windows (with workarounds) May 26, 2023
@jschwe
Copy link
Collaborator

jschwe commented May 26, 2023

A run with this issue can be seen here from this commit.

It appears you are using a rather old release of corrosion (v3.2). Did you test if this issue still exists on master?

In particular, the way Corrosion passes the CMake CC_/CXX_ to rust here causes issues when the rust linker attempts to link the binary with the MSVC linker

I'd like to have a better understanding of what exactly goes wrong.

Your setup seems to be the following from what I gathered:

  • Windows
  • CMake 3.xx?
  • Ninja Generator
  • MSVC ABI
  • clang compiler with GNU commandline syntax
  • You are not compiling any C-code in CMake (?), only via build-scripts, or at least that C-code is not linked into your rust crate, correct?
  • Linker invoked by rust for a cdylib. link.exe is invoked directly, but it probably should be lld-link instead, since the code was compiled with clang.
  • You did not add any global RUSTFLAGS or rustc flags, which would pass -C linker or -C linker-flavor to rust, to select lld-link.

If my understanding of your setup is correct, I'd ask you to:

  • test if the same issue occurs with corrosion on the master branch
  • if selecting lld-link as the linker solves the issue.

@riidefi
Copy link
Author

riidefi commented May 26, 2023

Thanks for the swift reply.

test if the same issue occurs with corrosion on the master branch

I apologize for not mentioning I had tried this locally. Here's a commit and CI run

if selecting lld-link as the linker solves the issue.

Unfortunately no dice: commit and CI run. (Note: ignore spurious "-Ctarget-feature=+crt-static" commit being undone here; in all configurations, this flag does not alter the error output observed here)

  = note: lld: error: undefined symbol: strdup
          >>> referenced by libcurl_sys-26100ec537c16e87.rlib(easy.o):(curl_global_init)
          >>> referenced by libcurl_sys-26100ec537c16e87.rlib(easy.o):(curl_easy_init)
          >>> referenced by libcurl_sys-26100ec537c16e87.rlib(easy.o):(Curl_cstrdup)
          >>> referenced 13 more times

          lld: error: undefined symbol: read
          >>> referenced by libcurl_sys-26100ec537c16e87.rlib(warnless.o):(curlx_read)

          lld: error: undefined symbol: write
          >>> referenced by libcurl_sys-26100ec537c16e87.rlib(warnless.o):(curlx_write)

          lld: error: undefined symbol: unlink
          >>> referenced by libcurl_sys-26100ec537c16e87.rlib(altsvc.o):(Curl_altsvc_save)
          >>> referenced by libcurl_sys-26100ec537c16e87.rlib(hsts.o):(Curl_hsts_save)
          >>> referenced by libcurl_sys-26100ec537c16e87.rlib(cookie.o):(Curl_flush_cookies)
          >>> referenced 1 more times

          lld: error: undefined symbol: close
          >>> referenced by libcurl_sys-26100ec537c16e87.rlib(file.o):(file_do)
          >>> referenced by libcurl_sys-26100ec537c16e87.rlib(file.o):(file_do)
          >>> referenced by libcurl_sys-26100ec537c16e87.rlib(file.o):(file_done)
          >>> referenced 3 more times

          lld: error: undefined symbol: fdopen
          >>> referenced by libcurl_sys-26100ec537c16e87.rlib(fopen.o):(Curl_fopen)

CMake 3.xx?

CMake 3.26.3

You are not compiling any C-code in CMake (?), only via build-scripts, or at least that C-code is not linked into your rust crate, correct?

Yes, for this test branch this is the case; of course in the main branch, the vast majority of the project code is C/C++.

Slightly beyond the scope of this issue, but although lld-link is indeed faster, it appears there may be some other issues that prevent it from being the default rust-lang/rust#71520

@jschwe
Copy link
Collaborator

jschwe commented May 27, 2023

(Note: ignore spurious "-Ctarget-feature=+crt-static" commit being undone here; in all configurations, this flag does not alter the error output observed here)

I'm guessing that this may actually be the issue. I haven't verified this yet, but from a quick look it seems the cc crate only passes -MT / -MD if the ToolFamily enum is set to Msvc, but for clang this is probably not the case, since clang is compiling for MSVC abi with GNU commandline syntax. See here .

For MSVC there are actually three kinds of toolchains, but cc doesn't seem to fully consider that yet.:

  • the msvc toolchain cl
  • clang with MSVC commandline syntax clang-cl
  • clang with GNU commandline syntax clang

@riidefi
Copy link
Author

riidefi commented May 27, 2023

I'm guessing that this may actually be the issue.

It really doesn't seem like it? I pushed a run with it added back, and on lld, which completes our "Punnett square" in which all four combinations fail:

+crt-static -crt-static
Link.exe no no
lld no no

I also locally verified that once my patch is applied, both MT and MD CRTs work. I opted for MD, since I am already bundling the MSVCRT dll for my C++ core, and it shaves off some filesize.

In IDA, I can verify that indeed it is defaulting to a dynamic CRT:
image
And after adding the flag, it indeed 1) is not importing these functions and 2) is bundling these functions in the .dll as expected:

image

The difference is also immediately noticeable: the .dll filesize grows 300KB.

@jschwe
Copy link
Collaborator

jschwe commented May 27, 2023

It really doesn't seem like it?

I think you misunderstood me. I'm saying this is an upstream issue of the cc crate, which doesn't respect the crt_static flag if the compiler is clang. I opened rust-lang/cc-rs#811 to fix this, and tested on mywindows machine that linking using clang succeeds if the cc dependency is patched to my version.

I tested this fix with your rust crate by:

  1. Setting the environment variables CC and CXX to clang.
  2. Invoking cargo build on the commandline, and verifying the failiure you experienced (without corrosion being involved)
  3. adding the following to the Cargo.toml to use my fix for the cc crate, and then recompiling.
[patch.crates-io]
cc = { git = 'https://github.com/jschwe/cc-rs.git', branch = 'fix_crt' }

Edit: Sorry it seems like I may have been a bit to quick with this reply and messed up which terminal window had the CC variable set. Currently investigating.

@jschwe jschwe changed the title cdylib broken on Clang + Windows (with workarounds) cc-rs link issuen on windows msvc targets when using Clang with GNU cmdline syntax May 27, 2023
@jschwe jschwe changed the title cc-rs link issuen on windows msvc targets when using Clang with GNU cmdline syntax cc-rs link issue on windows msvc targets when using Clang with GNU cmdline syntax May 27, 2023
@jschwe
Copy link
Collaborator

jschwe commented May 28, 2023

I compiled your crate with cargo rustc -vvv -- --print link-args and interestingly when CC is set to clang, the msvcrt library is missing in the link line when invoking link.exe.

I stripped everything before the first library on the linker line to reduce the noise.
linker line with CC=clang (the interesting / missing part is at the very end):

 "ws2_32.lib" "crypt32.lib" "legacy_stdio_definitions.lib" "windows.0.48.0.lib" "bcrypt.lib" "advapi32.lib" "windows.lib" "kernel32.lib" "advapi32.lib" "bcrypt.lib" "cfgmgr32.lib" "crypt32.lib" "cryptnet.lib" "fwpuclnt.lib" "gdi32.lib" "kernel32.lib" "msimg32.lib" "ncrypt.lib" "ntdll.lib" "opengl32.lib" "user32.lib" "winspool.lib" "ws2_32.lib" "kernel32.lib" "advapi32.lib" "bcrypt.lib" "kernel32.lib" "ntdll.lib" "userenv.lib" "ws2_32.lib" "kernel32.lib" "libcmt.lib" "/NXCOMPAT" "/LIBPATH:C:\\Users\\jschw\\.rustup\\toolchains\\nightly-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib" "/OUT:C:\\Users\\jschw\\CLionProjects\\RiiStudio\\source\\rust\\target\\debug\\deps\\riistudio_rs.dll" "/OPT:REF,NOICF" "/DLL" "/IMPLIB:C:\\Users\\jschw\\CLionProjects\\RiiStudio\\source\\rust\\target\\debug\\deps\\riistudio_rs.dll.lib" "/DEBUG" "/NATVIS:C:\\Users\\jschw\\.rustup\\toolchains\\nightly-x86_64-pc-windows-msvc\\lib\\rustlib\\etc\\intrinsic.natvis" "/NATVIS:C:\\Users\\jschw\\.rustup\\toolchains\\nightly-x86_64-pc-windows-msvc\\lib\\rustlib\\etc\\liballoc.natvis" "/NATVIS:C:\\Users\\jschw\\.rustup\\toolchains\\nightly-x86_64-pc-windows-msvc\\lib\\rustlib\\etc\\libcore.natvis" "/NATVIS:C:\\Users\\jschw\\.rustup\\toolchains\\nightly-x86_64-pc-windows-msvc\\lib\\rustlib\\etc\\libstd.natvis"
error: linking with `link.exe` failed: exit code: 1120

In comparison with CC=clang-cl:

"ws2_32.lib" "crypt32.lib" "legacy_stdio_definitions.lib" "windows.0.48.0.lib" "bcrypt.lib" "advapi32.lib" "windows.lib" "kernel32.lib" "advapi32.lib" "bcrypt.lib" "cfgmgr32.lib" "crypt32.lib" "cryptnet.lib" "fwpuclnt.lib" "gdi32.lib" "kernel32.lib" "msimg32.lib" "ncrypt.lib" "ntdll.lib" "opengl32.lib" "user32.lib" "winspool.lib" "ws2_32.lib" "kernel32.lib" "advapi32.lib" "bcrypt.lib" "kernel32.lib" "ntdll.lib" "userenv.lib" "ws2_32.lib" "kernel32.lib" "msvcrt.lib" "/NXCOMPAT" "/LIBPATH:C:\\Users\\jschw\\.rustup\\toolchains\\nightly-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib" "/OUT:C:\\Users\\jschw\\CLionProjects\\RiiStudio\\source\\rust\\target\\debug\\deps\\riistudio_rs.dll" "/OPT:REF,NOICF" "/DLL" "/IMPLIB:C:\\Users\\jschw\\CLionProjects\\RiiStudio\\source\\rust\\target\\debug\\deps\\riistudio_rs.dll.lib" "/DEBUG" "/NATVIS:C:\\Users\\jschw\\.rustup\\toolchains\\nightly-x86_64-pc-windows-msvc\\lib\\rustlib\\etc\\intrinsic.natvis" "/NATVIS:C:\\Users\\jschw\\.rustup\\toolchains\\nightly-x86_64-pc-windows-msvc\\lib\\rustlib\\etc\\liballoc.natvis" "/NATVIS:C:\\Users\\jschw\\.rustup\\toolchains\\nightly-x86_64-pc-windows-msvc\\lib\\rustlib\\etc\\libcore.natvis" "/NATVIS:C:\\Users\\jschw\\.rustup\\toolchains\\nightly-x86_64-pc-windows-msvc\\lib\\rustlib\\etc\\libstd.natvis"

Unfortunately, I haven't been able to find yet where rustc defines which native libraries should be linked in.

Complete logs:
original_clang.log
original_clang_cl.log

Edit:

Apparently, libcmt.lib is the library that is used when statically linking. And indeed when adding -crt-static to the target features msvcrt.lib does appear on the link line, but sadly linking still fails.

@riidefi
Copy link
Author

riidefi commented Jun 10, 2023

Thank you for taking such a thorough look at the issue. Until it is fixed upstream, is there a particular workaround you'd endorse? I'd love to have my project on Corrosion's main branch, instead of my fork with an ugly workaround in the interim.

Cheers

@jschwe
Copy link
Collaborator

jschwe commented Jun 14, 2023

I would have preferred to have the issue fixed upstream, but I currently don't have the time or knowledge about msvc to continue investigating.

About possible solutions in the meantime:

The user can set the environment variable <var>_<target> - for example, CC_x86_64-pc-windows-msvc=msvc, which overrides the one set by Corrosion (See the cc-rs Readme). Corrosion uses the underscore variant on purpose, to give the user the opportunity to override the variables, so I wouldn't view this as hacky. I'm not sure if I documented that yet, but that was my intention.

Would this be sufficient for you? I am a bit hesitant of adding more flags, when it may not be necessary.

@jschwe
Copy link
Collaborator

jschwe commented Aug 18, 2023

@riidefi Did you have a chance to view the cc-rs PR above? I got the feedback that building C++ with clang for MSVC is not really possible, since clang with the GNU syntax has no way of accepting the /MD or /MT flags that clang-cl or cl have.
I was considering setting clang-cl as the compiler / linker for cc-rs, if clang is specified on windows. I'm not sure if falling back to cl as the default is a good idea, since there could be ABI mismatches between clang and MSVC.

@riidefi
Copy link
Author

riidefi commented Aug 21, 2023

Hi! I think that makes a lot of sense and should work perfectly well? The one thing I'd note is that there are some slight differences between clang-cl and clang that might catch someone off guard? See the motivating example here rust-lang/cc-rs#856

(Of course, minimal in comparison to that between clang and MSVC cl)

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

No branches or pull requests

2 participants