ARM64 compilation broken on VS 2022 17.11

[StefanStojanovic]

What steps will reproduce the bug?

Running vcbuild.bat arm64.

How often does it reproduce? Is there a required condition?

Always with the main branch and VS 17.11.

What is the expected behavior? Why is that the expected behavior?

I expect Node.js to compile.

What do you see instead?

Compilation fails with:

E:\work\node\deps\ngtcp2\nghttp3\lib\nghttp3_ringbuf.c(38,14): error C2169: '__popcnt': intrinsic function, cannot be defined [E:\work\node\deps\ngtcp2\nghttp3.vcxproj]
E:\work\node\deps\ngtcp2\ngtcp2\lib\ngtcp2_ringbuf.c(36,21): error C2169: '__popcnt': intrinsic function, cannot be defined [E:\work\node\deps\ngtcp2\ngtcp2.vcxproj]

Additional information

As described in nodejs/build#3739 there is a compilation issue with VS 17.10. This was fixed in 17.11 which is currently the latest one. Our CI is pinned at VS 17.9 and now we want to move to vs 17.11, but even though x64 compilation is fixed, ARM64 fails. The issue is from the 2 dependency projects.

I'm pinging @jasnell and @zcbenz since they modified this file before (mostly as a part of a dependency update process). I have a fix for this and it is a straightforward one that uses _MSC_VER to make sure the problematic code is not included in VS 17.11 and above:

From af686be41d8d6e5dbe466a6d999adcdab2a35c0f Mon Sep 17 00:00:00 2001
From: StefanStojanovic <stefan.stojanovic@janeasystems.com>
Date: Thu, 12 Sep 2024 11:30:27 +0200
Subject: [PATCH 1/1] fix

---
 deps/ngtcp2/nghttp3/lib/nghttp3_ringbuf.c | 2 +-
 deps/ngtcp2/ngtcp2/lib/ngtcp2_ringbuf.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/deps/ngtcp2/nghttp3/lib/nghttp3_ringbuf.c b/deps/ngtcp2/nghttp3/lib/nghttp3_ringbuf.c
index 61a7d06cad3..38b54608371 100644
--- a/deps/ngtcp2/nghttp3/lib/nghttp3_ringbuf.c
+++ b/deps/ngtcp2/nghttp3/lib/nghttp3_ringbuf.c
@@ -33,7 +33,7 @@
 
 #include "nghttp3_macro.h"
 
-#if defined(_MSC_VER) && !defined(__clang__) &&                                \
+#if defined(_MSC_VER) && _MSC_VER < 1941 && !defined(__clang__) &&                                \
     (defined(_M_ARM) || defined(_M_ARM64))
 unsigned int __popcnt(unsigned int x) {
   unsigned int c = 0;
diff --git a/deps/ngtcp2/ngtcp2/lib/ngtcp2_ringbuf.c b/deps/ngtcp2/ngtcp2/lib/ngtcp2_ringbuf.c
index c381c231276..ecfdeb63b34 100644
--- a/deps/ngtcp2/ngtcp2/lib/ngtcp2_ringbuf.c
+++ b/deps/ngtcp2/ngtcp2/lib/ngtcp2_ringbuf.c
@@ -31,7 +31,7 @@
 
 #include "ngtcp2_macro.h"
 
-#if defined(_MSC_VER) && !defined(__clang__) &&                                \
+#if defined(_MSC_VER) && _MSC_VER < 1941 && !defined(__clang__) &&                                \
     (defined(_M_ARM) || defined(_M_ARM64))
 static unsigned int __popcnt(unsigned int x) {
   unsigned int c = 0;
-- 
2.45.2.windows.1

Although this could be applied as a floating patch to Node.js I assume we want to have these fixes in the projects upstream.

[targos]

Is there any good reason for MSVC to start erroring here? If not, I would expect a bug report to Microsoft, not an upstream patch (we can still float something in the mean time)

[StefanStojanovic]

Based on the code being guarded with #if, I'd say MSVC was not providing __popcnt before (which it should) and now it does, so technically, this would be a bugfix on the MSFT side.

Here is the issue that caused it to be added back in 2020, and another one from 2 weeks that I just saw now and the PR referencing it does the same thing I did upstream in one of the projects.

With all of this in mind, would it be acceptable to land a floating patch in Node.js since the upstream projects are also getting that fix?

[targos]

Yes, IMO it's always acceptable to land a floating patch when the dependencies have their own release cycles.

[StefanStojanovic]

OK, I will open a PR later in the day.