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
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -822,17 +822,44 @@ extern "C" NODE_EXTERN void node_module_register(void* mod);

#ifdef NODE_SHARED_MODE
# define NODE_CTOR_PREFIX
# define NODE_CTOR_ANONYMOUS_NAMESPACE_START
# define NODE_CTOR_ANONYMOUS_NAMESPACE_END
#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.

# define NODE_CTOR_ANONYMOUS_NAMESPACE_START NODE_CTOR_NAMESPACE {
# define NODE_CTOR_ANONYMOUS_NAMESPACE_END }
#endif

#if defined(_MSC_VER)
#if defined(__cplusplus)
// The NODE_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 NODE_C_CTOR(fn) \
NODE_CTOR_PREFIX void __cdecl fn(void); \
NODE_CTOR_ANONYMOUS_NAMESPACE_START \
struct fn##_ { \
static int Call##fn() { return (fn(), 0); } \
static inline const int x = Call##fn(); \
}; \
NODE_CTOR_ANONYMOUS_NAMESPACE_END \
NODE_CTOR_PREFIX void __cdecl fn(void)
#else
#pragma section(".CRT$XCU", read)
// The NODE_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 NODE_C_CTOR(fn) \
NODE_CTOR_PREFIX void __cdecl fn(void); \
__declspec(dllexport, allocate(".CRT$XCU")) \
void (__cdecl*fn ## _)(void) = fn; \
NODE_CTOR_PREFIX void __cdecl fn(void)
#endif
#else
#define NODE_C_CTOR(fn) \
NODE_CTOR_PREFIX void fn(void) __attribute__((constructor)); \
Expand Down
22 changes: 22 additions & 0 deletions src/node_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -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(); \
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.

}; \
} \
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)); \
Expand Down
3 changes: 3 additions & 0 deletions test/js-native-api/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@
#define DECLARE_NODE_API_GETTER(name, func) \
{ (name), NULL, NULL, (func), NULL, NULL, napi_default, NULL }

#define DECLARE_NODE_API_PROPERTY_VALUE(name, value) \
{ (name), NULL, NULL, NULL, NULL, (value), napi_default, NULL }

void add_returned_status(napi_env env,
const char* key,
napi_value object,
Expand Down
8 changes: 8 additions & 0 deletions test/node-api/test_init_order/binding.gyp
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" ]
}
]
}
10 changes: 10 additions & 0 deletions test/node-api/test_init_order/test.js
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');
47 changes: 47 additions & 0 deletions test/node-api/test_init_order/test_init_order.cc
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)