lgi-devs / lgi

Dynamic Lua binding to GObject libraries using GObject-Introspection
MIT License
440 stars 70 forks source link

Add (proper) support for Lua 5.4 #247

Closed anatol closed 2 years ago

anatol commented 4 years ago

Lua 5.4 has been officially released. Trying to compile lgi with Lua 5.4 gives me a compilation error:

callable.c: In function ‘closure_callback’:
callable.c:1237:13: error: too few arguments to function ‘lua_resume’
 1237 |       res = lua_resume (L, NULL, npos);
      |             ^~~~~~~~~~
In file included from lgi.h:11,
                 from callable.c:12:
/usr/include/lua.h:300:15: note: declared here
  300 | LUA_API int  (lua_resume)     (lua_State *L, lua_State *from, int narg,
      |               ^~~~~~~~~~
make[1]: *** [Makefile:63: callable.o] Error 1
make[1]: *** Waiting for unfinished jobs....
psychon commented 4 years ago

See #199. One of the tests involving coroutines runs into "C stack overflow". The gdb backtrace shows that there are no C stack overflows at the time. Feel free to help figuring out what is going on.

Edit: See https://github.com/pavouk/lgi/pull/199#issuecomment-657180751 for more details.

thmo commented 4 years ago

Maybe this: https://www.lua.org/bugs.html#5.4.0-9 ?

RhodiumToad commented 4 years ago

From the stacktrace from the "C stack overflow" error, I think the problem is that the code is doing lua_pcall using a Lua thread that has never been resumed. Such threads have a very small initial C stack limit, the idea being that lua_resume sets the stack limit based on the "from" thread parameter (which is also not being set, but that's a different issue).

The bug 5.4.0-9 is not relevant because that only causes an actual stack overflow to go undetected (causing a later crash), it does not cause spurious errors.

v1993 commented 4 years ago

Lua 5.4.1 RC1 is out and fixes all bugs of 5.4.0. I suggest running tests against it.

psychon commented 4 years ago

Tried with a branch pointing at https://github.com/psychon/lgi/commit/1b3f5e1ce45b1d32914f370d707108b3f37857c5. Result is:

cd .. && LD_LIBRARY_PATH=tests:$LD_LIBRARY_PATH \
        GI_TYPELIB_PATH=tests:$GI_TYPELIB_PATH \
        LUA_PATH="./?.lua;;" \
        LUA_CPATH="./?.so;;" \
        /usr/bin/dbus-launch lua tests/test.lua
(process:6414): Lgi-WARNING **: Error raised while calling 'lgi.cbk (number): Regress': attempt to call a number value
(process:6414): Lgi-WARNING **: Error raised while calling 'lgi.cbk (string: 0x695810): Regress': attempt to call a string value
(process:6414): Lgi-WARNING **: Error raised while calling 'lgi.cbk (table: 0x782df0): Regress': attempt to call a table value
(process:6414): Lgi-WARNING **: Error raised while calling 'lgi.cbk (table: 0x7196e0): Regress': attempt to call a table value
gireg   : all 114 tests passed.
marshal : all 1 tests passed.
corocbk : all 3 tests passed.
record  : all 1 tests passed.
** (lua:6414): WARNING **: Error retrieving accessibility bus address: org.freedesktop.DBus.Error.ServiceUnknown: The name org.a11y.Bus was not provided by any .service files
(lua:6414): GLib-GObject-CRITICAL **: g_param_spec_enum: assertion 'G_TYPE_IS_ENUM (enum_type)' failed
gobject : all 18 tests passed.
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received
The build has been terminated

https://travis-ci.org/github/psychon/lgi/builds/732008009

This one sounds new and does not happen locally for me, could be the cause:

(lua:6414): GLib-GObject-CRITICAL **: g_param_spec_enum: assertion 'G_TYPE_IS_ENUM (enum_type)' failed
thmo commented 4 years ago

@psychon : Can you comment on the analysis by @RhodiumToad in https://github.com/pavouk/lgi/issues/247#issuecomment-687906637 ?

psychon commented 4 years ago

@thmo Sure... uhm... well... sounds like a valid theory and @RhodiumToad seems to understand more about Lua internals than I do. That's about all that I can say about the comment. If this theory is correct, then I still have no idea what to do about this problem. When LGI gets a call from C (for a callback), it tries to find a lua thread where the call can be done. If none is found, lua_newthread() is called and a lua_pcall() is done. I do not see how this could be changed to lua_resume(), because we are "in a random context" and we certainly cannot allow Lua to longjmp() away here.

What kind of comment were you hoping for?

thmo commented 4 years ago

So we'd need input from someone with profound Lua knowledge on how to properly increase the stack size between lua_newthread() and lua_pcall. Hopefully that should be possible.

RhodiumToad commented 4 years ago

I think the easiest solution is probably to support only 5.4.2+ when that comes out, because that seems to fix the issue by reverting to the old scheme for handling the C stack.

5.4.2+ can be checked for at runtime by testing whether lua_setcstacklimit can actually change the limit (on 5.4.2 it will ignore attempts to change the limit and return a constant).

v1993 commented 4 years ago

Taking a look at Lua source code:

In lua_resume:
  if (from == NULL)
    L->nCcalls = CSTACKTHREAD;

You can just assign C stack limit directly like this. I'm not exactly sure how portable is this across Lua versions, however.

v1993 commented 4 years ago

Also note that Lua uses very rouge C stack overflow detection that is more than likely to trigger long before actual stack overflow happens on most PCs. That's why you won't find gdb reporting overflow when Lua does.

RhodiumToad commented 4 years ago
L->nCcalls = CSTACKTHREAD;

You can just assign C stack limit directly like this. I'm not exactly sure how portable is this across Lua versions, however.

No you can't, because lua_State is opaque from the point of view of the module (it is not defined in lua.h). Also the meaning of L->nCcalls isn't the same even between different point releases.

RhodiumToad commented 4 years ago

Also note that Lua uses very rouge C stack overflow detection that is more than likely to trigger long before actual stack overflow happens on most PCs. That's why you won't find gdb reporting overflow when Lua does.

Checking stack limits in a portable way is quite hard (some of the weirder architectures such as ia64 have things like multiple stacks). Lua's C stack limit (in versions except 5.4.0 and .1) is only there to catch uncontrolled on unreasonable nesting in C functions; and even then it doesn't do a perfect job, since string.gsub, string.format, and table.concat can be made to use very large amounts of stack space.

v1993 commented 3 years ago

Lua 5.4.2 is out, reversing to stackless implementation. Try testing with it.

xleph commented 3 years ago

gcc -v

gcc version 10.2.0 (GCC)

lua -v

5.4.2

sudo luarocks install lgi

... callable.c:1237:13: error: too few arguments to function 'lua resume' 1237 res = lua_resume (L, NULL, npos);

...

bug's still here. Some additional output if you want it: https://bit.ly/3na7lvj

thmo commented 3 years ago
... callable.c:1237:13: error: too few arguments to function 'lua resume' 1237 res = lua_resume (L, NULL, npos);

...

You need the patch from #199 , which has been merged but is not part of any released version.

ntd commented 2 years ago

I just built lgi with lua 5.4 and make check passes: is the problem vanished or is there something I am missing?

psychon commented 2 years ago

I'm fine with just assuming that this works, so let's cross some fingers and just close this.

Elv13 commented 2 years ago

We have seen some awesome -v with 5.4 in the last year. It compile and doesn't explode

Proof Dockerfile:

FROM ubuntu:21.04

RUN cp /etc/apt/sources.list /tmp; \
    sed -i 's/deb h/deb-src h/g' /etc/apt/sources.list; \
    cat /tmp/sources.list >> /etc/apt/sources.list

RUN apt update && apt upgrade -y && apt install git -y

RUN cat /etc/apt/sources.list

RUN DEBIAN_FRONTEND=noninteractive apt build-dep awesome lua-lgi -y
RUN apt remove lua-lgi lua-lgi-dev lua5.2 lua5.3 lua5.1 \
    liblua5.3-dev liblua5.2-dev liblua5.1-dev liblua5.1-0 \
    liblua5.1-0-dev liblua5.2-0 liblua5.3-0 -y
RUN apt install liblua5.4-dev libxcb-xfixes0-dev liblua5.4-0-dbg librsvg2-dev -y
RUN git clone https://github.com/pavouk/lgi.git
RUN sed -i 's|PKGS = |PKGS = lua |' /lgi/lgi/Makefile
RUN cd lgi && make PREFIX=/usr LUA_VERSION=5.4 && \
    make install LUA_VERSION=5.4 PREFIX=/usr

RUN git clone https://github.com/awesomewm/awesome
RUN cd awesome; sed -i 's|#error|//#error|' luaa.h
RUN cd awesome; sed -i 's|5.4.0|5.5.0|g' awesomeConfig.cmake
RUN cd awesome && make package

The only "patch" is to avoid calling init_rng(); and even there to fix is minor.

root@3eb4f7cca6c8:/awesome# ./build/awesome -v
awesome v4.3-1336-g21b908be-dirty (Too long)
 • Compiled against Lua 5.4.2 (running with 0.9.2)
 • API level: 4
 • D-Bus support: yes
 • xcb-errors support: no
 • execinfo support: yes
 • xcb-randr version: 1.6
 • LGI version: /usr/share/lua/5.4/lgi/version.lua
 • Transparency enabled: yes
 • Custom search paths: no

image

alerque commented 2 years ago

@Elv13 Do you have a link to the necessary patch? I'd love to get the Arch Linux awesome package updated to Lua 5.4 if its possible.

Elv13 commented 2 years ago
diff --git a/awesome.c b/awesome.c
index 24ee52d8..ef6c339e 100644
--- a/awesome.c
+++ b/awesome.c
@@ -37,6 +37,7 @@
 #include "systray.h"
 #include "xwindow.h"
 #include "options.h"
+#include "math.h"

 #include <getopt.h>

@@ -84,10 +85,16 @@ init_rng(void)
     lua_getfield(L, -1, "randomseed");

     /* Push a seed */
-    lua_pushnumber(L, g_random_int() + g_random_double());
+    lua_pushnumber(L, floor(g_random_int() + g_random_double()));

     /* Call math.randomseed */
-    lua_call(L, 1, 0);
+    if(lua_pcall(L, 1, 0, 0))
+    {
+        warn("Random number generator initialization failed: %s", lua_tostring(L, -1));
+        /* Remove error function and error string */
+        lua_pop(L, 2);
+        return;
+    }

     /* Remove "math" global */
     lua_pop(L, 1);
diff --git a/awesomeConfig.cmake b/awesomeConfig.cmake
index 5f292aaa..592c2b25 100644
--- a/awesomeConfig.cmake
+++ b/awesomeConfig.cmake
@@ -73,9 +73,9 @@ if (NOT LUA_FOUND)
 endif()

 set(LUA_FULL_VERSION "${LUA_VERSION_MAJOR}.${LUA_VERSION_MINOR}.${LUA_VERSION_PATCH}")
-# 5.1 <= LUA_VERSION < 5.4
-if(NOT ((LUA_FULL_VERSION VERSION_EQUAL 5.1.0 OR LUA_FULL_VERSION VERSION_GREATER 5.1.0) AND LUA_FULL_VERSION VERSION_LESS 5.4.0))
-    message(FATAL_ERROR "Awesome only supports Lua versions 5.1-5.3, please refer to"
+# 5.1 <= LUA_VERSION < 5.5
+if(NOT ((LUA_FULL_VERSION VERSION_EQUAL 5.1.0 OR LUA_FULL_VERSION VERSION_GREATER 5.1.0) AND LUA_FULL_VERSION VERSION_LESS 5.5.0))
+    message(FATAL_ERROR "Awesome only supports Lua versions 5.1-5.4, please refer to"
                         "https://awesomewm.org/apidoc/documentation/10-building-and-testing.md.html#Building")
 endif()

diff --git a/luaa.h b/luaa.h
index fae07270..04e28342 100644
--- a/luaa.h
+++ b/luaa.h
@@ -31,8 +31,8 @@
 #include "common/lualib.h"
 #include "common/luaclass.h"

-#if !(501 <= LUA_VERSION_NUM && LUA_VERSION_NUM < 504)
-#error "Awesome only supports Lua versions 5.1-5.3 and LuaJIT2, please refer to https://awesomewm.org/apidoc/documentation/10-building-and-testing.md.html#Building"
+#if !(501 <= LUA_VERSION_NUM && LUA_VERSION_NUM < 505)
+#error "Awesome only supports Lua versions 5.1-5.4 and LuaJIT2, please refer to https://awesomewm.org/apidoc/documentation/10-building-and-testing.md.html#Building"
 #endif

 #define luaA_deprecate(L, repl) \