-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,12 +44,34 @@ typedef struct napi_module { | |
#define NAPI_MODULE_VERSION 1 | ||
|
||
#if defined(_MSC_VER) | ||
#if defined(__cplusplus) | ||
// The NAPI_C_CTOR macro defines a function fn that is called during dynamic | ||
// initialization of static variables. | ||
// The order of the dynamic initialization is not defined and code in fn | ||
// function must avoid using other static variables with dynamic initialization. | ||
#define NAPI_C_CTOR(fn) \ | ||
static void __cdecl fn(void); \ | ||
namespace { \ | ||
struct fn##_ { \ | ||
static int Call##fn() { return (fn(), 0); } \ | ||
static inline const int x = Call##fn(); \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it needed to implement this by using a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. node.h has macro There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
}; \ | ||
} \ | ||
static void __cdecl fn(void) | ||
#else | ||
#pragma section(".CRT$XCU", read) | ||
// The NAPI_C_CTOR macro defines a function fn that is called during CRT | ||
// initialization. | ||
// C does not support dynamic initialization of static variables and this code | ||
// simulates C++ behavior. Exporting the function pointer prevents it from being | ||
// optimized. See for details: | ||
// https://docs.microsoft.com/en-us/cpp/c-runtime-library/crt-initialization?view=msvc-170 | ||
#define NAPI_C_CTOR(fn) \ | ||
static void __cdecl fn(void); \ | ||
__declspec(dllexport, allocate(".CRT$XCU")) void(__cdecl * fn##_)(void) = \ | ||
fn; \ | ||
static void __cdecl fn(void) | ||
#endif // defined(__cplusplus) | ||
#else | ||
#define NAPI_C_CTOR(fn) \ | ||
static void fn(void) __attribute__((constructor)); \ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"targets": [ | ||
{ | ||
"target_name": "test_init_order", | ||
"sources": [ "test_init_order.cc" ] | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
'use strict'; | ||
|
||
// This test verifies that C++ static variable dynamic initialization is called | ||
// correctly and does not interfere with the module initialization. | ||
const common = require('../../common'); | ||
const test_init_order = require(`./build/${common.buildType}/test_init_order`); | ||
const assert = require('assert'); | ||
|
||
assert.strictEqual(test_init_order.cppIntValue, 42); | ||
assert.strictEqual(test_init_order.cppStringValue, '123'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
#include <node_api.h> | ||
#include <memory> | ||
#include <string> | ||
#include "../../js-native-api/common.h" | ||
|
||
namespace { | ||
|
||
inline std::string testString = "123"; | ||
|
||
struct ValueHolder { | ||
int value{42}; | ||
}; | ||
|
||
class MyClass { | ||
public: | ||
// Ensure that the static variable is initialized by a dynamic static | ||
// initializer. | ||
static std::unique_ptr<ValueHolder> valueHolder; | ||
}; | ||
|
||
std::unique_ptr<ValueHolder> MyClass::valueHolder{new ValueHolder()}; | ||
|
||
} // namespace | ||
|
||
EXTERN_C_START | ||
napi_value Init(napi_env env, napi_value exports) { | ||
napi_value cppIntValue, cppStringValue; | ||
NODE_API_CALL( | ||
env, napi_create_int32(env, MyClass::valueHolder->value, &cppIntValue)); | ||
NODE_API_CALL( | ||
env, | ||
napi_create_string_utf8( | ||
env, testString.c_str(), NAPI_AUTO_LENGTH, &cppStringValue)); | ||
|
||
napi_property_descriptor descriptors[] = { | ||
DECLARE_NODE_API_PROPERTY_VALUE("cppIntValue", cppIntValue), | ||
DECLARE_NODE_API_PROPERTY_VALUE("cppStringValue", cppStringValue)}; | ||
|
||
NODE_API_CALL(env, | ||
napi_define_properties( | ||
env, exports, std::size(descriptors), descriptors)); | ||
|
||
return exports; | ||
} | ||
EXTERN_C_END | ||
|
||
NAPI_MODULE(NODE_GYP_MODULE_NAME, Init) |
There was a problem hiding this comment.
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.