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

Add support for building with ClangCL on Windows #111

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Nov 22, 2023

No description provided.

@saghul
Copy link
Contributor Author

saghul commented Nov 22, 2023

clang-cl : error : unknown argument ignored in clang-cl: '-funsigned-char' [-Werror,-Wunknown-argument] [D:\a\quickjs\quickjs\build\qjs.vcxproj]
clang-cl : error : unknown argument ignored in clang-cl: '-g' [-Werror,-Wunknown-argument] [D:\a\quickjs\quickjs\build\qjs.vcxproj]

I need to add flag checking probably...

@bnoordhuis
Copy link
Contributor

CMakeLists.txt should probably exclude ClangCL on this line:

if(CMAKE_C_COMPILER_ID MATCHES "AppleClang|Clang")

@saghul
Copy link
Contributor Author

saghul commented Nov 22, 2023

I just had a few minutes to test earlier 😅

I think a better way is to check if the compiler supports a given flag so it works without explicit compiler checks, I'll give that a try...

@saghul
Copy link
Contributor Author

saghul commented Nov 22, 2023

Ok, we need the performance.now() Windows implementation then. I'll lift it off libuv :-)

@saghul saghul force-pushed the windws-clang branch 18 times, most recently from 4c83b39 to 27d4c91 Compare November 28, 2023 10:34
@saghul
Copy link
Contributor Author

saghul commented Nov 28, 2023

@bnoordhuis Can I have an extra brain here? :-) Currently this is failing with:

 lld-link : error : undefined symbol: __udivti3 [D:\a\quickjs\quickjs\build\qjsc.vcxproj]
  >>> referenced by D:\a\quickjs\quickjs\libbf.c:1319
  >>>               qjs.lib(libbf.obj):(mp_divnorm)
  >>> referenced by D:\a\quickjs\quickjs\libbf.c:5776
  >>>               qjs.lib(libbf.obj):(mp_div1_dec)
  >>> referenced by D:\a\quickjs\quickjs\libbf.c:1220
  >>>               qjs.lib(libbf.obj):(udiv1norm_init)
  >>> referenced 7 more times

Which AFAIS is because the int128 functions are in clang-rt and not in MS's crt. I guess we need to embed that? A cursory look suggests it got fixed in Clang 17.X but that doesn't seem available here... Any ideas?

@saghul saghul requested a review from bnoordhuis November 28, 2023 12:29
@bnoordhuis
Copy link
Contributor

bnoordhuis commented Nov 28, 2023

You probably only need to reimplement divdq and muldq and I think you can do that with _udiv128 and _umul128 from <immintrin.h> - edit: x86_64 only but IMO no 32 bits support is okay

@saghul
Copy link
Contributor Author

saghul commented Nov 28, 2023

I'm going to need a passport to travel so far away from my comfort zone 😅

I'll give it a try!

@bnoordhuis
Copy link
Contributor

I had a look and I don't think my suggestion is going to work - libbf.h also references int128_t and uint128_t - but what you can probably do is forcing LIMB_BITS to 32, i.e.:

diff --git a/libbf.h b/libbf.h
index dacc2a6..a11016d 100644
--- a/libbf.h
+++ b/libbf.h
@@ -27,7 +27,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
-#if INTPTR_MAX >= INT64_MAX
+#if INTPTR_MAX >= INT64_MAX && !defined(_WIN32)
 #define LIMB_LOG2_BITS 6
 #else
 #define LIMB_LOG2_BITS 5

@saghul
Copy link
Contributor Author

saghul commented Nov 28, 2023

Thanks for taking a look! 🙏

@saghul
Copy link
Contributor Author

saghul commented Nov 28, 2023

Fucking A Ben!

I think the test can be related to !defined(_MSC_VER) since compiling with MinGW GCC is ok, only ClangCL seems to be the problem.

In addition, I'll check if the direct dispatch change is necessary. It gives a warning about gnu style labels but if it works I guess we don't need to change it.

@bnoordhuis
Copy link
Contributor

I think the test can be related to !defined(_MSC_VER) since compiling with MinGW GCC is ok, only ClangCL seems to be the problem.

I'm not 100% sure but I can see that breaking when:

  1. compiling libquickjs.a with mingw, then
  2. trying to consume it with clang-cl

...because compiler-rt doesn't provide the builtins mingw emits.

@saghul saghul force-pushed the windws-clang branch 6 times, most recently from 52b65ba to 6e6a4fd Compare November 29, 2023 23:09
@saghul saghul marked this pull request as ready for review November 29, 2023 23:11
@saghul
Copy link
Contributor Author

saghul commented Nov 29, 2023

@bnoordhuis I think this one is ready! The rabbithole was deeper than I thought! 😅

@saghul saghul changed the title Add Windows Clang build to CI Add support for building with ClangCL on Windows Nov 29, 2023
Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

cutils.c Outdated Show resolved Hide resolved
quickjs-libc.c Outdated Show resolved Hide resolved
Since ClangCL is compatible with MSVC this should get us almost there.

Ref: https://clang.llvm.org/docs/MSVCCompatibility.html
@saghul saghul merged commit bfd8c38 into master Nov 30, 2023
25 checks passed
@saghul saghul deleted the windws-clang branch November 30, 2023 00:23
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.

2 participants