Skip to content
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.

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

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

Closed
wants to merge 4 commits into from

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Mar 24, 2022

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
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/node-api

@nodejs-github-bot 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
src/node.h Outdated
#else
# define NODE_CTOR_PREFIX static
# define NODE_CTOR_NAMESPACE namespace
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

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

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@vmoroz
Copy link
Member Author

vmoroz commented Mar 30, 2022

@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.

src/node_api.h Outdated
namespace { \
struct fn##_ { \
static int Call##fn() { return (fn(), 0); } \
static inline const int x = Call##fn(); \
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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,

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@vmoroz vmoroz Apr 2, 2022

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Member

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

@vmoroz
Copy link
Member Author

vmoroz commented Apr 1, 2022

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
Copy link
Member Author

vmoroz commented Apr 1, 2022

@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.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

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

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

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@Flarna
Copy link
Member

Flarna commented Apr 4, 2022

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
Copy link
Member Author

vmoroz commented Apr 4, 2022

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.

xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
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>
@vmoroz vmoroz deleted the PR/Fix_CRT_XCU branch April 28, 2022 21:57
@juanarbol
Copy link
Member

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
Copy link
Member Author

vmoroz commented May 31, 2022

@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
Copy link
Member

@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
Copy link
Member

Flarna commented Jun 1, 2022

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
Copy link
Member Author

vmoroz commented Jun 1, 2022

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
Copy link
Member

Flarna commented Jun 1, 2022

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.

vmoroz added a commit to vmoroz/node that referenced this pull request Jun 1, 2022
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>
vmoroz added a commit to vmoroz/node that referenced this pull request Jun 1, 2022
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
Copy link
Member Author

vmoroz commented Jun 1, 2022

@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
Copy link
Member Author

vmoroz commented Jun 1, 2022

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... :(

@Flarna 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 pushed a commit that referenced this pull request Jun 23, 2022
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
danielleadams pushed a commit that referenced this pull request Jul 7, 2022
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
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pragma "section(".CRT$XCU", read)" can prevent global variable dynamic initialization
9 participants