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

src: explicitly register built-in modules #16565

Closed
wants to merge 1 commit into from

Conversation

yhwang
Copy link
Member

@yhwang yhwang commented Oct 27, 2017

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 yh.wang@ibm.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

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

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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

src/node.h Outdated
@@ -200,6 +200,7 @@ NODE_EXTERN extern bool force_fips_crypto;
# endif
#endif

NODE_EXTERN void RegisterBuiltinModules();
Copy link
Member

Choose a reason for hiding this comment

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

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

src/node.h Outdated
V(inspector) \
V(crypto) \
V(tls_wrap)

Copy link
Member

Choose a reason for hiding this comment

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

node_internals.h might be a better place?

src/node.h Outdated
#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)
Copy link
Member

Choose a reason for hiding this comment

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

ditto re: node_internals.h

src/node.h Outdated
@@ -502,14 +551,40 @@ extern "C" NODE_EXTERN void node_module_register(void* mod);
} \
}

/* for nodejs built-in modules only */
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@yhwang yhwang Oct 27, 2017

Choose a reason for hiding this comment

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

@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 yhwang changed the title lib: explicitly register built-in modules src: explicitly register built-in modules Oct 27, 2017
@yhwang yhwang force-pushed the remove-attr-constructor branch 2 times, most recently from 1616f7a to 5d48b68 Compare October 27, 2017 22:34
@yhwang
Copy link
Member Author

yhwang commented Oct 27, 2017

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

@Fishrock123 Fishrock123 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 28, 2017
src/node.h Outdated
@@ -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) \
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@gireeshpunathil gireeshpunathil left a comment

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@yhwang yhwang Oct 28, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

src/node.cc Outdated
* 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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

priv, \
flags) \
static node::node_module _module = \
{ \
Copy link
Member

Choose a reason for hiding this comment

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

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

V(uv, MODREG_WITH_DEF) \
V(inspector, MODREG_WITH_DEF) \
V(crypto, OPENSSL_MODREG_DEF) \
V(tls_wrap, OPENSSL_MODREG_DEF)
Copy link
Member

Choose a reason for hiding this comment

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

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.

NULL, \
__FILE__, \
NULL, \
(node::addon_context_register_func) (regfunc), \
Copy link
Member

Choose a reason for hiding this comment

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

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

NULL \
}; \
NODE_C_CTOR_BUILTIN(_register_ ## modname) { \
node_module_register(&_module); \
Copy link
Member

Choose a reason for hiding this comment

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

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.

src/node.cc Outdated
* 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
Copy link
Member

Choose a reason for hiding this comment

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

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 yhwang force-pushed the remove-attr-constructor branch 2 times, most recently from 5762a40 to 5c33fe4 Compare October 29, 2017 07:23
@yhwang
Copy link
Member Author

yhwang commented Oct 29, 2017

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

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM modulo style nits.

priv, \
nullptr \
}; \
void _register_ ## modname() { \
Copy link
Member

Choose a reason for hiding this comment

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

\ is not aligned with its surroundings.

NODE_MODULE_CONTEXT_AWARE_X(modname, regfunc, NULL, NM_F_INTERNAL) \
NODE_MODULE_CONTEXT_AWARE_X_BUILTIN(modname, \
regfunc, \
NULL, \
Copy link
Member

Choose a reason for hiding this comment

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

Since you're here: nullptr

Copy link
Member Author

Choose a reason for hiding this comment

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

sure and done!

V(tty_wrap) \
V(udp_wrap) \
V(uv) \
V(inspector)
Copy link
Member

Choose a reason for hiding this comment

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

nit: alphabetical order please :)

void _register_fs_event_wrap() {}
void _register_js_stream() {}
void _register_module_wrap() {}
void _register_config() {}
Copy link
Member

Choose a reason for hiding this comment

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

nit: alphabetical order

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

@yhwang yhwang force-pushed the remove-attr-constructor branch 2 times, most recently from 6c15dbd to 5d4200a Compare October 30, 2017 02:43
@yhwang yhwang force-pushed the remove-attr-constructor branch 3 times, most recently from d9b5438 to d0f06d3 Compare October 31, 2017 19:03
@yhwang
Copy link
Member Author

yhwang commented Oct 31, 2017

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.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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

#include "node.h"
#include "node_internals.h"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@yhwang
Copy link
Member Author

yhwang commented Nov 1, 2017

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

yhwang commented Dec 12, 2017

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

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
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 <yh.wang@ibm.com>
Refs: #14986 (comment)
Backport-PR-URL: #17625
PR-URL: #16565
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
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 <yh.wang@ibm.com>
Refs: #14986 (comment)
Backport-PR-URL: #17625
PR-URL: #16565
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@gibfahn gibfahn added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Dec 19, 2017
deepak1556 added a commit to electron/electron that referenced this pull request Jan 5, 2018
codebytere pushed a commit to electron/electron that referenced this pull request Jan 5, 2018
codebytere added a commit to electron/electron that referenced this pull request Jan 6, 2018
* 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
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 17, 2018
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 <yh.wang@ibm.com>
PR-URL: nodejs#16565
Backport-PR-URL: nodejs#18179
Refs: nodejs#14986 (comment)
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2018
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 <yh.wang@ibm.com>
Backport-PR-URL: #18179
PR-URL: #16565
Refs: #14986 (comment)
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
zcbenz pushed a commit to electron/electron that referenced this pull request Jan 31, 2018
* 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
alexeykuzmin pushed a commit to electron/electron that referenced this pull request Feb 21, 2018
* 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
alexeykuzmin pushed a commit to electron/electron that referenced this pull request Feb 22, 2018
* 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
zcbenz pushed a commit to electron/electron that referenced this pull request Feb 23, 2018
* 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
zcbenz pushed a commit to electron/electron that referenced this pull request Feb 23, 2018
* 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
sethlu pushed a commit to sethlu/electron that referenced this pull request May 3, 2018
* 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
@MylesBorins 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.