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.

Sign up for GitHub

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

Jump to bottom

Cross-compilation of zstd-sys for x86_64-pc-windows-msvc target fails starting at 1.0.77 #754

Closed
Hainish opened this issue Nov 21, 2022 · 21 comments
Closed

Cross-compilation of zstd-sys for x86_64-pc-windows-msvc target fails starting at 1.0.77 #754

Hainish opened this issue Nov 21, 2022 · 21 comments
Labels
O-windows Windows targets and toolchains

Comments

@Hainish
Copy link

Hainish commented Nov 21, 2022

I am cross-compiling zstd-sys to x86_64-pc-windows-msvc in https://github.com/EFForg/apkeep/blob/5bffbf77cba71751884a46234dab66d83d5b0dbe/build-remote.sh#L75-L101. When cc is updated from 1.0.76 to 1.0.77, the build starts failing with the following:

  running: "clang-cl" "-nologo" "-MD" "-O2" "-Brepro" "-m64" "-Wno-unused-command-line-argument" "-fuse-ld=lld-link" "/imsvc/home/admin/xwin/crt/include" "/imsvc/home/admin/xwin/sdk/include/ucrt" "/imsvc/home/ad
min/xwin/sdk/include/um" "/imsvc/home/admin/xwin/sdk/include/shared" "-I" "zstd/lib/" "-I" "zstd/lib/common" "-I" "zstd/lib/legacy" "-fvisibility=hidden" "-DZSTD_LIB_DEPRECATED=0" "-DXXH_PRIVATE_API=" "-DZSTDLIB
_VISIBILITY=" "-DZDICTLIB_VISIBILITY=" "-DZSTDERRORLIB_VISIBILITY=" "-DZSTD_LEGACY_SUPPORT=1" "-Fo/home/admin/apkeep/target/x86_64-pc-windows-msvc/release/build/zstd-sys-6caf8d3f6c2dc823/out/zstd/lib/legacy/zstd
_v06.o" "-c" "--" "zstd/lib/legacy/zstd_v06.c"
  cargo:warning=clang: warning: unknown argument ignored in clang-cl: '-fvisibility=hidden' [-Wunknown-argument]
  exit status: 0
  running: "clang-cl" "-nologo" "-MD" "-O2" "-Brepro" "-m64" "-Wno-unused-command-line-argument" "-fuse-ld=lld-link" "/imsvc/home/admin/xwin/crt/include" "/imsvc/home/admin/xwin/sdk/include/ucrt" "/imsvc/home/ad
min/xwin/sdk/include/um" "/imsvc/home/admin/xwin/sdk/include/shared" "-I" "zstd/lib/" "-I" "zstd/lib/common" "-I" "zstd/lib/legacy" "-fvisibility=hidden" "-DZSTD_LIB_DEPRECATED=0" "-DXXH_PRIVATE_API=" "-DZSTDLIB
_VISIBILITY=" "-DZDICTLIB_VISIBILITY=" "-DZSTDERRORLIB_VISIBILITY=" "-DZSTD_LEGACY_SUPPORT=1" "-Fo/home/admin/apkeep/target/x86_64-pc-windows-msvc/release/build/zstd-sys-6caf8d3f6c2dc823/out/zstd/lib/legacy/zstd
_v02.o" "-c" "--" "zstd/lib/legacy/zstd_v02.c"
  cargo:warning=clang: warning: unknown argument ignored in clang-cl: '-fvisibility=hidden' [-Wunknown-argument]
  exit status: 0
  running: "clang-cl" "-nologo" "-MD" "-O2" "-Brepro" "-m64" "-Wno-unused-command-line-argument" "-fuse-ld=lld-link" "/imsvc/home/admin/xwin/crt/include" "/imsvc/home/admin/xwin/sdk/include/ucrt" "/imsvc/home/ad
min/xwin/sdk/include/um" "/imsvc/home/admin/xwin/sdk/include/shared" "-I" "zstd/lib/" "-I" "zstd/lib/common" "-I" "zstd/lib/legacy" "-fvisibility=hidden" "-DZSTD_LIB_DEPRECATED=0" "-DXXH_PRIVATE_API=" "-DZSTDLIB
_VISIBILITY=" "-DZDICTLIB_VISIBILITY=" "-DZSTDERRORLIB_VISIBILITY=" "-DZSTD_LEGACY_SUPPORT=1" "-Fo/home/admin/apkeep/target/x86_64-pc-windows-msvc/release/build/zstd-sys-6caf8d3f6c2dc823/out/zstd/lib/legacy/zstd
_v03.o" "-c" "--" "zstd/lib/legacy/zstd_v03.c"
  cargo:warning=clang: warning: unknown argument ignored in clang-cl: '-fvisibility=hidden' [-Wunknown-argument]
  exit status: 0
  running: "clang-cl" "-nologo" "-MD" "-O2" "-Brepro" "-m64" "-Wno-unused-command-line-argument" "-fuse-ld=lld-link" "/imsvc/home/admin/xwin/crt/include" "/imsvc/home/admin/xwin/sdk/include/ucrt" "/imsvc/home/ad
min/xwin/sdk/include/um" "/imsvc/home/admin/xwin/sdk/include/shared" "-I" "zstd/lib/" "-I" "zstd/lib/common" "-I" "zstd/lib/legacy" "-fvisibility=hidden" "-DZSTD_LIB_DEPRECATED=0" "-DXXH_PRIVATE_API=" "-DZSTDLIB
_VISIBILITY=" "-DZDICTLIB_VISIBILITY=" "-DZSTDERRORLIB_VISIBILITY=" "-DZSTD_LEGACY_SUPPORT=1" "-Fo/home/admin/apkeep/target/x86_64-pc-windows-msvc/release/build/zstd-sys-6caf8d3f6c2dc823/out/zstd/lib/legacy/zstd
_v05.o" "-c" "--" "zstd/lib/legacy/zstd_v05.c"
  cargo:warning=clang: warning: unknown argument ignored in clang-cl: '-fvisibility=hidden' [-Wunknown-argument]
  exit status: 0
  running: "clang-cl" "-nologo" "-MD" "-O2" "-Brepro" "-m64" "-Wno-unused-command-line-argument" "-fuse-ld=lld-link" "/imsvc/home/admin/xwin/crt/include" "/imsvc/home/admin/xwin/sdk/include/ucrt" "/imsvc/home/ad
min/xwin/sdk/include/um" "/imsvc/home/admin/xwin/sdk/include/shared" "-I" "zstd/lib/" "-I" "zstd/lib/common" "-I" "zstd/lib/legacy" "-fvisibility=hidden" "-DZSTD_LIB_DEPRECATED=0" "-DXXH_PRIVATE_API=" "-DZSTDLIB
_VISIBILITY=" "-DZDICTLIB_VISIBILITY=" "-DZSTDERRORLIB_VISIBILITY=" "-DZSTD_LEGACY_SUPPORT=1" "-Fo/home/admin/apkeep/target/x86_64-pc-windows-msvc/release/build/zstd-sys-6caf8d3f6c2dc823/out/zstd/lib/legacy/zstd
_v04.o" "-c" "--" "zstd/lib/legacy/zstd_v04.c"
  cargo:warning=clang: warning: unknown argument ignored in clang-cl: '-fvisibility=hidden' [-Wunknown-argument]
  exit status: 0
  running: "ml64.exe" "-nologo" "-I" "zstd/lib/" "-I" "zstd/lib/common" "-I" "zstd/lib/legacy" "-DZSTD_LIB_DEPRECATED=0" "-DXXH_PRIVATE_API=" "-DZSTDLIB_VISIBILITY=" "-DZDICTLIB_VISIBILITY=" "-DZSTDERRORLIB_VISI
BILITY=" "-DZSTD_LEGACY_SUPPORT=1" "-fvisibility=hidden" "-Fo/home/admin/apkeep/target/x86_64-pc-windows-msvc/release/build/zstd-sys-6caf8d3f6c2dc823/out/zstd/lib/decompress/huf_decompress_amd64.o" "-c" "zstd/li
b/decompress/huf_decompress_amd64.S"
  exit status: 0

  --- stderr


  error occurred: Failed to find tool. Is `ml64.exe` installed?

In 1.0.76 it succeeds.
@Hainish
Copy link
Author

Hainish commented Nov 22, 2022

cc @gyscos

@thomcc
Copy link
Member

thomcc commented Nov 22, 2022

In 1.0.76 it succeeds.

Huh weird. We landed some changes around this in 1.0.76 and 1.0.75, but nothing in 1.0.77, so that's very surprising.

@thomcc thomcc added the O-windows Windows targets and toolchains label Nov 22, 2022
@dot-asm
Copy link
Contributor

dot-asm commented Nov 22, 2022

nothing in 1.0.77

1.0.77 changes meaning of is_asm in compile_object. As result, ml64 is called to compile a file it can't handle and gets clang-cl's flags it doesn't understand.

@thomcc
Copy link
Member

thomcc commented Nov 22, 2022

Very nice catch, thank you. I probably won't be able to fix this this week (although who knows, I have slightly more time than anticipated), but it does seem quite bad :(

@thomcc
Copy link
Member

thomcc commented Nov 22, 2022

Oh, looking more closely, the way this changes is_asm is that now .s and .asm (case-insensitive) are considered to be asm files, whereas before it was just .asm.

I didn't think about that at the time, but I think we should keep that behavior...

@Hainish
Copy link
Author

Hainish commented Nov 22, 2022

Is there an appropriate env var to specify that capitalized .S files should be handled by clang-cl as well?

@thomcc
Copy link
Member

thomcc commented Nov 22, 2022

No, but reasonable suggestions are welcome.

@Hainish
Copy link
Author

Hainish commented Nov 22, 2022 •
edited

Probably should be incorporated within the env vars already existing: https://github.com/EFForg/apkeep/blob/5bffbf77cba71751884a46234dab66d83d5b0dbe/build-remote.sh#L91-L93

@thomcc
Copy link
Member

thomcc commented Nov 22, 2022

Those impacting how we handle a specific file extension are pretty surprising though...

Aside from it failing in this specific project, do you have an argument for why .S files should be treated differently than other assembly files? My experience has certainly been that .s/.S are more common file extensions than .asm for assembly...

@Hainish
Copy link
Author

Hainish commented Nov 22, 2022

I'm out of my element here so I don't have good arguments to make, I just have this compilation error to deal with.

@Hainish
Copy link
Author

Hainish commented Nov 22, 2022

If my understanding of the issue is correct, it is being treated differently currently: it's calling out to ml64.exe rather than using clang-cl as specified in the env vars above. So getting past this means having .S files respect that env var and use clang-cl instead. But I may be misunderstanding the underlying problem.

@gyscos gyscos mentioned this issue Nov 22, 2022
Merged
@gyscos
Copy link

gyscos commented Nov 22, 2022

As mentioned in gyscos/zstd-rs#176, it appears we should not have included that file for compilation. Using ml64.exe is the correct choice for assembly files (and if you wanted to support asm files you'd have to install it), but in this case the assembly file does not actually support windows.
It was included by mistake because we were looking at the host OS rather than the target OS to decide whether to include this file.

@Hainish
Copy link
Author

Hainish commented Nov 22, 2022

Thank you, looks like you fixed it in the downstream dependency!

@Hainish Hainish closed this as completed Nov 22, 2022
@thomcc
Copy link
Member

thomcc commented Nov 22, 2022

Thank you, glad to see this resolved easily.

@dot-asm
Copy link
Contributor

dot-asm commented Nov 22, 2022 •
edited

Using ml64.exe is the correct choice for assembly files

Just in case for reference. clang-cl can compile assembly files, and accepts both .s/.S and .asm extensions, but the assembler syntax is incompatible with one accepted by ml64.exe. The syntax is shared with MinGW instead. In other words you can pass MinGW [assembly] files to clang-cl.

@dot-asm
Copy link
Contributor

dot-asm commented Nov 22, 2022

As for issue being closed. The problem still needs to be fixed.

@thomcc
Copy link
Member

thomcc commented Nov 23, 2022

As for issue being closed. The problem still needs to be fixed.

Not sure, I think if we want to have a new issue for what you describe (previously these files would have been passed to clang-cl and now they are not), perhaps it should be a new issue? That also should probably not be based solely off of file extension, and instead be based off something else.

@thomcc
Copy link
Member

thomcc commented Nov 23, 2022

The old behavior implicitly (and seemingly unintentionally) assumed that .S vs .asm are for different assembly syntaxes (with .S being used for gnu-as style syntax) when clang-cl is in use. I suppose we could change that if there's a compelling reason for that behavior, but it's definitely not in line with what I've seen in the wild.

This was referenced Nov 23, 2022
Merged
Open
@MarijnS95
Copy link

As mentioned in gyscos/zstd-rs#176, it appears we should not have included that file for compilation. Using ml64.exe is the correct choice for assembly files (and if you wanted to support asm files you'd have to install it), but in this case the assembly file does not actually support windows. It was included by mistake because we were looking at the host OS rather than the target OS to decide whether to include this file.

@gyscos so for completeness, because of the mismatch looking at host OS, we were previously happily cross-compiling huf_decompress_amd64.S to Windows because it was originally fed into clang-cl? If I am following along correctly, gyscos/zstd-rs#176 could be adjusted to instead see if a valid compiler would be used (since cc backed out of this change: #755) similar to how upstream checks for GCC/Clang: https://github.com/facebook/zstd/blob/4d82a4d3f227bb6ed369f1a179a6736b913a05e1/build/meson/lib/meson.build#L48-L55

@thomcc
Copy link
Member

thomcc commented Nov 23, 2022

Yeah, you should be able to use those files.

@thomcc
Copy link
Member

thomcc commented Nov 23, 2022

I'll try and cut a release with #755 (and anything else that lands before then) this weekend (although it may be a bit later due to the holiday).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
O-windows Windows targets and toolchains
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@Hainish @gyscos @thomcc @MarijnS95 @dot-asm