mozjs icon indicating copy to clipboard operation
mozjs copied to clipboard

Windows Builds Fail when `clang-cl` Flags are added to `CXXFLAGS`

Open Redfire75369 opened this issue 1 year ago • 8 comments

The root issue here is that bindgen uses clang (not clang-cl) on windows, and on unrecognised flags, it fails. Upon adding CXXFLAGS="/clang:-flto=thin /clang:-fuse-ld=lld-link", the below code automatically adds it to bindgen's flags as well. As /clang: flags are only recognised by clang-cl, there is no way for cc and the makefile.cargo to receive the CXXFLAGS but not bindgen. https://github.com/servo/mozjs/blob/c7fb1b8d029ba80443da92fbdadb650fcdb493a7/mozjs-sys/build.rs#L334-L338

There are a few possible approaches:

  1. If a flag starts with /clang: on windows, do not add it to bindgen.
  2. Add a separate BINDGEN_CXXFLAGS variable for bindgen's flags.

As said earlier, the root issue is the usage of clang, which means this error may not be just from /clang: flags but almost any cl flag. Hence, I would suggest the latter approach.

For a concrete example of the failure, see https://github.com/Redfire75369/spiderfire/actions/runs/7112793006/job/19363443030#step:6:18053, where a libclang error is caused.

In the event it matters for updating whatever depends on it, #136 was the original PR where this behaviour was introduced.

Redfire75369 avatar Dec 06 '23 10:12 Redfire75369

Does setting the CLANG_PATH environment variable to clang-cl fix this issue?

jschwe avatar May 29 '24 12:05 jschwe

I've just tested that, and that doesn't do anything. Neither bindgen, nor libclang have any handling for such an environment variable as far I have seen.

Redfire75369 avatar Jul 28 '24 17:07 Redfire75369

bindgen uses libclang-sys, which documents CLANG_PATH here: https://github.com/KyleMayes/clang-sys?tab=readme-ov-file#environment-variables

The usage area is here in bindgen, which calls this function in clang-sys.

But I guess this maybe only affects the clang executable which is used to detect the include paths, and perhaps does not affect the parsing of arguments, which might be done directly via libclang.

jschwe avatar Jul 29 '24 06:07 jschwe

Looking at the libclang source, it directly adds clang to the args. https://github.com/llvm/llvm-project/blob/release/18.x/clang/tools/libclang/CIndex.cpp#L4097

There's also no codepath to switch it to clang-cl reasonably.

Stack is along the lines of: bindgen::ir::context::BindgenContext::new -> bindgen::clang::TranslationUnit::parse -> clang_sys::clang_parseTranslationUnit (C++) -> clang_sys::clang_parseTranslationUnit2 (C++, adds the clang) -> clang_sys::clang_parseTranslationUnit2FullArgv (C++) -> clang_sys::clang_parseTranslationUnit_Impl (C++)

It would be possible to use clang_parseTranslationUnit2FullArgv directly, but I can't really think of an API that makes sense for that.

Redfire75369 avatar Jul 29 '24 08:07 Redfire75369

https://llvm.org/devmtg/2014-04/PDFs/Talks/clang-cl.pdf mentions clang-cl.exe == clang.exe --driver-mode=cl

Does adding --driver-mode=cl as a clang argument to the builder help?

mukilan avatar Jul 29 '24 09:07 mukilan

That seems to half-work at least. It at least needs /clang:-fuse-ld=lld, /link /SUBSYSTEM:WINDOWS, /TP (instead of -x c++), /FI (instead of -include). Haven't tested it in a full build, currently at this:

lld-link: error: undefined symbol: WinMain
>>> referenced by D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:102
>>>               libcmt.lib(exe_winmain.obj):(int __cdecl invoke_main(void))
>>> referenced by D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
>>>               libcmt.lib(exe_winmain.obj):(int __cdecl __scrt_common_main_seh(void))
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Command:

clang "./src/jsapi.hpp" "-I" "<omitted>/dist/include" "-I" "<omitted>/js/src" "/TP" -fms-compatibility --driver-mode=cl /clang:-flto=thin /clang:-fuse-ld=lld "-DRUST_BINDGEN" "-DSTATIC_JS_API" "-std:c++17" "-DWIN32" "-D_CRT_USE_BUILTIN_OFFSETOF" "/FI" "<omitted>/js/src/js-confdefs.h" /link /SUBSYSTEM:WINDOWS 

Redfire75369 avatar Jul 29 '24 09:07 Redfire75369

The /link causes problems though as it's a catch-all, so no inputs are detected, causing stacker for example to fail to build.

Command "clang-cl" "-nologo" "-MD" "-O2" "-Brepro" "-m64" "--driver-mode=cl" "/clang:-flto=thin" "/clang:-fuse-ld=lld" "/link" "/SUBSYSTEM:WINDOWS" "-I" "src/arch" "-DWINDOWS" "-FoC:\\Users\\Redfire\\spiderfire\\target\\x86_64-pc-windows-msvc\\release\\build\\stacker-a0dccd7a5f938667\\out\\src/arch/windows.o" "-c" "--" "src/arch/windows.c" with args "clang-cl" did not execute successfully

Redfire75369 avatar Jul 29 '24 10:07 Redfire75369

Given that BINDGEN_EXTRA_CLANG_ARGS exists, it might be best to just stop adding CFLAGS and CXXFLAGS to bindgen.

Redfire75369 avatar Aug 05 '24 04:08 Redfire75369