[addons] Implement new init function argument dispatch

[agnat]

In Short:

Stop discarding the (addon) init function signature. Instead capture it at compile-time and provide a suitable adapter, thus restoring type safety.

Doing so uncovered a bug-to-happen: The old addon_register_func declared the module argument with type Handle<Value> instead of Handle<Object>. This only compiled because of the discarded type information. It only worked because it exploited numerous v8 implementation details.

It also exposed a few diverged prototypes of Initialize(...) functions. Both is fixed in this patch.

The new init function handling makes a few things easier:

• Builtin modules had an unused module argument which is no longer neccessary.
• We no longer have to deal with context aware modules seperatly. It's all the same.

The Issue:

This is not easy to spot and everything works just fine. So let me explain. The addon register function is currently declared as:

typedef void (*addon_register_func)(
    v8::Handle<v8::Object> exports,
    v8::Handle<v8::Value> module,
    void* priv);

But the documentation and thus everybody uses:

void Init(Local<Object> exports, Local<Object> module) {}

Ignore the Handle<> to Local<> thing for now and focus on the switch from Value to Object on the module argument. That looks like an attempted upcast. Now, how does this even compile? It compiles because of this cast in the NODE_MODULE_X(...) macro:

#define NODE_MODULE_X(modname, regfunc, priv, flags)                  \
  extern "C" {                                                        \
    static node::node_module _module =                                \
    {                                                                 \
      /* things */                                                    \
      (node::addon_register_func) (regfunc),    /* <=== EVIL */       \ 
      /* more things */                                               \
    };                                                                \
    NODE_C_CTOR(_register_ ## modname) {                              \
      node_module_register(&_module);                                 \
    }                                                                 \
  }

The intention here is to allow addons to only mention the first few arguments and ignore the rest. Kind of like default arguments but reverse. The result is that we miss the type mismatch and bypass any possible argument conversion that might help. (No such conversion exists, but we miss that too).

It also ignores that in fact Handle<X> and Handle<Y> are two completely unrelated types. Since the handles are passed by value, we not only depend on Handle<X> and Handle<Y> having the same API (which is reasonable), but also on things like sizeof() being equal. We treat them like actual pointers while they are objects passed on the stack. (Again, even if they where pointers such a cast Value to Object would be illegal... without RTTI blah blah).

All of this only works because we know how the handles are implemented and how polymorphism works in v8. Although non of this is likely to change I still think we are way to deep in v8 implementation details.

A minor nuisance is that NODE_MODULE(...) accepts any garbage as a function pointer.

So, we are looking for a way to deal with different init(...) signatures without producing a gaping hole in the type system.

A Solution:

Instead of discarding the type information we use it to "generate" an adapter function at compile-time. This adapter function always takes the full set of arguments and calls the user provided init function with the "requested" subset. This is implemented using template techniques to introspect the init(...) functions at compile-time. This is implemented in the first commit. The lot of it is in node.h and is best read from bottom to top.

The second commit touches all builtin modules and uses the new infrastructure.

All of this is pretty complex. I hope I caught the essence. If not, please ask.

[agnat]

@bnoordhuis commented on a outdated version:

Just a quick note, but this PR is a semver-major change [...]

Is it? That is not really intentional. The goal here is to be API compatible.

introduces a lot of style changes and style violations,

Unintentional too. It passes make cpplint. Could you elaborate?

and may not work with VS if it depends on SFINAE.

I'm pretty confident that part works. It's pretty basic. We don't require the compiler to pick up anything fancy after the mismatch. We just need it to bail (VS is pretty good at bailing). If there are issues on VS (regarding partial specialization, for example) they are probably easily mitigated. I'm not so sure about the variadic template, but that is easily unrolled ...

We don't have a windows CI build, or do we?

[bnoordhuis]

The goal here is to be API compatible.

It should be API and ABI compatible in order to be semver-minor or semver-patch. This change most definitely breaks ABI.

It passes make cpplint. Could you elaborate?

cpplint doesn't catch everything, unfortunately. I'm thinking of things like introducing blank lines, char *s or char * s instead of char* s, multiple namespaces on the same line, etc.

We don't have a windows CI build, or do we?

We do: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/57/

[agnat]

It should be API and ABI compatible in order to be semver-minor or semver-patch. This change most definitely breaks ABI.

Definitely.

cpplint doesn't catch everything, unfortunately. I'm thinking of things like introducing blank lines, char s or char * s instead of char s, multiple namespaces on the same line, etc.

Ah, k. Violation somehow sounded more severe than that...

Yeah, well... Cleaning it up makes only sense if we land it. The patch already bitrotted once and you are telling me it is likely to bitrot again. So, how do we land it?

[agnat]

... or... do we want to land it? I mean, do you agree that the situation described above is rather scary and needs addressing?

I'm aware that this kind of template based design is not common in node (for reasons) and not everybody likes it. I propose it anyway because it is a very elegant solution to a very nasty problem... and it uses a variadic template :-D

So, if you guys feel that we should do something else about it...

[agnat]

BTW, while working on this I came across something that felt suspiciously like a static initialization order fiasco. The symptom was that all of a sudden one builtin module (the crypto module) disappeared. The error was No such module: crypto. A bit of printf(...) debugging showed that in node_module_register(...) the name field mp->nm_modname was empty, but only for the crypto module. I assumed a bug in my code, started to stash and recompile a lot, and all of a sudden it was gone.

I'm guessing that the initialization order of the static node_module instance and the __attribute__(constructor) function is undefined.

Is that a known issue? I've been looking for a ticket but came up zip.

[agnat]

Something is missing:

Mac OS 10.10.4
Apple LLVM version 6.1.0 (clang-602.0.53) (based on LLVM 3.6.0svn)

[agnat]

Hm, that's also what happened on the mac bot of the jenkins build. Interesting... maybe my patch does trigger it somehow.

[rvagg]

/cc @nodejs/addon-api

[agnat]

So...

Initialization order indeed. Using construct on first use, making the module struct a static local seems to fix it.

@bnoordhuis could you give it another whirl?

[jasnell]

Any reason to keep this one open?

[bnoordhuis]

Sorry, seems I missed @agnat's ping from August. This PR will need rebasing. Perhaps one or two other contributors can chime in whether they like this approach or not.

[agnat]

Oh, right... Thanks for bringing this up...

Well, if you like the approach I'll rebase it...

[jasnell]

ping @nodejs/collaborators

[eljefedelrodeodeljefe]

I like the approach, but it's a little hard to review (concerning time). What was @bnoordhuis general opinion on this? Re: ABI, could land for v7 then in order not to stretch efforts now.

[bnoordhuis]

What was @bnoordhuis general opinion on this?

That it was a little too clever to my liking but nothing I could quite articulate. :-)

(My concerns about SFINAE not working the same everywhere were real but that is less of an issue now that we are dropping support for VS 2013.)

I think it could be simplified, though. If the goal is to get rid of the C-style cast, one option is to turn the nm_register_func and nm_context_register_func fields into a struct or tagged union, with implicit, single argument constructors to initialize it. That would enforce type safety in 1/10th lines of code.

[jasnell]

Ping... @agnat ... is this still something you wanted to pursue?

[jasnell]

Closing due to lack of forward progress on this