Skip to content

Support for non-CL Clang on Windows #981

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Support for non-CL Clang on Windows #981

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c7a9a92
Fixes to format preprocessing for Clang on Windows
HJfod Feb 5, 2025
2b1f713
Add Clang & Ninja workflow on Windows
HJfod Feb 8, 2025
51f5332
Fix Windows Clang + Ninja action
HJfod Feb 8, 2025
1607d47
Add JS_PRINTF_FORMAT to getopt_compat
HJfod Feb 8, 2025
bb6dfe0
Add missing JS_PRINTF_FORMAT
HJfod Feb 8, 2025
6731454
Fix warnings in quickjs-libc.c & update printf macro in quickjs.h
HJfod Feb 8, 2025
3073f11
Merge branch 'master' of https://github.com/quickjs-ng/quickjs
HJfod Mar 15, 2025
deb2196
Fix Windows Clang build by suppressing CRT warnings
HJfod Mar 15, 2025
f08401e
Fix building with non-CL Clang on Windows
HJfod Mar 15, 2025
de85fe6
More fixes to Windows Clang workflow
HJfod Mar 15, 2025
0ccbeae
Fix outdated name of QJS_BUILD_EXAMPLES option in Windows Clang
HJfod Mar 15, 2025
9529a1b
Remove outdated interrupt test from Windows Clang action
HJfod Mar 15, 2025
e311961
Merge branch 'master' of https://github.com/quickjs-ng/quickjs
HJfod Mar 16, 2025
833f67a
Make changes according to PR code review
HJfod Mar 18, 2025
ebee0ce
Merge branch 'master' of https://github.com/quickjs-ng/quickjs
HJfod May 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,40 @@ jobs:
cl.exe /DJS_NAN_BOXING=0 /Zs cxxtest.cc
cl.exe /DJS_NAN_BOXING=1 /Zs cxxtest.cc

windows-clang:
windows-ninja-clang:
Copy link
Contributor

@saghul saghul Mar 17, 2025

Choose a reason for hiding this comment

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

Nit, can we call this windows-real-clang perhaps?

runs-on: windows-latest
strategy:
fail-fast: false
matrix:
buildType: [Debug, Release]
steps:
- uses: actions/checkout@v4
- name: install ninja
run: |
choco install ninja
ninja.exe --version
- name: build
run: |
git submodule update --init --checkout --depth 1
cmake -B build -DQJS_BUILD_EXAMPLES=ON -DCMAKE_BUILD_TYPE=${{matrix.buildType}} -DCMAKE_C_COMPILER=clang.exe -G "Ninja"
cmake --build build --config ${{matrix.buildType}}
- name: stats
run: |
build\qjs.exe -qd
- name: cxxtest
run: |
clang.exe -D JS_NAN_BOXING=0 -fsyntax-only cxxtest.cc
clang.exe -D JS_NAN_BOXING=1 -fsyntax-only cxxtest.cc
- name: test
run: |
cp build\fib.dll examples\
cp build\point.dll examples\
build\qjs.exe examples\test_fib.js
build\qjs.exe examples\test_point.js
build\run-test262.exe -c tests.conf
build\function_source.exe

windows-clang-cl:
runs-on: windows-latest
strategy:
fail-fast: false
Expand Down
9 changes: 8 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,13 @@ xcheck_add_c_compiler_flag(-Wno-stringop-truncation)
xcheck_add_c_compiler_flag(-Wno-array-bounds)
xcheck_add_c_compiler_flag(-funsigned-char)

# Clang on Windows without MSVC command line fails because the codebase uses
# functions like strcpy over strcpy_s
if(CMAKE_C_COMPILER_ID STREQUAL "Clang" AND WIN32)
Copy link
Contributor

@saghul saghul Mar 17, 2025

Choose a reason for hiding this comment

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

Don't you also want to add NOT MSVC here, so make sure it's not ClangCL ?

add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
add_compile_definitions(_CRT_NONSTDC_NO_DEPRECATE)
endif()
Copy link
Contributor

@bnoordhuis bnoordhuis Mar 17, 2025

Choose a reason for hiding this comment

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

Suggested change
if(CMAKE_C_COMPILER_ID STREQUAL "Clang" AND WIN32)
add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
add_compile_definitions(_CRT_NONSTDC_NO_DEPRECATE)
endif()
if(CMAKE_C_COMPILER_ID STREQUAL "Clang" AND WIN32)
add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
add_compile_definitions(_CRT_NONSTDC_NO_DEPRECATE)
endif()

Copy link

@coreyapowell coreyapowell May 29, 2025

Choose a reason for hiding this comment

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

this did not seem to be there or working if the branch was current yesterday. I had to manual #define and also "#pragma warning(disable : 4996)" in several files for MS Clang to compile.

Copy link

@coreyapowell coreyapowell May 30, 2025

Choose a reason for hiding this comment

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

forget my comment ... I upgraded and compile now with msbuild and the problem is fixed.


# ClangCL is command line compatible with MSVC, so 'MSVC' is set.
if(MSVC)
xcheck_add_c_compiler_flag(-Wno-unsafe-buffer-usage)
Expand Down Expand Up @@ -77,7 +84,7 @@ endif()
if(WIN32)
if(MSVC)
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /STACK:8388608")
else()
elseif(MINGW)
Copy link
Contributor

@saghul saghul Mar 17, 2025

Choose a reason for hiding this comment

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

Why do you want to skip this on Clang?

Copy link
Author

@HJfod HJfod Mar 18, 2025

Choose a reason for hiding this comment

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

Clang didn't seem to understand the parameter syntax and I couldn't find an equivalent option. Can take another look 👍

Copy link

@coreyapowell coreyapowell May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

--stack_size
Clang in MS environment doesn't have a clang-gcc program, it has a clang-cpp program. no it's not GCC. In fact It does not carry the MSVC flag, it carries the WIN32 flag but MSYS2 Clang carries WIN32 as well. I don't think there's a special flag.
so what I did was if (msvc) elseif (GNU) else () .. doesn't seem to see Clang on my end!
Correction:. Clang knows nothing about GCC or GNU. in MSYS2 Clang responds to MINGW.
There may be something intermittent about how Clang responds to flags. I ended up reinstalling my build systems more than once, trying to reproduce a bug; if Ninja is installed a certain way in MSYS2 (the way that WinLibs does it). I verified that Clang will lose the WIN32 flag.

Copy link

@coreyapowell coreyapowell May 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I was wrong. --stack_size is clang on mac. there's no stack size in MS version and I don't think there is in linux. What it does do is ignore the flag --stack_size and not --stack, and I get 140 warnings and don't see it.
oh and just fyi .. I reinstalled my build environments for clang and the flags completely change between ninja, make, and msbuild. so that section acts differently. I've seen 4 different flagsets today. but I have no 'Clang' flag in MSYS or MSVC. Clang in the MINGW environment gives no WIN32 flag. if someone uses WinLibs, I bet this flag isn't even there.

set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,--stack,8388608")
endif()
endif()
Expand Down
3 changes: 2 additions & 1 deletion cutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ extern "C" {

/* Borrowed from Folly */
#ifndef JS_PRINTF_FORMAT
#ifdef _MSC_VER
/* Clang on Windows doesn't seem to support _Printf_format_string_ */
#if defined(_MSC_VER) && !defined(__clang__)
#include <sal.h>
#define JS_PRINTF_FORMAT _Printf_format_string_
#define JS_PRINTF_FORMAT_ATTR(format_param, dots_param)
Expand Down
6 changes: 4 additions & 2 deletions getopt_compat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@
#include <stdarg.h>
#include <stdio.h>
#include <windows.h>
/* Required for JS_PRINTF_FORMAT */
Copy link
Contributor

@saghul saghul Mar 17, 2025

Choose a reason for hiding this comment

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

Can we not do this and silence the warnings if need be? I actually want to get rid of this header altogether by parsing command line arguments by hand in qjsc, since it's only used there...

#include "cutils.h"

#ifdef __cplusplus
extern "C" {
Expand Down Expand Up @@ -145,7 +147,7 @@ static const char illoptchar[] = "unknown option -- %c";
static const char illoptstring[] = "unknown option -- %s";

static void
_vwarnx(const char *fmt,va_list ap)
JS_PRINTF_FORMAT_ATTR(1, 0) _vwarnx(JS_PRINTF_FORMAT const char *fmt,va_list ap)
{
(void)fprintf(stderr,"%s: ",__progname);
if (fmt != NULL)
Expand All @@ -154,7 +156,7 @@ _vwarnx(const char *fmt,va_list ap)
}

static void
warnx(const char *fmt,...)
JS_PRINTF_FORMAT_ATTR(1, 2) warnx(JS_PRINTF_FORMAT const char *fmt,...)
{
va_list ap;
va_start(ap,fmt);
Expand Down
8 changes: 4 additions & 4 deletions quickjs-libc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,10 @@ static void js_set_thread_state(JSRuntime *rt, JSThreadState *ts)
js_std_cmd(/*SetOpaque*/1, rt, ts);
}

#ifdef __GNUC__
#if defined(__GNUC__) || defined(__clang__)
Copy link
Contributor

@bnoordhuis bnoordhuis Mar 17, 2025

Choose a reason for hiding this comment

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

Doesn't clang-on-windows define __GNUC__? Because clang-on-linux definitely does:

$ clang -E -dM - < /dev/null | grep GNUC
#define __GNUC_MINOR__ 2
#define __GNUC_PATCHLEVEL__ 1
#define __GNUC_STDC_INLINE__ 1
#define __GNUC__ 4

Copy link
Contributor

@saghul saghul Mar 17, 2025

Choose a reason for hiding this comment

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

I believe it does too. What are you trying to fix here @HJfod ?

Copy link
Author

@HJfod HJfod Mar 18, 2025

Choose a reason for hiding this comment

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

image
Removing the defined(__clang__) check makes it not work, so seems like Clang doesn't define __GNUC__

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wformat-nonliteral"
#endif // __GNUC__
#endif // __GNUC__ || __clang__
static JSValue js_printf_internal(JSContext *ctx,
int argc, JSValueConst *argv, FILE *fp)
{
Expand Down Expand Up @@ -417,9 +417,9 @@ static JSValue js_printf_internal(JSContext *ctx,
dbuf_free(&dbuf);
return JS_EXCEPTION;
}
#ifdef __GNUC__
#if defined(__GNUC__) || defined(__clang__)
#pragma GCC diagnostic pop // ignored "-Wformat-nonliteral"
#endif // __GNUC__
#endif // __GNUC__ || __clang__

uint8_t *js_load_file(JSContext *ctx, size_t *pbuf_len, const char *filename)
{
Expand Down
4 changes: 2 additions & 2 deletions quickjs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6985,7 +6985,7 @@ static int JS_PRINTF_FORMAT_ATTR(3, 4) JS_ThrowTypeErrorOrFalse(JSContext *ctx,
}
}

#ifdef __GNUC__
#if defined(__GNUC__) || defined(__clang__)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wformat-nonliteral"
#endif // __GNUC__
Expand All @@ -7002,7 +7002,7 @@ static JSValue JS_ThrowSyntaxErrorAtom(JSContext *ctx, const char *fmt, JSAtom a
JS_AtomGetStr(ctx, buf, sizeof(buf), atom);
return JS_ThrowSyntaxError(ctx, fmt, buf);
}
#ifdef __GNUC__
#if defined(__GNUC__) || defined(__clang__)
#pragma GCC diagnostic pop // ignored "-Wformat-nonliteral"
#endif // __GNUC__

Expand Down
2 changes: 1 addition & 1 deletion quickjs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ extern "C" {

/* Borrowed from Folly */
#ifndef JS_PRINTF_FORMAT
#ifdef _MSC_VER
#if defined(_MSC_VER) && !defined(__clang__)
#include <sal.h>
#define JS_PRINTF_FORMAT _Printf_format_string_
#define JS_PRINTF_FORMAT_ATTR(format_param, dots_param)
Expand Down