Fix stdc atomics detection and add vs2019 msvc job by satk0 · Pull Request #592 · quickjs-ng/quickjs

satk0 · 2024-10-10T20:36:09Z

Made a PR as mentioned in issue #586

saghul · 2024-10-10T20:45:41Z

I think you need to use the windows-2019 image: https://github.com/actions/runner-images

satk0 · 2024-10-10T21:22:52Z

Hmm, maybe escaping /experimental:c11atomics for not vs2022 in CMakelists.txt would somehow help?

saghul · 2024-10-11T06:16:04Z

I think the problem is that we now unconditionally import the atomics compat header: https://github.com/search?q=repo%3Aquickjs-ng%2Fquickjs%20%23include%20%22quickjs-c-atomics.h%22&type=code

Skipping the run-262 build should be doable, and we only use it in a single place in quickjs-libc.c so I think if we address that we should be good to go.

In your case (radare2) I guess you only care about the former problem, right?

saghul · 2024-10-11T06:34:23Z

Looking at https://learn.microsoft.com/en-us/cpp/overview/compiler-versions?view=msvc-170 and knowing C11 atomics made it to Visual Studio 2022 version 17.5,
any MSVC < 19.35 is currently not supported.

A working build:

-- The C compiler identification is MSVC 19.41.34120.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.41.34120/bin/Hostx64/x64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- No build type selected, default to Release
-- Building in Release mode
-- Building with MSVC 19.41.34120.0 on Windows-10.0.20348

A failed one:

-- The C compiler identification is MSVC 19.29.30154.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Enterprise/VC/Tools/MSVC/14.29.30133/bin/Hostx64/x64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- No build type selected, default to Release
-- Building in Release mode
-- Building with MSVC 19.29.30154.0 on Windows-10.0.17763

I think our best bet here is to use Windows specific code for those older compiler versions. We could borrow some code from here I think: https://github.com/videolan/dav1d/blob/master/include/compat/msvc/stdatomic.h

Pinging @bnoordhuis so he is aware :-)

satk0 · 2024-10-11T11:30:22Z

Thanks a ton for such a rich answer, it's amazing how much I can learn here 😀

In your case (radare2) I guess you only care about the former problem, right?

Yes, the sole problem with radare2 was that we omitted adding C standard to meson build, so __STDC_NO_ATOMICS__ was undefined.

I think our best bet here is to use Windows specific code for those older compiler versions.

Will #if __has_include("stdatomic.h") check help in this case?

saghul · 2024-10-11T13:20:38Z

Will #if __has_include("stdatomic.h") check help in this case?

Alas no, because __has_include is not supported in MSVC for C (it might, but just for C++)

satk0 · 2024-10-11T14:07:33Z

Meh, could you please give me any hint on how this can be solved 😕 ?

bnoordhuis · 2024-10-11T14:41:00Z

You can probably guard on:

#if defined(_MSC_VER) && _MSC_VER < 1935
// do whatever for old msvc
#else
#include <stdatomic.h>
#endif

edit: s/1930/1935/ for 17.5

saghul · 2024-10-11T14:50:32Z

To add, you can borrow the functions we need from https://github.com/videolan/dav1d/blob/master/include/compat/msvc/stdatomic.h and snadwhich them in the if-defs Ben pointed out.

satk0 · 2024-10-11T15:10:41Z

Great! Thank you both for your answers, will try to copy it here

satk0 · 2024-10-11T20:39:41Z

still failing :/

bnoordhuis

The polyfill is broken. It won't work for e.g. _Atomic uint8_t *.

.github/workflows/ci.yml

bnoordhuis · 2024-10-11T21:09:56Z

quickjs-c-atomics.h

+    *expected = InterlockedCompareExchange(obj, desired, orig);
+    return *expected == orig;
+}
+#define atomic_compare_exchange_strong(p_a, expected, desired) atomic_compare_exchange_strong_int((LONG *)p_a, (LONG *)expected, (LONG)desired)


This and atomic_load/atomic_store/etc. aren't safe. quickjs.c definitely calls them on values that aren't sizeof(LONG).

Will removing the casting to LONG make them safe?

No. There's https://learn.microsoft.com/en-us/previous-versions/ttk2z1ws(v=vs.85) for compare-and-swap for 16, 32 and 64 bits values, but not 8 bits, AFAIK.

Will replacing these functions with _ alternatives do?

My suggestion would be to use _Generic, to dispatch based on the type. That should (fingers crossed) work with 2019.

satk0 · 2024-10-11T22:47:17Z

How come that macos-12 is failing now all of the sudden 🤔

trufae · 2024-10-12T16:18:34Z

Looks like ready to merge

bnoordhuis · 2024-10-12T16:43:43Z

https://github.com/quickjs-ng/quickjs/pull/592/files#r1797425774 is still unresolved.

I don't know of a way to implement compare-and-swap for 8 bit types (not without falling back to assembly) so maybe the best way forward is to:

disable CONFIG_ATOMICS when building for 2019 (see define in quickjs.c)
add _Generic int32/uint32 overloads for atomic_load, atomic_store and atomic_fetch_add
don't build run-test262 (already the case in this PR)

saghul · 2024-10-12T16:46:32Z

The CO does pass but I think Ben's comments haven't been addressed yet.

saghul · 2024-10-12T17:04:30Z

https://github.com/quickjs-ng/quickjs/pull/592/files#r1797425774 is still unresolved.

I don't know of a way to implement compare-and-swap for 8 bit types (not without falling back to assembly) so maybe the best way forward is to:

disable CONFIG_ATOMICS when building for 2019 (see define in quickjs.c)

add _Generic int32/uint32 overloads for atomic_load, atomic_store and atomic_fetch_add

don't build run-test262 (already the case in this PR)

The problem is libc now depends on atomics too (can_poll) so that's a problem...

bnoordhuis · 2024-10-12T17:13:48Z

Yes, that's addressed by (2), generic overloads for int32 and uint32 for three atomic operations. quickjs-libc.c doesn't use anything else (for now, anyway.)

saghul · 2024-10-12T17:28:39Z

Great, let's strive for 2 then!

satk0 · 2024-10-12T17:37:58Z

Woow, didn't know that you can invoke a different function based on different type in C, will try that, thanks 😎

satk0 · 2024-10-16T16:28:38Z

Done

EDIT: Eh, again fail :///

saghul · 2024-10-16T18:43:04Z

The build step looks successful but it really failed with some syntax errors...

satk0 · 2024-10-16T19:25:57Z

quickjs-libc.c(3847,5): error C7702: no compatible type for 'int *' in _Generic association list, I think that's the issue but I don't know how to solve it, could u plz help?

saghul · 2024-10-22T08:26:05Z

I just landed the PR which removed the need for atomics on quickjs-libc. So, you can probably get away with not implementing support for them yourself now ;-)

satk0 · 2024-10-22T14:10:26Z

Great! Thanks a lot, I will try to update qjs on radare2 and see how it goes, will let u know

quickjs.c

satk0 · 2024-10-22T18:46:46Z

Short note: It's working now for radare2 !!

saghul · 2024-10-22T18:48:11Z

Great to hear! 👏👏

satk0 mentioned this pull request Oct 10, 2024

Update to the latest quickjs-ng, so we dont need to ship custom patches radareorg/radare2#23440

Merged

3 tasks

satk0 force-pushed the add-vs2019-ci-job branch from 2895c2a to f86f91f Compare October 11, 2024 18:54

bnoordhuis reviewed Oct 11, 2024

View reviewed changes

satk0 force-pushed the add-vs2019-ci-job branch from 5d7c4da to 178c002 Compare October 16, 2024 16:26

satk0 mentioned this pull request Oct 21, 2024

Add support for MSVC 2019 without c11 atomics #612

Closed

saghul reviewed Oct 22, 2024

View reviewed changes

quickjs.c Show resolved Hide resolved

satk0 added 4 commits October 22, 2024 19:02

Fix stdc atomics detection

9a1bf41

Add vs2019 job

3fb230d

Implement atomics for older vs versions

450f899

Fix job, delete atomics msvc attempt

afbbaf3

satk0 force-pushed the add-vs2019-ci-job branch from 178c002 to afbbaf3 Compare October 22, 2024 17:15

saghul merged commit 62f4713 into quickjs-ng:master Oct 22, 2024

Conversation

satk0 commented Oct 10, 2024

saghul commented Oct 10, 2024

satk0 commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

saghul commented Oct 11, 2024

saghul commented Oct 11, 2024

satk0 commented Oct 11, 2024

saghul commented Oct 11, 2024

satk0 commented Oct 11, 2024

bnoordhuis commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

saghul commented Oct 11, 2024

satk0 commented Oct 11, 2024

satk0 commented Oct 11, 2024

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bnoordhuis Oct 11, 2024

Choose a reason for hiding this comment

satk0 Oct 11, 2024

Choose a reason for hiding this comment

bnoordhuis Oct 12, 2024

Choose a reason for hiding this comment

satk0 Oct 12, 2024

Choose a reason for hiding this comment

bnoordhuis Oct 12, 2024

Choose a reason for hiding this comment

satk0 commented Oct 11, 2024

trufae commented Oct 12, 2024

bnoordhuis commented Oct 12, 2024

saghul commented Oct 12, 2024

saghul commented Oct 12, 2024

bnoordhuis commented Oct 12, 2024

saghul commented Oct 12, 2024

satk0 commented Oct 12, 2024

satk0 commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

saghul commented Oct 16, 2024

satk0 commented Oct 16, 2024

saghul commented Oct 22, 2024

satk0 commented Oct 22, 2024

Uh oh!

satk0 commented Oct 22, 2024

saghul commented Oct 22, 2024

Labels

4 participants

satk0 commented Oct 10, 2024 •

edited

Loading

bnoordhuis commented Oct 11, 2024 •

edited

Loading

satk0 commented Oct 16, 2024 •

edited

Loading