node-api,src: fix module registration in MSVC C++ #42459

vmoroz · 2022-03-24T18:14:11Z

This PR addresses the issue #41852 where the use of the .CRT$XCU section by NAPI_C_CTOR and NODE_C_CTOR macros can prevent dynamic initialization of C++ static variables by MSVC.

Issue details

We use the NAPI_C_CTOR and NODE_C_CTOR macros to register Node-API and legacy Node modules.
They make sure that the registration function is called when an addon shared library (.dll or .so) is loaded.
Typically, developers use C++ static variable dynamic initializers (when a variable is initialized by a function call) to call such registration functions. The standard C programming language does not have such facility and the C-based code must rely on the compiler-specific extensions to achieve the same results:

GCC and Clang define __attribute__((constructor)) to specify a function that must be called on shared library load.
MSVC does not have any such special extensions for C and developers use the CRT initialization mechanism that MSVC uses for C++. To call dynamic initializers for static variables it uses the special .CRT$XCU section.

We use both these techniques in NAPI_C_CTOR and NODE_C_CTOR macros depending on the compiler.
But when these macros are used in C++ code as we see in #41852, it can interfere with the MSVC C++ static variable initialization.

Solution

The solution for this issue is to use different code for C++ vs C in node_api.h.
In node.h we can just switch to C++ code because this header can be used only by C++ code.
This PR does the change only for MSVC, but we can adopt the same technique for other compilers in future.
We call the registration function as a part of the C++ static variable initialization.

#define NAPI_C_CTOR(fn)                                                        \
  static void __cdecl fn(void);                                                \
  namespace {                                                                  \
  struct fn##_ {                                                               \
    fn##_() { fn(); }                                                          \
  } fn##_v_;                                                                   \
  }                                                                            \
  static void __cdecl fn(void)

Note that we use an anonymous namespace to match the static keyword used for the functions.

Resolve #41852

nodejs-github-bot · 2022-03-24T18:14:15Z

Review requested:

@nodejs/gyp
@nodejs/node-api

vmoroz · 2022-03-24T18:15:03Z

src/node.h

 #else
 # define NODE_CTOR_PREFIX static
+# define NODE_CTOR_NAMESPACE namespace


This is to avoid the lint-cpp issue with using anonymous namespace { in the header files.

mhdawson

LGTM

vmoroz · 2022-03-30T15:04:37Z

@mhdawson, the build fails because the new code uses the new C++17 features and it is being compiled by VS2015.
Do we have a requirement that Node.js addons must be compiled by old versions of VS that do not support C++17?
This is from the log file https://ci.nodejs.org/job/node-test-binary-windows-native-suites/nodes=win2012r2-vs2015-COMPILED_BY-vs2019-x86/12553/console

06:22:28     "..\\addon.cc(24): error C2433: 'demo::`anonymous-namespace'::_register_addon_::x': 'inline' not permitted on data declarations
...
06:22:28     'gyp info find Python using Python version 3.10.4 found at "C:\\Python310\\python.exe"\n' +
06:22:28     'gyp info find VS using VS2015 (14.0) found at:\n' +
06:22:28     'gyp info find VS "C:\\Program Files (x86)\\Microsoft Visual Studio 14.0"\n' +

I am going to change the PR to enable the fix only for MSVC versions that support C++17 and keep the old behavior for older MSVC versions.

Flarna · 2022-03-31T20:06:42Z

src/node_api.h

+  namespace {                                                                  \
+  struct fn##_ {                                                               \
+    static int Call##fn() { return (fn(), 0); }                                \
+    static inline const int x = Call##fn();                                    \


Is it needed to implement this by using a static inline which requires C++ 17?
The variant there requires no new C++ but it still avoids the need for the pragma.

@Flarna, no, we do not need it in this context. It is only required for the changes in the node.h to support the shared mode. Since the support for VS2015 is required for addons, I am going to refactor the code in a way that only that shared mode is going to require the C++17 and all other places are going to use the method you proposed above,

Why is it needed in node.h but not here? What's the difference between using napi or node in this regard?

node.h has macro NODE_SHARED_MODE that removes the static prefix for the registration function.
I assume this is done to support using the NODE_C_CTOR macro in header files.
With functions that approach works, but initialization of static class variables which we want to use in MSVC C++ it does not work. Thus, I decided to use the new C++17 inline static that was created for such scenarios. My hope was that C++17 is the requirement that we have for Node.js code and its modules.
I am going to research it a little bit more: I believe the C++11 static field initialization in headers can be addressed by using templates. If not, then I will keep use of C++17 inline static only for NODE_SHARED_MODE and use simple static field initialization for other scenarios.

It seems that the shared mode introduced in 410296c was required for compiling Node.js as a shared module. It has nothing to do with being able to use registration in header files. Thus, no complexity is needed.

mhdawson · 2022-03-31T22:22:29Z

Do we have a requirement that Node.js addons must be compiled by old versions of VS that do not support C++17?

There is a doc that has some info about addons/windows compilers but I can't seem to find it. @joaocgreis, @richardlau do you know where it is?

richardlau · 2022-04-01T02:15:16Z

https://github.com/nodejs/build/blob/master/doc/windows-visualstudio-supported-versions.md? Although it hasn't been updated since Node.js 15.

vmoroz · 2022-04-01T13:37:50Z

https://github.com/nodejs/build/blob/master/doc/windows-visualstudio-supported-versions.md? Although it hasn't been updated since Node.js 15.

Based on the doc, we must support VS 2015 for addon developers.

vmoroz · 2022-04-01T17:41:19Z

@mhdawson, I am going to have a new iteration for this PR that uses the C++17 inline variables only for shared NODE_MODULE macro. In other cases it will be a standard C++11 static variable initialization. Please do not land this PR before that.

mhdawson

Marking as request changes so we don't land until updates as mentioned by @vmoroz

Flarna · 2022-04-04T05:38:27Z

Seems like the added test requires an ifdef C++17 as it uses inline on data declarations.

06:29:06     "..\\test_init_order.cc(8): error C2433: 'testString': 'inline' not permitted on data declarations [C:\\workspace\\node-test-binary-windows-native-suites\\node\\test\\node-api\\test_init_order\\build\\test_init_order.vcxproj]\r\n",

vmoroz · 2022-04-04T15:57:28Z

Seems like the added test requires an ifdef C++17 as it uses inline on data declarations.

06:29:06     "..\\test_init_order.cc(8): error C2433: 'testString': 'inline' not permitted on data declarations [C:\\workspace\\node-test-binary-windows-native-suites\\node\\test\\node-api\\test_init_order\\build\\test_init_order.vcxproj]\r\n",

You are right. My intention was to have a test that matches the scenario described in the issue #41852.
Well, I changed it to more traditional static variable initializers that must not require anything beyond C++14.
Ideally, if we must support C++14 for modules, then the test modules must be compiled with C++14 instead of C++17.

PR-URL: nodejs#42459 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

juanarbol · 2022-05-31T01:45:30Z

This is causing this error in the v16.x branch:

make[1]: Leaving directory '/home/juanarbol/GitHub/node/test/node-api/test_make_callback/build'                 
Building addon in /home/juanarbol/GitHub/node/test/node-api/test_policy                                         
/home/juanarbol/GitHub/node/tools/build-addons.js:58                                                            
main(process.argv[3]).catch((err) => setImmediate(() => { throw err; }));                                       
                                                          ^                                                     
                                                                                                                
Error: Command failed: /home/juanarbol/GitHub/node/out/Release/node /home/juanarbol/GitHub/node/deps/npm/node_mo
dules/node-gyp/bin/node-gyp.js rebuild --directory=/home/juanarbol/GitHub/node/test/node-api/test_init_order    
../test_init_order.cc:46:40: error: no member named 'size' in namespace 'std'                                   
                    env, exports, std::size(descriptors), descriptors));                                        
                                  ~~~~~^                                                                        
../../../js-native-api/common.h:53:27: note: expanded from macro 'NODE_API_CALL'                                
  NODE_API_CALL_BASE(env, the_call, NULL)                                                                       
                          ^~~~~~~~                                                                              
../../../js-native-api/common.h:45:10: note: expanded from macro 'NODE_API_CALL_BASE'                           
    if ((the_call) != napi_ok) {                                         \                                      
         ^~~~~~~~                                                                                               
1 error generated.                                                                                              
make[1]: *** [test_init_order.target.mk:111: Release/obj.target/test_init_order/test_init_order.o] Error 1      
                                                                                                                
    at ChildProcess.exithandler (node:child_process:398:12)                                                     
    at ChildProcess.emit (node:events:527:28)                                                                   
    at maybeClose (node:internal/child_process:1092:16)                                                         
    at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5) {                                
  code: 1,                                                                                                      
  killed: false,                                                                                                
  signal: null,                                                                                                 
  cmd: '/home/juanarbol/GitHub/node/out/Release/node /home/juanarbol/GitHub/node/deps/npm/node_modules/node-gyp/
bin/node-gyp.js rebuild --directory=/home/juanarbol/GitHub/node/test/node-api/test_init_order',                 
  stdout: "make[1]: Entering directory '/home/juanarbol/GitHub/node/test/node-api/test_init_order/build'\n" +
    '  CXX(target) Release/obj.target/test_init_order/test_init_order.o\n' +
    "make[1]: Leaving directory '/home/juanarbol/GitHub/node/test/node-api/test_init_order/build'\n",
  stderr: "../test_init_order.cc:46:40: error: no member named 'size' in namespace 'std'\n" +
    '                    env, exports, std::size(descriptors), descriptors));\n' +
    '                                  ~~~~~^\n' +
    "../../../js-native-api/common.h:53:27: note: expanded from macro 'NODE_API_CALL'\n" +
    '  NODE_API_CALL_BASE(env, the_call, NULL)\n' +
    '                          ^~~~~~~~\n' +
    "../../../js-native-api/common.h:45:10: note: expanded from macro 'NODE_API_CALL_BASE'\n" +
    '    if ((the_call) != napi_ok) {                                         \\\n' +
    '         ^~~~~~~~\n' +
    '1 error generated.\n' +
    'make[1]: *** [test_init_order.target.mk:111: Release/obj.target/test_init_order/test_init_order.o] Error 1\
n'
}
make: *** [Makefile:437: test/node-api/.buildstamp] Error 1

vmoroz · 2022-05-31T20:48:33Z

@juanarbol , @mhdawson , how can I address this issue? Should I create a new PR, or somehow augment this PR?
The fix should be simple: std::size function used in the unit test is a part of C++17 and it seems that the target environment does not support it. We must replace it with the sizeof(descriptors)/sizeof(napi_property_descriptor) or just with the numeric literal 2.

legendecas · 2022-06-01T02:32:24Z

@vmoroz You can check out the backport docs for detailed steps: https://github.com/nodejs/node/blob/master/doc/contributing/backporting-to-release-lines.md. I believe we have an arraysize util function defined here https://github.com/nodejs/node/blob/master/src/util.h#L328 for the use cases.

Flarna · 2022-06-01T06:03:52Z

A similar backport PR would be needed for v14 (after it landed on v16 and a v16 release including it is available for a while).
But I'm not sure if it is really needed to backport this to v14. if not add label dont-land-on-v14.x.

vmoroz · 2022-06-01T16:34:33Z

A similar backport PR would be needed for v14 (after it landed on v16 and a v16 release including it is available for a while). But I'm not sure if it is really needed to backport this to v14. if not add label dont-land-on-v14.x.

@Flarna , sorry, I do not have permissions to add/change any labels.
I will let you and other Node.js contributors to decide if the PR is needed to be ported to v14.x.
For the v16.x I will create a PR, but I will need contributors help to start CI and do many other steps described in contributing doc.

Flarna · 2022-06-01T16:52:09Z

I added the dont-land-on-v14.x label. Once the backport PR is ready we will take care to start CI,...
Be prepared that backport PRs may take a longer time until they get merged as release aren't done that often and only some people are allowed to merge to release branches.

PR-URL: nodejs#42459 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

PR-URL: nodejs#42459 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Backport-PR-URL: nodejs#43293

vmoroz · 2022-06-01T23:43:44Z

@vmoroz You can check out the backport docs for detailed steps: https://github.com/nodejs/node/blob/master/doc/contributing/backporting-to-release-lines.md. I believe we have an arraysize util function defined here https://github.com/nodejs/node/blob/master/src/util.h#L328 for the use cases.

Thank you, @legendecas , for the reference: I followed it the best way I could.
I could not use the arraysize because the code is in the unit test that does not have access to util.h.

vmoroz · 2022-06-01T23:47:54Z

As a side note: I was surprised that I could not compile Node v16.x in Visual Studio 2022.
It seems that the V8 code there can be compiled only with Visual Studio 2019.
It took some time to realize it... :(

PR-URL: #42459 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Backport-PR-URL: #43293

PR-URL: nodejs/node#42459 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Backport-PR-URL: nodejs/node#43293

node-api,src: fix module registration in MSVC C++

93eecc8

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Mar 24, 2022

vmoroz commented Mar 24, 2022

View reviewed changes

addaleax approved these changes Mar 29, 2022

View reviewed changes

mhdawson approved these changes Mar 29, 2022

View reviewed changes

mhdawson mentioned this pull request Mar 29, 2022

test-cluster-primary-error flaky in test-asan job #39655

Open

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 29, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 29, 2022

This comment was marked as outdated.

Sign in to view

add a guard for the new C++17 features

2b38a2a

Flarna reviewed Mar 31, 2022

View reviewed changes

mhdawson requested changes Apr 1, 2022

View reviewed changes

change code to use simple C++ initialization

69a6843

Flarna approved these changes Apr 3, 2022

View reviewed changes

Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2022

This comment was marked as outdated.

Sign in to view

Remove C++17 specific code from tests

ec61c24

This was referenced Apr 8, 2022

CI Reliability 2022-04-08 nodejs/reliability#245

Open

CI Reliability 2022-04-09 nodejs/reliability#246

Open

BethGriggs mentioned this pull request Apr 9, 2022

2022-04-19, Version 18.0.0 (Current) #42262

Merged

This was referenced Apr 10, 2022

CI Reliability 2022-04-10 nodejs/reliability#247

Open

CI Reliability 2022-04-11 nodejs/reliability#248

Open

vmoroz deleted the PR/Fix_CRT_XCU branch April 28, 2022 21:57

juanarbol added the backport-requested-v16.x label May 31, 2022

juanarbol mentioned this pull request May 31, 2022

node-api: explicitly set __cdecl for Node-API functions #42780

Closed

Flarna added the dont-land-on-v14.x label Jun 1, 2022

vmoroz mentioned this pull request Jun 1, 2022

[v16.x backport] node-api,src: fix module registration in MSVC C++ #43293

Closed

Flarna added backport-open-v16.x and removed backport-requested-v16.x needs-ci PRs that need a full CI run. labels Jun 2, 2022

danielleadams added backported-to-v16.x and removed backport-open-v16.x labels Jun 23, 2022

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node-api,src: fix module registration in MSVC C++ #42459

node-api,src: fix module registration in MSVC C++ #42459

vmoroz commented Mar 24, 2022 •

edited

Loading

nodejs-github-bot commented Mar 24, 2022

vmoroz Mar 24, 2022

mhdawson left a comment

This comment was marked as outdated.

This comment was marked as outdated.

vmoroz commented Mar 30, 2022

Flarna Mar 31, 2022

vmoroz Apr 1, 2022

Flarna Apr 1, 2022

vmoroz Apr 1, 2022

vmoroz Apr 2, 2022 •

edited

Loading

mhdawson commented Mar 31, 2022

richardlau commented Apr 1, 2022

vmoroz commented Apr 1, 2022

vmoroz commented Apr 1, 2022

mhdawson left a comment

This comment was marked as outdated.

This comment was marked as outdated.

Flarna commented Apr 4, 2022 •

edited

Loading

vmoroz commented Apr 4, 2022

juanarbol commented May 31, 2022

vmoroz commented May 31, 2022

legendecas commented Jun 1, 2022

Flarna commented Jun 1, 2022 •

edited

Loading

vmoroz commented Jun 1, 2022

Flarna commented Jun 1, 2022

vmoroz commented Jun 1, 2022

vmoroz commented Jun 1, 2022

node-api,src: fix module registration in MSVC C++ #42459

node-api,src: fix module registration in MSVC C++ #42459

Conversation

vmoroz commented Mar 24, 2022 • edited Loading

Issue details

Solution

nodejs-github-bot commented Mar 24, 2022

vmoroz Mar 24, 2022

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

vmoroz commented Mar 30, 2022

Flarna Mar 31, 2022

Choose a reason for hiding this comment

vmoroz Apr 1, 2022

Choose a reason for hiding this comment

Flarna Apr 1, 2022

Choose a reason for hiding this comment

vmoroz Apr 1, 2022

Choose a reason for hiding this comment

vmoroz Apr 2, 2022 • edited Loading

Choose a reason for hiding this comment

mhdawson commented Mar 31, 2022

richardlau commented Apr 1, 2022

vmoroz commented Apr 1, 2022

vmoroz commented Apr 1, 2022

mhdawson left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

Flarna commented Apr 4, 2022 • edited Loading

vmoroz commented Apr 4, 2022

juanarbol commented May 31, 2022

vmoroz commented May 31, 2022

legendecas commented Jun 1, 2022

Flarna commented Jun 1, 2022 • edited Loading

vmoroz commented Jun 1, 2022

Flarna commented Jun 1, 2022

vmoroz commented Jun 1, 2022

vmoroz commented Jun 1, 2022

vmoroz commented Mar 24, 2022 •

edited

Loading

vmoroz Apr 2, 2022 •

edited

Loading

Flarna commented Apr 4, 2022 •

edited

Loading

Flarna commented Jun 1, 2022 •

edited

Loading