src: explicitly register built-in modules #16565

yhwang · 2017-10-27T21:11:43Z

Previously, built-in modules are registered before main() via
__attribute__((constructor)) mechanism in GCC and similiar
mechanism in MSVC. This causes some issues when node is built as
static library. Calling module registration function for built-in
modules in node::Init() helps to avoid the issues.

Refs: #14986 (comment)
Signed-off-by: Yihong Wang [email protected]

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

addaleax

Seems fine to me. The commit message should start with src: rather than lib: for C++ file changes. :)

addaleax · 2017-10-27T21:24:10Z

src/node.h

@@ -200,6 +200,7 @@ NODE_EXTERN extern bool force_fips_crypto;
 # endif
 #endif

+NODE_EXTERN void RegisterBuiltinModules();


Can you add a short doc comment for this method, in particular that embedders need to call it on startup?

I know a lot in this file isn’t very well-documented, but at least new code should come with something. :)

addaleax · 2017-10-27T21:24:59Z

src/node.h

+    V(inspector)                                                              \
+    V(crypto)                                                                 \
+    V(tls_wrap)
+


node_internals.h might be a better place?

addaleax · 2017-10-27T21:25:10Z

src/node.h

 #else
 #define NODE_C_CTOR(fn)                                               \
  NODE_CTOR_PREFIX void fn(void) __attribute__((constructor));        \
  NODE_CTOR_PREFIX void fn(void)
+#define NODE_C_CTOR_BUILTIN(fn)                                       \
+  void fn(void);                                                      \
+  void fn(void)


ditto re: node_internals.h

addaleax · 2017-10-27T21:25:37Z

src/node.h

@@ -502,14 +551,40 @@ extern "C" NODE_EXTERN void node_module_register(void* mod);
    }                                                                 \
  }

+/* for nodejs built-in modules only */


This is a very good sign that this, too, would be nicer to have in node_internals.h :)

@addaleax thanks, actually @bnoordhuis had the same comments. I got 2-3 working servers, seems I didn't pick the right one to push the change. let me do it now. Thanks for the comments.

yhwang · 2017-10-27T22:34:27Z

I pushed an updated version. It should address the node_internals.h suggestions.

Fishrock123 · 2017-10-28T02:21:22Z

src/node.h

@@ -508,8 +508,6 @@ extern "C" NODE_EXTERN void node_module_register(void* mod);
 #define NODE_MODULE_CONTEXT_AWARE(modname, regfunc)                   \
  NODE_MODULE_CONTEXT_AWARE_X(modname, regfunc, NULL, 0)

-#define NODE_MODULE_CONTEXT_AWARE_BUILTIN(modname, regfunc)           \
-  NODE_MODULE_CONTEXT_AWARE_X(modname, regfunc, NULL, NM_F_BUILTIN)   \


Probably a good idea but this is definitely semver-major.

@Fishrock123 What here is semver-major?

I’m on the fence myself but if it is semver-major, then that is the case because embedders need to make an extra function call – not because of the other changes?

gireeshpunathil

Thanks! I was attempting to build node through xlC compiler once, and could not proceed further due to this semantics that was not possible with xlC.

Does this cover all the instances of attribute constructs, or only the ones that are associated with built-in modules?

addaleax · 2017-10-28T11:51:59Z

src/node_internals.h

+ * use the __attribute__((constructor)). Need to
+ * explicitly call the _register* functions.
+ */
+void RegisterBuiltinModules();


Sorry, I was unclear, but this is part of the embedder API so it should be in the public header – right?

Hi @addaleax and @gireeshpunathil, actually, this change is for built-in modules. It only removes the __attribute__((constructos)) for build-in modules. So, I moved this to the node_internals.h and it needs to be invoked inside the node::Start().

For third-party modules, they are built as shared library in most case and __attribute__((constructor)) works properly in dlopen(). I left that part as-is. I was not aware of that xIc compiler doesn't support it. If we want to remove the __attribute__((constructor)) for third-party modules, we also need to change the module initialization process.

@yhwang Yes, but not all embedders use node::Start(int argc, char** argv), right? So they need a definition of this function to be available.

bnoordhuis · 2017-10-28T16:19:02Z

src/node.cc

+ * function for each built-in modules explicitly in
+ * node::RegisterBuiltinModules(). This is only forward declaration.
+ * The definitions are in each module's implementation when calling
+ * the NODE_MODULE_CONTEXT_AWARE_BUILTIN.


Tiny nit: can you use // comments? C-style comments aren't exactly wrong but they look incongruous because we use C++-style comments everywhere else.

bnoordhuis · 2017-10-28T16:22:23Z

src/node_internals.h

+                                            priv,                             \
+                                            flags)                            \
+  static node::node_module _module =                                          \
+  {                                                                           \


Style nit: { should go on the previous line and since you're here, can you replace NULL with nullptr?

bnoordhuis · 2017-10-28T16:37:16Z

src/node_internals.h

+    V(uv,             MODREG_WITH_DEF)                                        \
+    V(inspector,      MODREG_WITH_DEF)                                        \
+    V(crypto,         OPENSSL_MODREG_DEF)                                     \
+    V(tls_wrap,       OPENSSL_MODREG_DEF)


This is not wrong but I'd approach it a little differently.

#define NODE_BUILTIN_STANDARD_MODULES(V) \ V(async_wrap) \ V(cares_wrap) \ // ... #if HAVE_OPENSSL #define NODE_BUILTIN_OPENSSL_MODULES(V) V(crypto) V(tls_wrap) #else #define NODE_BUILTIN_OPENSSL_MODULES(V) #endif #if NODE_HAVE_I18N_SUPPORT #define NODE_BUILTIN_ICU_MODULES(V) V(icu) #else #define NODE_BUILTIN_ICU_MODULES(V) #endif #define NODE_BUILTIN_MODULES(V) \ NODE_BUILTIN_STANDARD_MODULES(V) \ NODE_BUILTIN_OPENSSL_MODULES(V) \ NODE_BUILTIN_ICU_MODULES(V)

It's arguably a little easier to comprehend because the V macro takes only one argument this way.

bnoordhuis · 2017-10-28T16:38:20Z

src/node_internals.h

+    NULL,                                                                     \
+    __FILE__,                                                                 \
+    NULL,                                                                     \
+    (node::addon_context_register_func) (regfunc),                            \


Something for a future cleanup PR: getting rid of this cast.

bnoordhuis · 2017-10-28T16:41:02Z

src/node_internals.h

+    NULL                                                                      \
+  };                                                                          \
+  NODE_C_CTOR_BUILTIN(_register_ ## modname) {                                \
+    node_module_register(&_module);                                           \


Another something for a future cleanup PR: export the struct and call node_module_register() directly in node.cc, instead of first going through _register_foo().

Performance-wise it's unlikely to matter but it should make it a little easier to understand for future maintainers.

Also, I think you can get rid of NODE_C_CTOR_BUILTIN now, you don't need to special-case for Windows anymore.

bnoordhuis · 2017-10-28T16:48:57Z

src/node.cc

+ * The definitions are in each module's implementation when calling
+ * the NODE_MODULE_CONTEXT_AWARE_BUILTIN.
+ */
+#define DECLAREFUN(modname, impl) void _register_##modname(void) impl


Tiny nit: void _register_##modname() (no void argument list, that's a C-ism) and perhaps you can call the macro simply V? Also applies to line 4999.

yhwang · 2017-10-29T07:23:18Z

@bnoordhuis I updated the commit to address all your comments.

@addaleax you're right. So I moved the invocation of node::RegisterBuiltinModules() into node::Init(). Is that okay? If you know some scenarios that embedders would still need the RegisterBuiltinModules(), I will move it to node.h.

bnoordhuis

LGTM modulo style nits.

bnoordhuis · 2017-10-29T09:05:26Z

src/node_internals.h

+    priv,                                                                     \
+    nullptr                                                                   \
+  };                                                                          \
+  void _register_ ## modname() {                                             \


\ is not aligned with its surroundings.

bnoordhuis · 2017-10-29T09:07:21Z

src/node_internals.h

-  NODE_MODULE_CONTEXT_AWARE_X(modname, regfunc, NULL, NM_F_INTERNAL)  \
+  NODE_MODULE_CONTEXT_AWARE_X_BUILTIN(modname,                        \
+                                      regfunc,                        \
+                                      NULL,                           \


Since you're here: nullptr

sure and done!

TimothyGu · 2017-10-29T21:20:53Z

src/node_internals.h

+    V(tty_wrap)                                                               \
+    V(udp_wrap)                                                               \
+    V(uv)                                                                     \
+    V(inspector)


nit: alphabetical order please :)

TimothyGu · 2017-10-29T21:21:04Z

test/cctest/node_module_reg.cc

+void _register_fs_event_wrap() {}
+void _register_js_stream() {}
+void _register_module_wrap() {}
+void _register_config() {}


nit: alphabetical order

yhwang · 2017-10-31T19:06:23Z

Hi @Fishrock123 , @addaleax and @bnoordhuis I removed the change in src/node.h and all changes go to src/node_internals now. Can you review again? Thanks.

bnoordhuis

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/11133/

bnoordhuis · 2017-11-01T19:54:06Z

src/node_v8.cc

@@ -20,6 +20,7 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.

 #include "node.h"
+#include "node_internals.h"


Is this include and the one in uv.cc necessary?

Yes. need to add node_internals.h for node_v8.cc and uv.cc

yhwang · 2017-11-01T20:39:34Z

@bnoordhuis thanks for the review. would you remove the semver-major? label? I can do the node.h clean up in a separated PR.

yhwang · 2017-12-12T00:16:42Z

I created a PR for it and please kick off a CI for it. Thanks

Previously, built-in modules are registered before main() via __attribute__((constructor)) mechanism in GCC and similiar mechanism in MSVC. This causes some issues when node is built as static library. Calling module registration function for built-in modules in node::Init() helps to avoid the issues. Signed-off-by: Yihong Wang <[email protected]> Refs: #14986 (comment) Backport-PR-URL: #17625 PR-URL: #16565 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

nodejs/node#16565

* update submodule refs for node v9.3.0 * Define "llvm_version" for Node.js build * NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE * update NodePlatform to MultiIsolatePlatform * fix linting error * update node ref * REVIEW: Explicitly register builtin modules nodejs/node#16565 * update libcc ref * switch libcc to c62 * REVIEW: Address node api changes - Always start the inspector agent for nodejs/node#17085 - Set the tracing controller for node nodejs/node#15538 - Isolate data creation now requires plaform nodejs/node#16700

Previously, built-in modules are registered before main() via __attribute__((constructor)) mechanism in GCC and similiar mechanism in MSVC. This causes some issues when node is built as static library. Calling module registration function for built-in modules in node::Init() helps to avoid the issues. Signed-off-by: Yihong Wang <[email protected]> PR-URL: nodejs#16565 Backport-PR-URL: nodejs#18179 Refs: nodejs#14986 (comment) Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Previously, built-in modules are registered before main() via __attribute__((constructor)) mechanism in GCC and similiar mechanism in MSVC. This causes some issues when node is built as static library. Calling module registration function for built-in modules in node::Init() helps to avoid the issues. Signed-off-by: Yihong Wang <[email protected]> Backport-PR-URL: #18179 PR-URL: #16565 Refs: #14986 (comment) Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

* update submodule refs for node v9.3.0 * Define "llvm_version" for Node.js build * NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE * update NodePlatform to MultiIsolatePlatform * fix linting error * update node ref * REVIEW: Explicitly register builtin modules nodejs/node#16565 * update libcc ref * switch libcc to c62 * REVIEW: Address node api changes - Always start the inspector agent for nodejs/node#17085 - Set the tracing controller for node nodejs/node#15538 - Isolate data creation now requires plaform nodejs/node#16700

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Oct 27, 2017

addaleax reviewed Oct 27, 2017

View reviewed changes

addaleax requested a review from bnoordhuis October 27, 2017 21:27

yhwang changed the title ~~lib: explicitly register built-in modules~~ src: explicitly register built-in modules Oct 27, 2017

yhwang force-pushed the remove-attr-constructor branch 2 times, most recently from 1616f7a to 5d48b68 Compare October 27, 2017 22:34

Fishrock123 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 28, 2017

Fishrock123 reviewed Oct 28, 2017

View reviewed changes

gireeshpunathil approved these changes Oct 28, 2017

View reviewed changes

addaleax approved these changes Oct 28, 2017

View reviewed changes

bnoordhuis reviewed Oct 28, 2017

View reviewed changes

yhwang force-pushed the remove-attr-constructor branch 2 times, most recently from 5762a40 to 5c33fe4 Compare October 29, 2017 07:23

bnoordhuis approved these changes Oct 29, 2017

View reviewed changes

jasnell approved these changes Oct 29, 2017

View reviewed changes

yhwang force-pushed the remove-attr-constructor branch from 5c33fe4 to 9d5676b Compare October 29, 2017 17:06

TimothyGu reviewed Oct 29, 2017

View reviewed changes

yhwang force-pushed the remove-attr-constructor branch 2 times, most recently from 6c15dbd to 5d4200a Compare October 30, 2017 02:43

cjihrig approved these changes Oct 30, 2017

View reviewed changes

yhwang force-pushed the remove-attr-constructor branch 3 times, most recently from d9b5438 to d0f06d3 Compare October 31, 2017 19:03

yhwang force-pushed the remove-attr-constructor branch from d0f06d3 to 074e201 Compare October 31, 2017 22:04

bnoordhuis approved these changes Nov 1, 2017

View reviewed changes

MylesBorins added backported-to-v9.x and removed backport-requested-v9.x labels Dec 12, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Dec 19, 2017

gibfahn mentioned this pull request Dec 19, 2017

src: use NODE_BUILTIN_MODULE_CONTEXT_AWARE() macro #17071

Merged

deepak1556 added a commit to electron/electron that referenced this pull request Jan 5, 2018

REVIEW: Explicitly register builtin modules

699fb9a

nodejs/node#16565

codebytere pushed a commit to electron/electron that referenced this pull request Jan 5, 2018

REVIEW: Explicitly register builtin modules

13577c6

nodejs/node#16565

This was referenced Jan 16, 2018

Semver Minor Tracking nodejs/Release#298

Closed

Backport async hooks to v8.x #18179

Closed

MylesBorins added backported-to-v8.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Aug 17, 2018

deepak1556 mentioned this pull request Mar 6, 2019

refactor: load electron builtin modules with process._linkedBinding electron/electron#17247

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: explicitly register built-in modules #16565

src: explicitly register built-in modules #16565

yhwang commented Oct 27, 2017 •

edited

Loading

addaleax left a comment

addaleax Oct 27, 2017

addaleax Oct 27, 2017

addaleax Oct 27, 2017

addaleax Oct 27, 2017

yhwang Oct 27, 2017 •

edited

Loading

yhwang commented Oct 27, 2017 •

edited

Loading

Fishrock123 Oct 28, 2017

addaleax Oct 28, 2017

gireeshpunathil left a comment

addaleax Oct 28, 2017

yhwang Oct 28, 2017 •

edited

Loading

addaleax Oct 28, 2017

bnoordhuis Oct 28, 2017

bnoordhuis Oct 28, 2017

bnoordhuis Oct 28, 2017

bnoordhuis Oct 28, 2017

bnoordhuis Oct 28, 2017

bnoordhuis Oct 28, 2017

yhwang commented Oct 29, 2017

bnoordhuis left a comment

bnoordhuis Oct 29, 2017

bnoordhuis Oct 29, 2017

yhwang Oct 29, 2017

TimothyGu Oct 29, 2017

TimothyGu Oct 29, 2017

yhwang Oct 30, 2017

yhwang commented Oct 31, 2017 •

edited

Loading

bnoordhuis left a comment

bnoordhuis Nov 1, 2017

yhwang Nov 1, 2017

yhwang commented Nov 1, 2017

yhwang commented Dec 12, 2017

src: explicitly register built-in modules #16565

src: explicitly register built-in modules #16565

Conversation

yhwang commented Oct 27, 2017 • edited Loading

Checklist

Affected core subsystem(s)

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhwang Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

yhwang commented Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhwang Oct 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhwang commented Oct 29, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhwang commented Oct 31, 2017 • edited Loading

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhwang commented Nov 1, 2017

yhwang commented Dec 12, 2017

yhwang commented Oct 27, 2017 •

edited

Loading

yhwang Oct 27, 2017 •

edited

Loading

yhwang commented Oct 27, 2017 •

edited

Loading

yhwang Oct 28, 2017 •

edited

Loading

yhwang commented Oct 31, 2017 •

edited

Loading