MSVC 2022 build errors

[mekhontsev]

1. Bunch of errors like: Error C2011 'fd_set': 'struct' type redefinition

This problem is caused when including <windows.h> before <winsock2.h>

Solution: just remove "#include <windows.h>" from cutils.h because <winsock2.h> already does it

2. JS_MKVAL and JS_MKPTR macros in quickjs.h when we include it in any c++ file

Solution: replace

#define JS_MKVAL(tag, val) (JSValue){ (JSValueUnion){ .int32 = val }, tag }
#define JS_MKPTR(tag, p) (JSValue){ (JSValueUnion){ .ptr = p }, tag }

with

#ifdef __cplusplus
#define JS_MKVAL(tag, val) JSValue{{ .int32 = val }, tag }
#define JS_MKPTR(tag, p) JSValue{{ .ptr = p }, tag }
#else
#define JS_MKVAL(tag, val) (JSValue){ (JSValueUnion){ .int32 = val }, tag }
#define JS_MKPTR(tag, p) (JSValue){ (JSValueUnion){ .ptr = p }, tag }
#endif

or with something better)

[saghul]

Can you send a PR? Bonus points if you first update the CI so it uses MSVC 2022 as well so the failure surfaces.

[andrieshiemstra]

Fyi

When I apply suggested changes above it does fix the type redefinition errors but I end up with a c2099 initializer is not a constant error in quickjs.c at line 40762 (static JSValue js_math_random(JSContext *ctx, JSValueConst this_val,)

It looks like the people at rquickjs fixed this once but I can't seem to apply those changes to fix the error... (or I simply don't understand what the patch actually changes)
DelSkayn/rquickjs@4f0a6ea#diff-39b2cd28edfd834319a0739d989590be79f41956d560ef1ee874dc01daae882d

[saghul]

Do you know how to use MSVC 2022? This is what our Windows runners use:

Run cmake -B build -DCMAKE_BUILD_TYPE=Release -G "Visual Studio 17 2022"
-- The C compiler identification is MSVC 19.38.33135.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.38.33130/bin/HostX64/x64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Building in Release mode
-- Building with MSVC 19.38.33135.0 on Windows-10.0.20348

[bnoordhuis]

It looks like the people at rquickjs fixed this once but I can't seem to apply those changes to fix the error... (or I simply don't understand what the patch actually changes)

I suspect it works around the... creative... approach of tying libc's ceil and Math.ceil together (edit: and floor, etc.)

Math.ceil is implemented by means of the JS_CFUNC_SPECIAL_DEF macro, and that macro basically consists of a struct initialization that looks like this:

(JSCFunctionListEntry){ u.func.cproto = ceil } // note: cproto is uint8_t, not a function pointer

msvc is pretty aggressive when it comes to inlining and replacing function calls with intrinsics. I suspect that #pragma function (ceil) makes the compiler understand the above construct but that the proper fix is to change the initialization to something like:

(JSCFunctionListEntry){ u.func.cfunc.f_f = ceil } // f_f is a double (*)(double) function pointer

[Icemic]

For the winsock problem, it will be fixed just to define WIN32_LEAN_AND_MEAN to _WIN32_WINNT=0x0602, as CMakeLists.txt did. For more detail: https://stackoverflow.com/a/8294669 . The current CMakeLists.txt on VS2022 works fine (at least for me), so I guess you are trying to build it by yourself?

However, then I encountered C2099 too. It is reported at Line 41035:
https://github.com/quickjs-ng/quickjs/blob/02c06d003687ebef2458d5368c08d9215c6502d6/quickjs.c#L41035

It can be reproduced as follows:

cmake .
cmake --build . --config Release

The --config Release is the key point. It only reproduces under Release mode in MSVC or Debug mode but with -O2

[saghul]

Interesting! How is --config different from build type?

[Icemic]

Interesting! How is --config different from build type?

It just like you changing the option in VS2022 GUI, and yes, it will reproduce if you build it with release mode by opening quickjs.sln in VS2022 GUI.

So I believe this also hints at another issue that set(CMAKE_BUILD_TYPE "Release") is not works as expected to VS2022 😆

[Icemic]

I mean, even if set CMAKE_BUILD_TYPE to Release, vs2022 compiles in Debug mode. That's why you can only find build outputs in Debug folder.

[saghul]

Aha, that makes sense! I'll look into it!

[saghul]

It looks like the people at rquickjs fixed this once but I can't seem to apply those changes to fix the error... (or I simply don't understand what the patch actually changes)

I suspect it works around the... creative... approach of tying libc's ceil and Math.ceil together (edit: and floor, etc.)

Math.ceil is implemented by means of the JS_CFUNC_SPECIAL_DEF macro, and that macro basically consists of a struct initialization that looks like this:

(JSCFunctionListEntry){ u.func.cproto = ceil } // note: cproto is uint8_t, not a function pointer

msvc is pretty aggressive when it comes to inlining and replacing function calls with intrinsics. I suspect that #pragma function (ceil) makes the compiler understand the above construct but that the proper fix is to change the initialization to something like:

(JSCFunctionListEntry){ u.func.cfunc.f_f = ceil } // f_f is a double (*)(double) function pointer

Hum. I'm looking at this but cannot see that.

#define JS_CFUNC_SPECIAL_DEF(name, length, cproto, func1) { name, JS_PROP_WRITABLE | JS_PROP_CONFIGURABLE, JS_DEF_CFUNC, 0, .u = { .func = { length, JS_CFUNC_ ## cproto, { .cproto = func1 } } } }

This is called like so: JS_CFUNC_SPECIAL_DEF("ceil", 1, f_f, ceil ),

So it will be expanded to: { "ceil", JS_PROP_WRITABLE | JS_PROP_CONFIGURABLE, JS_DEF_CFUNC, 0, .u = { .func = { 1, JS_CFUNC_f_f, { .f_f = ceil } } } }

No?

The cproto field which is uint8_t is set to JS_CFUNC_f_f which is an enum and then the .cproto field would be .f_f after substitution.

Did I miss something?

[saghul]

I "fixed" the CI in #346 so now I see the errors, but they are a bit different.

[saghul]

The function pragma trick seems reasonable I think! https://learn.microsoft.com/en-us/cpp/preprocessor/function-c-cpp?view=msvc-170

[chqrlie]

The initializer is OK, but it seems problematic to intiialize static data such as a function pointer with the address of an entry point in a dynamic loaded library, especially on Windows. We should use wrappers for all math functions, which will be the compiler the opportunity to inline the calls and avoid the DLL overhead.