NODE_MODULE(...): Miscompilation, crashes and other issues

[agnat]

The other day I accidentally did this:

NODE_MODULES("hello", init)

It doesn't compile. Not noticing my mistake I changed it to this:

NODE_MODULES(init, "hello")

That compiles just fine but node crashes hard (SIGSEGV) when loading the add-on. That is because the second argument is casted relentlessly to a function pointer by the macro. I wondered why. Turns out that the cast serves as poor man's overloaded function in C. That is, the add-on's init function is allowed to take fewer arguments than the caller (nodes dlopen) provides. However, this "feature" means that the init functions type doesn't match. Hence the cast. The cast breaks type safety completely and results in the miscompilation and crash described above.

Another thing I've been wondering about is the name argument. It seems a little kafkaesque that I have to state the name of the target add-on in the add-on, just so node can check that the add-on's name is in fact the add-on's name. Specially since the build system already knows the name of the target and all that is needed is:

'target_defaults': { 'defines': ['NODE_MODULE_NAME=>(_target_name)'] } 

in node-gyp's addon.gypi. Boom. All add-on's know their name and we can drop the argument.

Here is what I'd like to write:

NODE_MODULE(v8::Handle<v8::Object> exports/*, maybe more */) {
    exports->Set(...);
}

It is more compact, very readable and just leaves less room for error... provided that type safety is restored.

Thoughts?

[indutny]

The fact that node-gyp is used with npm doesn't mean that it is the only way to build modules. I guess you could use [nan][0] or something like this, if you would like to use simpler defines. Anyway, I suggest you to open an issue in node-gyp and see how far we'll get from there. Perhaps, we'll figure out some define like NODE_GYP_MODULE to simplify your life.

Regarding casts: unfortunately they are here to stay, since this is the only way to have overloaded functions, and people really need them. The fact that C++ addon crashes if it is poorly written is not something that should bother people much. If people don't check APIs when writing stuff - they could write much more dangerous stuff than just not-loading-module.

[0] https://github.com/rvagg/nan

[agnat]

Hehe ... a ticket ending in "Thoughts?" closed after the first comment ... Got it. Guess you could call it a culture of debate.

Regarding casts: unfortunately they are here to stay, since this is the only way to have overloaded functions [...]

lolwut? ... Uh... no. Most definitely not.

The fact that C++ addon crashes if it is poorly written is not something that should bother people much.

Well, it bothers me that it does compile. It just shouldn't. Good C++ APIs bail at compile time.

The fact that node-gyp is used with npm doesn't mean that it is the only way to build modules.

True that. However, I've never seen a build system that is not capable of providing the target name as a define.

Anyway... PR is at nodejs/node-gyp#541

[indutny]

Hehe ... a ticket ending in "Thoughts?" closed after the first comment ... Got it. Guess you could call it a culture of debate.

It is looking a bit different from my point of view. As a core developer - I get tons of github issues in my inbox every day. Triaging them is the only way to get through this. As a bug manifestation - this issue may get only one reply: "won't fix in near future". I am really sorry if it looked to you like a bad debate culture, this is just a bug triaging for me, not debates.

lolwut? ... Uh... no. Most definitely not.

Ok, what is the better way to do it?

Well, it bothers me that it does compile. It just shouldn't. Good C++ APIs bail at compile time.

This would certainly be improved if you'll help me with a previous question.

True that. However, I've never seen a build system that is not capable of providing the target name as a define.

This is totally right, but it is much easier to figure it out this way.

Anyway, I'm open to suggestions, but not much to debates :) Hope you don't feel offended because of this.

[indutny]

Oh, and another note. Since the suggestions are merely a changes in code, I guess they could be proposed in a Pull Request ;)

[agnat]

It is looking a bit different from my point of view.

You're right. Sorry for being tetchy about it.

Ok, what is the better way to do it?

1. Always assign a function pointer that has the right type.
2. Generate a dispatch at compile time by introspecting the actual signature of the init function.

Let me do a quick sketch before I plunge into an actual PR.

However, I've never seen a build system that is not capable of providing the target name as a define.

This is totally right, but it is much easier to figure it out this way.

You think? If you're using a different build system you're going to emulate addon.gypi anyway. You'll need to setup linker behavior, pass BUILDING_NODE_EXTENSION, add include dirs, &c. So what's the point?

In my view, both the name matching in dlopen(...) and the token pasting in NODE_MODULE(...) are node implementation details. If we can hide them (we can), we should...

[agnat]

First stab at making NODE_MODULE(...) type safe:

https://gist.github.com/agnat/1f70bd870258918bf429

[agnat]

PR, as requested... ;)

[indutny]

@agnat this is what I call OpenSource! :)