plugin-interface: use extern "C" for plugin init function
#150
Conversation
phip1611
requested review from
rageagainsthepc,
K-Mayer and
cakeless
as code owners
phip1611
force-pushed
the
plugin-interface
branch
from
12765a4 to
b501524
Compare
There was a problem hiding this comment.
First of, thanks for the PR. 👍
I am not really happy with this solution because it makes plugin initialization less foolproof. There is not really a good way of reliably validating all input parameters on the plugin side, nor should this be the responsibility of the plugin. On the vmicore side loading the wrong function will probably lead to undefined behavior or crashes. On the other hand, these are all more or less theoretical issues and it's probably fine to accept these downsides if it is not possible to demangle this function signature on all platforms in a consistent way. I would simply remove all checks you've added and accept the fact that everything will break if we load the wrong library who happens to have a similar export.
|
So, you didn't run into any demangling errors with the plugin api version? Should we export this as |
Fair point. However, we should look at actual realistic use-cases of smartvmi. Correct me if I'm wrong but I don't see real use-cases in the past or the near or even far future, where smartvmi is not consumed as entire tree and built from that. Although I understand the possible idea that smartvmi is distributed via a package manager and users create plugins on their own, VMI is much too niche for that to be actually used like that. Right? Or am I missing something? :)
Nope, no problems. I'd say leave it as it is for now if it works |
phip1611
force-pushed
the
plugin-interface
branch
from
b501524 to
e7bb755
Compare
The university partners we have worked together with over the past two years mentioned that they might consider letting students write their thesis using this software and for this use case it might be handy to develop a plugin separately. I don't know how likely this is to happen though ;) |
| if (plugin == nullptr) | ||
| { | ||
| throw PluginException(pluginName, "init function returned null"); | ||
| } |
There was a problem hiding this comment.
I think you forgot to remove this? The init function can never return a nullptr, just an empty unique_ptr
| @@ -17,6 +17,8 @@ | |||
|
|
|||
| namespace VmiCore | |||
| { | |||
| constexpr std::string_view PLUGIN_INIT_FUNCTION = "init_plugin"; | |||
There was a problem hiding this comment.
Just a suggestion. No need to add it if you don't want to, but maybe name it something like "vmicore_plugin_init" since we are already at it?
|
Regarding the formatting issues: Most IDEs support formatting with clang-format, so you don't have to do it manually. Alternatively, you can also use the |
phip1611
force-pushed
the
plugin-interface
branch
from
e7bb755 to
5199b69
Compare
|
Done. Are we good to go? |
There are still some formatting issues unfortunately. |
|
Weird, I thought I ran exactly the same commands locally and it worked. Now, hopefully. |
Currently, smartvmi loads plugins dynamically by loading their shared object. From that shared object, the main entry function is searched. The problem with that is that the current approach depends on the hard-coded C++-mangled symbol name. This however depends on the toolchain (compiler version etc.). Specifically, when compiled on NixOS, the name didn't match the upstream source code version. As this symbol name is only relevant when loading a shared object into memory as part of the shared object's metadata analysis, this name doesn't have to be unique across plugins. Therefore, to get rid of the hardcoded symbol name, we use "extern C" to make things easier and more portable. The function was renamed from "init" to "vmicore_plugin_init" to have a more expressive name. The disadvantage is that the caller can no longer implicitly verify the function signature used when the plugin was compiled. However, with a few more assertions, we can mitigate this mostly.
phip1611
force-pushed
the
plugin-interface
branch
from
5199b69 to
f49beef
Compare
Currently, smartvmi loads plugins dynamically by loading their shared object. From that shared object, the main entry function is searched. The problem with that is that the current approach depends on the hard-coded C++-mangled symbol name. This however depends on the toolchain (compiler version etc.). Specifically, when compiled on NixOS, the name didn't match the upstream source code version.
As this symbol name is only relevant when loading a shared object into memory as part of the shared object's metadata analysis, this name doesn't have to be unique across plugins. Therefore, to get rid of the hardcoded symbol name, we use "extern C" to make things easier and more portable.
The function was renamed from "init" to "init_plugin" to have a more expressive name.
The disadvantage is that the caller can no longer implicitly verify the function signature used when the plugin was compiled.
However, with a few more assertions, we can mitigate this mostly.