src: kickstart addon by calling its constructor

[gabrielschulhof]

If the addon does not self-register and a well-known Init symbol cannot
be found, look for a version-agnostic symbol which calls the library
constructor and call it if found.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[gabrielschulhof]

I'll add the documentation if this modification is accepted.

[Trott]

@nodejs/addon-api

[bnoordhuis]

Couldn't you simply emit multiple node_register_module_vXX symbols instead of devising a third add-on loading mechanism?

I assume the goal is to be automatically forward-compatible but that's not necessarily a good idea. For starters, it cements the layout of struct node_module - that could never be changed again if this lands.

That said... that ship may have sailed already because nm_version == -1 is treated specially. Kinda frustrating #11975, #14902 and #19262 were reviewed by nine or ten people and no one called it out.

[gabrielschulhof]

@bnoordhuis I don't understand why this particular PR would cement struct node_module. This merely simulates upon subsequent load what the library loader would do upon first load. The only thing this cements AFAICT is that we expect a newly loaded addon to call a function exposed by Node.js either in response to a "stimulus" by the library loader, or, with this change, in response to a "stimulus" by Node.js.

This PR makes no assumption about which Node.js function the addon calls, nor about the structure of the data it passes to that function.

For N-API purposes, I don't know that it would help that I scanned over a range of node_register_module_vXX symbols, because when you build the N-API module, you don't know ahead of time what values of XX to cover. Also, scanning over a range of symbols would only be useful if we could call any of them that we find. So, I guess we could do a bunch of special cases like

if (found-v-46){// call it like this } else if (found-v-47){// call it some other way } else if (found-v-63){// call it yet another way } 

but since all these symbols take V8 objects, we can only call one such symbol, and only in such a way as is compatible with the current V8 ABI. So, scanning over a range of symbols would only make sense if we controlled the signature of those symbols.

I think the lowest common denominator, as with the library constructor, is void (*)(void);. The only missing piece is that dlopen() doesn't run the constructor again. So, come to think of it, the ability to save multiple versions of an addon into a single shared object is actually kind of orthogonal to this PR and useful in its own right.

[gabrielschulhof]

Wait a sec - this short-circuits the search for the version-specific init special symbol. I need to fix that.

[gabrielschulhof]

Like, what if the module self-registers, but with the wrong version? It should still look for the version-specific Init.

[bnoordhuis]

I don't understand why this particular PR would cement struct node_module.

It links a struct node_module into the linked list. That means Node.js and the add-on need to agree on its layout because Node.js queries it later on.

Since there is no version number to sanity-check on when mp->nm_version == -1, any change to the struct breaks existing add-ons because they'll be using the old, incompatible layout.

[gabrielschulhof]

@bnoordhuisthis PR doesn't add the module to the linked list. It merely invokes the library constructor the second time around.

Can we not retain the old structure for the case where mp->nm_version == -1?

Whatever problems there are with the addon loading we will have whether or not this PR lands, and AFAICT this PR doesn't add to them.

[bnoordhuis]

Can we not retain the old structure for the case where mp->nm_version == -1?

No, that's pinning us down for all eternity. Early Node.js releases were unversioned but we explicitly moved away from that because it's untenable in the long run.

this PR doesn't add to them

It adds a second API that exploits what is essentially an oversight. It'll make it twice as hard to fix in the future.

[gabrielschulhof]

@bnoordhuis I've tried a different tack. Let's see where that leads us.