Native Plugins #527

adamharrison · 2021-09-14T03:43:11Z

Added in a quick interface to allow C interop.

The idea behind this is described in #525 ; this is just a sample implementation, with a sample plugin (forthcoming).

adamharrison · 2021-09-14T04:16:17Z

To test it out, just build lite-xl; and build the plugin using build_plugin.sh, and stick it into data/plugins/sample.so.

Just a proof of concept, but I think it works. We can provide a header file for anyone that wants to use a native plugin, which would initialise local function pointers to the entire lua library (and all the common macros).

This way we can have our (statically linked) cake, and eat it too! And it's only about 100 lines of C!

Jan200101 · 2021-09-14T10:24:26Z

src/api/system.c

+  size_t sname, modlen, pathlen;
+  char buffer[512] = "lua_open_";
+  const char* modname = luaL_checklstring(L, -2, &modlen);
+  const char* path = luaL_checklstring(L, -1, &pathlen);


If what we point to is not a string, what is modlen and pathlen set to?

Jan200101 · 2021-09-14T10:25:54Z

src/api/system.c

+  const char* modname = luaL_checklstring(L, -2, &modlen);
+  const char* path = luaL_checklstring(L, -1, &pathlen);
+  void* library = SDL_LoadObject(path);
+  if (modlen == 0 || !library)


in the case modname could not be found and modlen is not changed it will retain whatever value is in memory and pass this check despite not being a string.

If it is not a string Lua will raise and error and the C code with longjmp to the Lua code managing the error and the C flow will not continue. I guess your comment is because you are not familiar with Lua C API and its error handling.

Jan200101 · 2021-09-14T10:29:26Z

src/api/system.c

+  void* library = SDL_LoadObject(path);
+  if (modlen == 0 || !library)
+    return luaL_error(L, "Unable to load %s: %s", modname, SDL_GetError());
+  for (sname = modlen - 1; sname > 0 && modname[sname] != '.'; --sname);


its unclear that the for loop does not have a body
perhaps add an empty body or switch to a while loop?

So, I did think this was a normal idiom in C for counting things, but I have no issues changing the ; to {}. Does anyone else have any comments on this one?

Oh it is but it comes at the cost of readability

To be honest, I find it just as readable; again, I think it's a common idiom used frequently in C (at least C that I've read). But if someone else wants to the chime in here, and also concurs, I have no issue changing it.

Jan200101 · 2021-09-14T10:36:58Z

src/api/system.c

+#if _WIN32
+  #define PATH_SEPARATOR '\\'
+  #define DYNAMIC_SUFFIX "dll"
+#else 
+  #if __APPLE__
+    #define DYNAMIC_SUFFIX "lib"
+  #else
+    #define DYNAMIC_SUFFIX "so"
+  #endif
+  #define PATH_SEPARATOR '/'
+#endif


The preprocessors _WIN32 and __APPLE__ should usually be defined but in the case they are value that has a false truthiness we might fall through to the Unix Logic.

Suggested change

#if _WIN32

#define PATH_SEPARATOR '\\'

#define DYNAMIC_SUFFIX "dll"

#else

#if __APPLE__

#define DYNAMIC_SUFFIX "lib"

#else

#define DYNAMIC_SUFFIX "so"

#endif

#define PATH_SEPARATOR '/'

#endif

#if defined(_WIN32)

#define PATH_SEPARATOR '\\'

#define DYNAMIC_SUFFIX "dll"

#elif defined(__APPLE__)

#define PATH_SEPARATOR '/'

#define DYNAMIC_SUFFIX "lib"

#elif defined(__linux__)

#define PATH_SEPARATOR '/'

#define DYNAMIC_SUFFIX "so"

#endif

#endif

Right, we may set an preprocessor #error directive in the unknown OS branch. Otherwise we may default to unix or check for other unix-like Os like BSD systems.

I'm happy to make both these changes, but this does not follow the conventions elsewhere in lite (not bothering with defined, not bothering to explicitly check linux). If we change it here, we should change it everywhere else that's applicable.

Personally, I don't think it's necessary to have defined; though I don't mind adding it (is there ever a realistic case where these would have different outcomes)?

Regarding defaulting to linux, I think that's a reasonable step, as there are far more *nix flavours than just linux, and they're all relatively similar in terms of this kind of thing.

Jan200101 · 2021-09-14T10:42:15Z

src/api/system.c

+  const char* cpath = luaL_checklstring(L, -1, &cpath_len);
+  char lib[8192];
+  for (size_t i = 0; i <= cpath_len; ++i) {
+    if (i == cpath_len || cpath[i] == ';') {
+      if (cpath_q) {
+        size_t offset = cpath_q - cpath_idx;
+        strncpy(lib, &cpath[cpath_idx], offset);


can we be sure that cpath fits in lib?

You're right, an additional check should be made here.

adamharrison · 2021-09-16T16:40:54Z

OK! Big changes.

Removed the searcher_plugin function entirely from C, as it wasn't necessary. It's now in start.lua in much more compacted and easy to read form; the only thing we call in C is load_native_plugin.
I generated a header called lite_plugin_api.h using the generate_header script on the lua repository. It's now accessed in the sample plugin via a single function, so things now look like this:

#include "lite_plugin_api.h"

int lua_open_sample(lua_State* L, void* (*symbol(const char*))) {
  lite_init_plugin(symbol);
  lua_createtable(L, 0, 0);
  lua_pushstring(L, "value");
  lua_setfield(L, -2, "example");
  return 1;
}

The only thing that's missing is me making a table for the lua uility and aux libraries; but other than that, it's pretty much good to go, I think (obviously, I'll move around the scripts, sample plugin and stuff). Thoughts?

adamharrison · 2021-09-16T20:23:25Z

OK! I think that's everything. I've now committed what I feel can be the final-ish version of the plugin API header, finalised the entrypoint. Plugin would now look like this:

#include "lite_xl_plugin_api.h"

int lua_open_sample(lua_State* L, void* XL) {
  lite_xl_plugin_init(XL);
  lua_createtable(L, 0, 0);
  lua_pushstring(L, "value");
  lua_setfield(L, -2, "example");
  return 1;
}

All in all, only 66 extra SLOC of C; not including the plugin header which isn't actually part of the application.

adamharrison · 2021-09-16T20:46:37Z

One last thing that may be worthy of doing before finalizing this:

Several people (I think @liquidev , and a few others), in discord bring up the issue of a non-standard entrypoint perhaps should be a different name than the standard lua entrypoint. As there's a different function signature, if you go in with an standard-named entrypoint with only a single parameter, and we call it from lite with a second parameter, I'm pretty sure that's UB, and things will likely get weird from there.

So I'm actually going to split the entrypoint in two, so that we can use legacy entrypoints if they're named as such (as use the normal behaviour), and the extended entrypoint if it's called lua_open_lite_xl_xxxxx, where xxxxx is the plugin name.

adamharrison · 2021-09-17T20:25:50Z

Hey ya'll, I've got an itch to make some native plugins once this gets in, so if no one has any objections, I'd like to set the date for acceptance of this as Sept 24; so if no one has any other concerns by then, I'll merge.

Mehgugs · 2021-09-18T01:03:20Z

data/core/start.lua

@@ -19,3 +19,18 @@ package.path = DATADIR .. '/?.lua;' .. package.path
 package.path = DATADIR .. '/?/init.lua;' .. package.path
 package.path = USERDIR .. '/?.lua;' .. package.path
 package.path = USERDIR .. '/?/init.lua;' .. package.path
+
+local dynamic_suffix = MACOS and 'lib' or (WINDOWS and 'dll' or 'so')
+package.cpath = DATADIR .. '/?.' .. dynamic_suffix .. ';' .. package.cpath


the initial .. package.cpath should be dopped here, I think using the system default should be avoided. This is also true for package.path but that is not as big of a problem.

Re: This and the next comment. So, in theory, if you were to require a native plugin that didn't interface with lua, really; it would still be valid. Like if requiring a plugin just ran a program. Or, if you require a plugin with the exact same system lib as your statically linked library, I think it would probably work; which may make easier for people using luarocks to have system modules.

I dunno if that's a compelling reason to not do this though. I do see the reason why we should disallow that. Does anyone have any other thoughts? I think that @Mehgugs is probably correct, though. It probably should be disallowed, if no one has any objections.

It's still possible to load a shared object by path from lua using package.loadlib if people are so determined, this would just prevent the automatic behaviour. The all in one loader (the fourth searcher) is just something native plugins wont need to work, and as above its functionality can still be reproduced with loadlib if people really do need that external module loaded.

Mehgugs · 2021-09-18T01:04:39Z

data/core/start.lua

+local dynamic_suffix = MACOS and 'lib' or (WINDOWS and 'dll' or 'so')
+package.cpath = DATADIR .. '/?.' .. dynamic_suffix .. ';' .. package.cpath
+package.cpath = USERDIR .. '/?.' .. dynamic_suffix .. ';' .. package.cpath
+package.searchers[3] = function(modname)


package.searchers[4] should be removed too unless it's explicitly needed to ensure all native module requires go through native plugin machinery.

Mehgugs · 2021-09-18T01:09:48Z

data/core/start.lua

+package.cpath = USERDIR .. '/?.' .. dynamic_suffix .. ';' .. package.cpath
+package.searchers[3] = function(modname)
+  local s,e = 0
+  repeat


Could package.searchpath be used here instead?

It absolutely could be. Good call; I'll refactor.

Mehgugs · 2021-09-18T01:18:35Z

src/api/system.c

+  if (name == 0 || !library)
+    return luaL_error(L, "Unable to load %s: %s", name, SDL_GetError());
+  lua_pushlightuserdata(L, library);
+  lua_setfield(L, LUA_REGISTRYINDEX, name);


It might be better to use a table in the registry to keep plugins together, rather than putting them in the registry proper since its a user defined name.

I didn't want to bother, but yeah; it's probably a better call for future-proofing. I'll refactor.

adamharrison · 2021-09-21T03:38:37Z

OK, that should incorporate @Mehgugs 's changes.

…ndant C code that's already encapsulated within PLATFORM.

adamharrison · 2021-09-21T03:52:29Z

Would like a comment on 713ef78, if anyone has one. I removed my setglobal calls to LINUX and WINDOWS, as they seemed redundant; I see we actually have the platform in PLATFORM; I also snipped out the MACOS call as well; it seems like the only place that was used in lua was the keymap, and I just changed that to use platform. I know this should probably be a separate PR, but I figured given that I was already adding code in this PR to accommodate this paradigm, it may be a good place to just cut it out instead. Thoughts?

franko · 2021-09-21T10:25:07Z

Would like a comment on 713ef78, if anyone has one. I removed my setglobal calls to LINUX and WINDOWS, as they seemed redundant; I see we actually have the platform in PLATFORM; I also snipped out the MACOS call as well; it seems like the only place that was used in lua was the keymap, and I just changed that to use platform. I know this should probably be a separate PR, but I figured given that I was already adding code in this PR to accommodate this paradigm, it may be a good place to just cut it out instead. Thoughts?

Of course I am fine with this change. The only thing is, I think platform should be called "macOS" as this is the real official name.

adamharrison · 2021-09-21T15:55:33Z

Of course I am fine with this change. The only thing is, I think platform should be called "macOS" as this is the real official name.

Hrm. The only issue with this is that we are pulling the stuff directly from SDL. Do you want to do a translation at that level before it goes into Platform?

Mehgugs · 2021-09-22T19:38:28Z

SDL's names do seem a bit arbitrary:

#elif __MACOS__
    return "MacOS Classic";
#elif __MACOSX__
    return "Mac OS X";

I think almost everyone would be ambivalent about translating, but this would probably be a breaking change and maybe not a super necessary one?

Of course I am fine with this change. The only thing is, I think platform should be called "macOS" as this is the real official name.

adamharrison · 2021-09-22T19:59:48Z

It would be breaking, yeah. That's not necessarily the end of the world if we can verify that almost no one is using PLATFORM or MACOS.

franko · 2021-09-22T20:23:53Z

If it is from SDL2 we can stick with their names. I think they set them up before Apple changed the name of Mac OS X.

adamharrison · 2021-09-22T21:26:16Z

OK, that should be the last change.

adamharrison · 2021-09-22T21:41:30Z

I'm going to merge this if no one has objections tomorrow, mmmk?

adamharrison · 2021-09-23T15:45:42Z

Last chance! Or it's going in.

franko · 2021-09-26T20:57:23Z

I have to have a closer look at that but I guess it is useful for plugin development. Thank you for this contribution.

Added in searcher.

a66a76f

adamharrison marked this pull request as draft September 14, 2021 03:43

Added in sample plugin and tested things out. Works.

e9f48ce

Jan200101 reviewed Sep 14, 2021

View reviewed changes

Moved things around, fixed a few things up.

377ce1c

adamharrison added 3 commits September 16, 2021 15:47

Moving things into final positions.

2987554

Cleaned up, added utility API.

03b467d

Added some comments.

801b7dd

Undid deletion.

10c3c9d

adamharrison marked this pull request as ready for review September 16, 2021 20:26

Should be elif.

1721b8f

Split entrypoints in half.

fbc11c0

Mehgugs reviewed Sep 18, 2021

View reviewed changes

adamharrison added 2 commits September 20, 2021 23:33

Incorporated some suggestions, and some functions.

3ca1277

Added in plugin table.

c01c5a2

adamharrison added 2 commits September 20, 2021 23:42

Less C code, and more namespacing is better.

e135294

Removed extra macros, used PLATFORM. Also removed MACOS, as it's redu…

713ef78

…ndant C code that's already encapsulated within PLATFORM.

Removed extra boolean.

5ffe4ea

adamharrison mentioned this pull request Sep 21, 2021

Link Lua dynamically by default #41

Closed

adamharrison added 2 commits September 22, 2021 17:24

Removed searchers[4].

b8da46e

Mispelling.

466464d

adamharrison merged commit 8c32950 into lite-xl:master Sep 23, 2021

takase1121 mentioned this pull request Nov 6, 2021

fixing native plugin API #657

Merged

Native Plugins #527

Native Plugins #527

Conversation

adamharrison commented Sep 14, 2021

adamharrison commented Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamharrison Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamharrison commented Sep 16, 2021 • edited Loading

adamharrison commented Sep 16, 2021 • edited Loading

adamharrison commented Sep 16, 2021

adamharrison commented Sep 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mehgugs Sep 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamharrison commented Sep 21, 2021

adamharrison commented Sep 21, 2021

franko commented Sep 21, 2021

adamharrison commented Sep 21, 2021

Mehgugs commented Sep 22, 2021 • edited Loading

adamharrison commented Sep 22, 2021

franko commented Sep 22, 2021

adamharrison commented Sep 22, 2021

adamharrison commented Sep 22, 2021

adamharrison commented Sep 23, 2021

franko commented Sep 26, 2021

adamharrison commented Sep 14, 2021 •

edited

Loading

adamharrison Sep 14, 2021 •

edited

Loading

adamharrison commented Sep 16, 2021 •

edited

Loading

adamharrison commented Sep 16, 2021 •

edited

Loading

Mehgugs Sep 18, 2021 •

edited

Loading

Mehgugs commented Sep 22, 2021 •

edited

Loading