* [Tarantool-patches] [PATCH luajit 0/2] gc: handle errors in finalizers @ 2023-10-27 12:57 Maksim Kokryashkin via Tarantool-patches 2023-10-27 12:57 ` [Tarantool-patches] [PATCH luajit 1/2] Print errors from __gc finalizers instead of rethrowing them Maksim Kokryashkin via Tarantool-patches 2023-10-27 12:57 ` [Tarantool-patches] [PATCH luajit 2/2] Fix last commit Maksim Kokryashkin via Tarantool-patches 0 siblings, 2 replies; 7+ messages in thread From: Maksim Kokryashkin via Tarantool-patches @ 2023-10-27 12:57 UTC (permalink / raw) To: tarantool-patches, sergeyb, skaplun, m.kokryashkin; +Cc: Maksim Kokryashkin This patchset introduces VM event for finalizer error handling. The first patch contains the VM event implementation, the second is just a minor fixup for the first one. Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-946-print-errors-from-gc-fin PR: https://github.com/tarantool/tarantool/pull/9315 Issue: https://github.com/LuaJIT/LuaJIT/pull/946 Mike Pall (2): Print errors from __gc finalizers instead of rethrowing them. Fix last commit. src/Makefile.dep.original | 14 +++---- src/lib_aux.c | 35 +++++++++++++++- src/lj_gc.c | 10 ++++- src/lj_vmevent.h | 7 ++-- test/PUC-Rio-Lua-5.1-tests/gc.lua | 12 +++--- .../fix-finalizer-error-handler-init.test.c | 30 +++++++++++++ .../lj-946-print-errors-from-gc-fin.test.lua | 42 +++++++++++++++++++ 7 files changed, 130 insertions(+), 20 deletions(-) create mode 100644 test/tarantool-c-tests/fix-finalizer-error-handler-init.test.c create mode 100644 test/tarantool-tests/lj-946-print-errors-from-gc-fin.test.lua -- 2.39.3 (Apple Git-145) ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH luajit 1/2] Print errors from __gc finalizers instead of rethrowing them. 2023-10-27 12:57 [Tarantool-patches] [PATCH luajit 0/2] gc: handle errors in finalizers Maksim Kokryashkin via Tarantool-patches @ 2023-10-27 12:57 ` Maksim Kokryashkin via Tarantool-patches 2023-11-01 8:15 ` Sergey Kaplun via Tarantool-patches 2023-10-27 12:57 ` [Tarantool-patches] [PATCH luajit 2/2] Fix last commit Maksim Kokryashkin via Tarantool-patches 1 sibling, 1 reply; 7+ messages in thread From: Maksim Kokryashkin via Tarantool-patches @ 2023-10-27 12:57 UTC (permalink / raw) To: tarantool-patches, sergeyb, skaplun, m.kokryashkin From: Mike Pall <mike> Finalizers are not supposed to throw errors -- this is undefined behavior. Lua 5.1 - 5.3 and (previously) LuaJIT rethrow the error. This randomly breaks some unrelated code that just happens to do an allocation. Bad. Lua 5.4 catches the error and emits a warning instead. But warnings are not enabled by default, so it fails silently. Even worse. LuaJIT (now) catches the error and emits a VM event. The default event handler function prints "ERROR in finalizer: ...". Set a custom handler function with: jit.attach(handler, "errfin") (cherry-picked from commit 1c279127050e86e99970100e9c42e0f09cd54ab7) The test for an error in a finalizer from the PUC-Rio suite is disabled since these errors are now handled with VM event. Sergey Kaplun: * added the test for the problem Part of tarantool/tarantool#9145 --- src/Makefile.dep.original | 14 +++---- src/lib_aux.c | 33 ++++++++++++++- src/lj_gc.c | 10 ++++- src/lj_vmevent.h | 7 ++-- test/PUC-Rio-Lua-5.1-tests/gc.lua | 12 +++--- .../lj-946-print-errors-from-gc-fin.test.lua | 42 +++++++++++++++++++ 6 files changed, 98 insertions(+), 20 deletions(-) create mode 100644 test/tarantool-tests/lj-946-print-errors-from-gc-fin.test.lua diff --git a/src/Makefile.dep.original b/src/Makefile.dep.original index d35b6d9a..53426a7e 100644 --- a/src/Makefile.dep.original +++ b/src/Makefile.dep.original @@ -1,6 +1,6 @@ lib_aux.o: lib_aux.c lua.h luaconf.h lauxlib.h lj_obj.h lj_def.h \ lj_arch.h lj_err.h lj_errmsg.h lj_state.h lj_trace.h lj_jit.h lj_ir.h \ - lj_dispatch.h lj_bc.h lj_traceerr.h lj_lib.h lj_alloc.h + lj_dispatch.h lj_bc.h lj_traceerr.h lj_lib.h lj_alloc.h lj_vmevent.h lib_base.o: lib_base.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h \ lj_def.h lj_arch.h lj_gc.h lj_err.h lj_errmsg.h lj_debug.h lj_str.h \ lj_tab.h lj_meta.h lj_state.h lj_frame.h lj_bc.h lj_ctype.h lj_cconv.h \ @@ -123,7 +123,7 @@ lj_func.o: lj_func.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h lj_gc.h \ lj_gc.o: lj_gc.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h lj_gc.h \ lj_err.h lj_errmsg.h lj_buf.h lj_str.h lj_tab.h lj_func.h lj_udata.h \ lj_meta.h lj_state.h lj_frame.h lj_bc.h lj_ctype.h lj_cdata.h lj_trace.h \ - lj_jit.h lj_ir.h lj_dispatch.h lj_traceerr.h lj_vm.h + lj_jit.h lj_ir.h lj_dispatch.h lj_traceerr.h lj_vm.h lj_vmevent.h lj_gdbjit.o: lj_gdbjit.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \ lj_gc.h lj_err.h lj_errmsg.h lj_debug.h lj_frame.h lj_bc.h lj_buf.h \ lj_str.h lj_strfmt.h lj_jit.h lj_ir.h lj_dispatch.h @@ -238,11 +238,11 @@ ljamalg.o: ljamalg.c lua.h luaconf.h lauxlib.h lj_assert.c lj_obj.h lj_def.h \ lj_arch.h lj_gc.c lj_gc.h lj_err.h lj_errmsg.h lj_buf.h lj_str.h lj_tab.h \ lj_func.h lj_udata.h lj_meta.h lj_state.h lj_frame.h lj_bc.h lj_ctype.h \ lj_cdata.h lj_trace.h lj_jit.h lj_ir.h lj_dispatch.h lj_traceerr.h \ - lj_vm.h lj_err.c lj_debug.h lj_ff.h lj_ffdef.h lj_strfmt.h lj_char.c \ - lj_char.h lj_bc.c lj_bcdef.h lj_obj.c lj_buf.c lj_wbuf.c lj_wbuf.h lj_utils.h \ - lj_str.c lj_tab.c lj_func.c lj_udata.c lj_meta.c lj_strscan.h lj_lib.h \ - lj_debug.c lj_state.c lj_lex.h lj_alloc.h luajit.h lj_dispatch.c \ - lj_ccallback.h lj_profile.h lj_memprof.h lj_vmevent.c lj_vmevent.h \ + lj_vm.h lj_vmevent.h lj_err.c lj_debug.h lj_ff.h lj_ffdef.h lj_strfmt.h \ + lj_char.c lj_char.h lj_bc.c lj_bcdef.h lj_obj.c lj_buf.c lj_wbuf.c lj_wbuf.h \ + lj_utils.h lj_str.c lj_tab.c lj_func.c lj_udata.c lj_meta.c lj_strscan.h \ + lj_lib.h lj_debug.c lj_state.c lj_lex.h lj_alloc.h luajit.h lj_dispatch.c \ + lj_ccallback.h lj_profile.h lj_memprof.h lj_vmevent.c \ lj_vmmath.c lj_strscan.c lj_strfmt.c lj_strfmt_num.c lj_api.c lj_mapi.c \ lmisclib.h lj_profile.c lj_profile_timer.h lj_profile_timer.c \ lj_memprof.c lj_lex.c lualib.h lj_parse.h lj_parse.c lj_bcread.c \ diff --git a/src/lib_aux.c b/src/lib_aux.c index c40565c3..d532a12f 100644 --- a/src/lib_aux.c +++ b/src/lib_aux.c @@ -21,6 +21,7 @@ #include "lj_state.h" #include "lj_trace.h" #include "lj_lib.h" +#include "lj_vmevent.h" #if LJ_TARGET_POSIX #include <sys/wait.h> @@ -311,6 +312,18 @@ static int panic(lua_State *L) return 0; } +#ifndef LUAJIT_DISABLE_VMEVENT +static int error_finalizer(lua_State *L) +{ + const char *s = lua_tostring(L, -1); + fputs("ERROR in finalizer: ", stderr); + fputs(s ? s : "?", stderr); + fputc('\n', stderr); + fflush(stderr); + return 0; +} +#endif + #ifdef LUAJIT_USE_SYSMALLOC #if LJ_64 && !LJ_GC64 && !defined(LUAJIT_USE_VALGRIND) @@ -332,7 +345,15 @@ static void *mem_alloc(void *ud, void *ptr, size_t osize, size_t nsize) LUALIB_API lua_State *luaL_newstate(void) { lua_State *L = lua_newstate(mem_alloc, NULL); - if (L) G(L)->panic = panic; + if (L) { + G(L)->panic = panic; +#ifndef LUAJIT_DISABLE_VMEVENT + luaL_findtable(L, LUA_REGISTRYINDEX, LJ_VMEVENTS_REGKEY, LJ_VMEVENTS_HSIZE); + lua_pushcfunction(L, error_finalizer); + lua_rawseti(L, -2, VMEVENT_HASH(LJ_VMEVENT_ERRFIN)); + G(L)->vmevmask = VMEVENT_MASK(LJ_VMEVENT_ERRFIN); +#endif + } return L; } @@ -350,7 +371,15 @@ LUALIB_API lua_State *luaL_newstate(void) #else L = lua_newstate(lj_alloc_f, ud); #endif - if (L) G(L)->panic = panic; + if (L) { + G(L)->panic = panic; +#ifndef LUAJIT_DISABLE_VMEVENT + luaL_findtable(L, LUA_REGISTRYINDEX, LJ_VMEVENTS_REGKEY, LJ_VMEVENTS_HSIZE); + lua_pushcfunction(L, error_finalizer); + lua_rawseti(L, -2, VMEVENT_HASH(LJ_VMEVENT_ERRFIN)); + G(L)->vmevmask = VMEVENT_MASK(LJ_VMEVENT_ERRFIN); +#endif + } return L; } diff --git a/src/lj_gc.c b/src/lj_gc.c index 19d4c963..591862b3 100644 --- a/src/lj_gc.c +++ b/src/lj_gc.c @@ -27,6 +27,7 @@ #include "lj_trace.h" #include "lj_dispatch.h" #include "lj_vm.h" +#include "lj_vmevent.h" #define GCSTEPSIZE 1024u #define GCSWEEPMAX 40 @@ -515,8 +516,13 @@ static void gc_call_finalizer(global_State *g, lua_State *L, hook_restore(g, oldh); if (LJ_HASPROFILE && (oldh & HOOK_PROFILE)) lj_dispatch_update(g); g->gc.threshold = oldt; /* Restore GC threshold. */ - if (errcode) - lj_err_throw(L, errcode); /* Propagate errors. */ + if (errcode) { + ptrdiff_t errobj = savestack(L, L->top-1); /* Stack may be resized. */ + lj_vmevent_send(L, ERRFIN, + copyTV(L, L->top++, restorestack(L, errobj)); + ); + L->top--; + } } /* Finalize one userdata or cdata object from the mmudata list. */ diff --git a/src/lj_vmevent.h b/src/lj_vmevent.h index 050fb4dd..56c0d798 100644 --- a/src/lj_vmevent.h +++ b/src/lj_vmevent.h @@ -24,9 +24,10 @@ /* VM event IDs. */ typedef enum { VMEVENT_DEF(BC, 0x00003883), - VMEVENT_DEF(TRACE, 0xb2d91467), - VMEVENT_DEF(RECORD, 0x9284bf4f), - VMEVENT_DEF(TEXIT, 0xb29df2b0), + VMEVENT_DEF(TRACE, 0x12d91467), + VMEVENT_DEF(RECORD, 0x1284bf4f), + VMEVENT_DEF(TEXIT, 0x129df2b0), + VMEVENT_DEF(ERRFIN, 0x12d93888), LJ_VMEVENT__MAX } VMEvent; diff --git a/test/PUC-Rio-Lua-5.1-tests/gc.lua b/test/PUC-Rio-Lua-5.1-tests/gc.lua index 10e1f2dd..05ac0d11 100644 --- a/test/PUC-Rio-Lua-5.1-tests/gc.lua +++ b/test/PUC-Rio-Lua-5.1-tests/gc.lua @@ -260,13 +260,13 @@ u, m = nil collectgarbage() assert(m==10) - +-- The test below is disabled for LuaJIT, since it handles errors from +-- GC finalizers via VM event. -- errors during collection -u = newproxy(true) -getmetatable(u).__gc = function () error "!!!" end -u = nil -assert(not pcall(collectgarbage)) - +-- u = newproxy(true) +-- getmetatable(u).__gc = function () error "!!!" end +-- u = nil +-- assert(not pcall(collectgarbage)) if not rawget(_G, "_soft") then print("deep structures") diff --git a/test/tarantool-tests/lj-946-print-errors-from-gc-fin.test.lua b/test/tarantool-tests/lj-946-print-errors-from-gc-fin.test.lua new file mode 100644 index 00000000..cce417d2 --- /dev/null +++ b/test/tarantool-tests/lj-946-print-errors-from-gc-fin.test.lua @@ -0,0 +1,42 @@ +local tap = require('tap') +local test = tap.test('lj-946-print-errors-from-gc-fin'):skipcond({ + ['Test requires JIT enabled'] = not jit.status(), +}) + +test:plan(2) + +local ffi = require('ffi') +local error_in_finalizer = false + +local function errfin_handler() + error_in_finalizer = true +end + +local function new_bad_cdata() + return ffi.gc(ffi.new('char [?]', 1024), 'uncallable string') +end + +local function test_f() + collectgarbage('collect') + -- Make GC aggressive enough to end atomic phase before + -- exit from the trace. + collectgarbage('setstepmul', 400) + -- Number of iterations is empirical, just big enough for the + -- issue to strike. + for _ = 1, 4000 do + new_bad_cdata() + end +end + +jit.opt.start('hotloop=1') +-- Handler is registered, but never called before the patch, +-- but should be called after the patch. +jit.attach(errfin_handler, 'errfin') +local status = pcall(test_f) +-- We have to stop GC now because any step raises the error due to +-- cursed cdata objects. +collectgarbage('stop') +test:ok(status, 'test function completed successfully') +test:ok(error_in_finalizer, 'error handler called') + +test:done(true) -- 2.39.3 (Apple Git-145) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] Print errors from __gc finalizers instead of rethrowing them. 2023-10-27 12:57 ` [Tarantool-patches] [PATCH luajit 1/2] Print errors from __gc finalizers instead of rethrowing them Maksim Kokryashkin via Tarantool-patches @ 2023-11-01 8:15 ` Sergey Kaplun via Tarantool-patches 0 siblings, 0 replies; 7+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2023-11-01 8:15 UTC (permalink / raw) To: Maksim Kokryashkin; +Cc: tarantool-patches Hi, Maksim! Thanks for the patch! Please, consider my comments below. On 27.10.23, Maksim Kokryashkin wrote: > From: Mike Pall <mike> > > Finalizers are not supposed to throw errors -- this is undefined behavior. > Lua 5.1 - 5.3 and (previously) LuaJIT rethrow the error. This randomly > breaks some unrelated code that just happens to do an allocation. Bad. > Lua 5.4 catches the error and emits a warning instead. But warnings are > not enabled by default, so it fails silently. Even worse. > LuaJIT (now) catches the error and emits a VM event. The default event > handler function prints "ERROR in finalizer: ...". > Set a custom handler function with: jit.attach(handler, "errfin") > > (cherry-picked from commit 1c279127050e86e99970100e9c42e0f09cd54ab7) > > The test for an error in a finalizer from the PUC-Rio suite is > disabled since these errors are now handled with VM event. I suppose, that it's worth to mention that there is a regression (and maybe short describe it) inside this commit, that will be fixed in the next commit. > > Sergey Kaplun: Typo: s/Sergey Kaplun/Maxim Kokryashkin/ > * added the test for the problem > > Part of tarantool/tarantool#9145 > --- > src/Makefile.dep.original | 14 +++---- > src/lib_aux.c | 33 ++++++++++++++- > src/lj_gc.c | 10 ++++- > src/lj_vmevent.h | 7 ++-- > test/PUC-Rio-Lua-5.1-tests/gc.lua | 12 +++--- > .../lj-946-print-errors-from-gc-fin.test.lua | 42 +++++++++++++++++++ > 6 files changed, 98 insertions(+), 20 deletions(-) > create mode 100644 test/tarantool-tests/lj-946-print-errors-from-gc-fin.test.lua > > diff --git a/src/Makefile.dep.original b/src/Makefile.dep.original > index d35b6d9a..53426a7e 100644 > --- a/src/Makefile.dep.original > +++ b/src/Makefile.dep.original <snipped> > diff --git a/src/lib_aux.c b/src/lib_aux.c > index c40565c3..d532a12f 100644 > --- a/src/lib_aux.c > +++ b/src/lib_aux.c > @@ -21,6 +21,7 @@ > #include "lj_state.h" > #include "lj_trace.h" > #include "lj_lib.h" > +#include "lj_vmevent.h" > > #if LJ_TARGET_POSIX > #include <sys/wait.h> > @@ -311,6 +312,18 @@ static int panic(lua_State *L) > return 0; > } > > +#ifndef LUAJIT_DISABLE_VMEVENT > +static int error_finalizer(lua_State *L) > +{ > + const char *s = lua_tostring(L, -1); > + fputs("ERROR in finalizer: ", stderr); > + fputs(s ? s : "?", stderr); > + fputc('\n', stderr); > + fflush(stderr); > + return 0; > +} > +#endif > + > #ifdef LUAJIT_USE_SYSMALLOC > > #if LJ_64 && !LJ_GC64 && !defined(LUAJIT_USE_VALGRIND) > @@ -332,7 +345,15 @@ static void *mem_alloc(void *ud, void *ptr, size_t osize, size_t nsize) > LUALIB_API lua_State *luaL_newstate(void) > { > lua_State *L = lua_newstate(mem_alloc, NULL); > - if (L) G(L)->panic = panic; > + if (L) { > + G(L)->panic = panic; > +#ifndef LUAJIT_DISABLE_VMEVENT > + luaL_findtable(L, LUA_REGISTRYINDEX, LJ_VMEVENTS_REGKEY, LJ_VMEVENTS_HSIZE); > + lua_pushcfunction(L, error_finalizer); > + lua_rawseti(L, -2, VMEVENT_HASH(LJ_VMEVENT_ERRFIN)); > + G(L)->vmevmask = VMEVENT_MASK(LJ_VMEVENT_ERRFIN); > +#endif > + } > return L; > } > > @@ -350,7 +371,15 @@ LUALIB_API lua_State *luaL_newstate(void) > #else > L = lua_newstate(lj_alloc_f, ud); > #endif > - if (L) G(L)->panic = panic; > + if (L) { > + G(L)->panic = panic; > +#ifndef LUAJIT_DISABLE_VMEVENT > + luaL_findtable(L, LUA_REGISTRYINDEX, LJ_VMEVENTS_REGKEY, LJ_VMEVENTS_HSIZE); > + lua_pushcfunction(L, error_finalizer); > + lua_rawseti(L, -2, VMEVENT_HASH(LJ_VMEVENT_ERRFIN)); > + G(L)->vmevmask = VMEVENT_MASK(LJ_VMEVENT_ERRFIN); > +#endif > + } > return L; > } Side note: Yes, of course there is no need to declare another routine for this... Copy-past is better. Sigh... Please mention when (at the moment of Lua initialization) the default VM handler is set. > > diff --git a/src/lj_gc.c b/src/lj_gc.c > index 19d4c963..591862b3 100644 > --- a/src/lj_gc.c > +++ b/src/lj_gc.c > @@ -27,6 +27,7 @@ > #include "lj_trace.h" > #include "lj_dispatch.h" > #include "lj_vm.h" > +#include "lj_vmevent.h" > > #define GCSTEPSIZE 1024u > #define GCSWEEPMAX 40 > @@ -515,8 +516,13 @@ static void gc_call_finalizer(global_State *g, lua_State *L, > hook_restore(g, oldh); > if (LJ_HASPROFILE && (oldh & HOOK_PROFILE)) lj_dispatch_update(g); > g->gc.threshold = oldt; /* Restore GC threshold. */ > - if (errcode) > - lj_err_throw(L, errcode); /* Propagate errors. */ > + if (errcode) { > + ptrdiff_t errobj = savestack(L, L->top-1); /* Stack may be resized. */ > + lj_vmevent_send(L, ERRFIN, > + copyTV(L, L->top++, restorestack(L, errobj)); > + ); > + L->top--; > + } > } > > /* Finalize one userdata or cdata object from the mmudata list. */ > diff --git a/src/lj_vmevent.h b/src/lj_vmevent.h > index 050fb4dd..56c0d798 100644 > --- a/src/lj_vmevent.h > +++ b/src/lj_vmevent.h > @@ -24,9 +24,10 @@ > /* VM event IDs. */ > typedef enum { > VMEVENT_DEF(BC, 0x00003883), > - VMEVENT_DEF(TRACE, 0xb2d91467), > - VMEVENT_DEF(RECORD, 0x9284bf4f), > - VMEVENT_DEF(TEXIT, 0xb29df2b0), > + VMEVENT_DEF(TRACE, 0x12d91467), > + VMEVENT_DEF(RECORD, 0x1284bf4f), > + VMEVENT_DEF(TEXIT, 0x129df2b0), > + VMEVENT_DEF(ERRFIN, 0x12d93888), > LJ_VMEVENT__MAX > } VMEvent; AFAICS, Mike removes the high bits of these hashes since they are scratched via the bitwise operation `(hash)<<3` in `VMEVENT_DEF` anyway. Please mention this in the commit message. > > diff --git a/test/PUC-Rio-Lua-5.1-tests/gc.lua b/test/PUC-Rio-Lua-5.1-tests/gc.lua > index 10e1f2dd..05ac0d11 100644 > --- a/test/PUC-Rio-Lua-5.1-tests/gc.lua > +++ b/test/PUC-Rio-Lua-5.1-tests/gc.lua > @@ -260,13 +260,13 @@ u, m = nil > collectgarbage() > assert(m==10) > > - > +-- The test below is disabled for LuaJIT, since it handles errors from > +-- GC finalizers via VM event. > -- errors during collection > -u = newproxy(true) > -getmetatable(u).__gc = function () error "!!!" end > -u = nil > -assert(not pcall(collectgarbage)) > - > +-- u = newproxy(true) > +-- getmetatable(u).__gc = function () error "!!!" end > +-- u = nil > +-- assert(not pcall(collectgarbage)) > Maybe its better just to setup the "errfin" handler to `error()` function? > if not rawget(_G, "_soft") then > print("deep structures") > diff --git a/test/tarantool-tests/lj-946-print-errors-from-gc-fin.test.lua b/test/tarantool-tests/lj-946-print-errors-from-gc-fin.test.lua > new file mode 100644 > index 00000000..cce417d2 > --- /dev/null > +++ b/test/tarantool-tests/lj-946-print-errors-from-gc-fin.test.lua > @@ -0,0 +1,42 @@ > +local tap = require('tap') > +local test = tap.test('lj-946-print-errors-from-gc-fin'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +test:plan(2) > + > +local ffi = require('ffi') > +local error_in_finalizer = false > + > +local function errfin_handler() > + error_in_finalizer = true > +end Is it better just to add `test:ok(true, 'error handler called')` here? > + > +local function new_bad_cdata() > + return ffi.gc(ffi.new('char [?]', 1024), 'uncallable string') > +end > + > +local function test_f() > + collectgarbage('collect') > + -- Make GC aggressive enough to end atomic phase before Typo: s/atomic/the atomic/ > + -- exit from the trace. Typo: s/exit from/exiting/ > + collectgarbage('setstepmul', 400) > + -- Number of iterations is empirical, just big enough for the Typo: s/Number/The number/ > + -- issue to strike. > + for _ = 1, 4000 do > + new_bad_cdata() > + end > +end > + > +jit.opt.start('hotloop=1') > +-- Handler is registered, but never called before the patch, Typo: s/registered,/registered/ > +-- but should be called after the patch. Typo: s/patch, but/patch. It/ > +jit.attach(errfin_handler, 'errfin') > +local status = pcall(test_f) > +-- We have to stop GC now because any step raises the error due to > +-- cursed cdata objects. > +collectgarbage('stop') > +test:ok(status, 'test function completed successfully') > +test:ok(error_in_finalizer, 'error handler called') > + > +test:done(true) I suggest to test the default handler too, within the separate test. Also, maybe we should test other cases (error function (in default case), function with tailcall to error, etc.), any ideas about them? > -- > 2.39.3 (Apple Git-145) > -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH luajit 2/2] Fix last commit. 2023-10-27 12:57 [Tarantool-patches] [PATCH luajit 0/2] gc: handle errors in finalizers Maksim Kokryashkin via Tarantool-patches 2023-10-27 12:57 ` [Tarantool-patches] [PATCH luajit 1/2] Print errors from __gc finalizers instead of rethrowing them Maksim Kokryashkin via Tarantool-patches @ 2023-10-27 12:57 ` Maksim Kokryashkin via Tarantool-patches 2023-11-01 8:56 ` Sergey Kaplun via Tarantool-patches 1 sibling, 1 reply; 7+ messages in thread From: Maksim Kokryashkin via Tarantool-patches @ 2023-10-27 12:57 UTC (permalink / raw) To: tarantool-patches, sergeyb, skaplun, m.kokryashkin From: Mike Pall <mike> Reported by PluMGMK. (cherry-picked from commit 224129a8e64bfa219d35cd03055bf03952f167f6) The error handler for GC finalizers was not cleared from the stack after the initialization. This commit adds stack cleanup. Maxim Kokryashkin: * added the description and the test for the problem Part of tarantool/tarantool#9145 --- src/lib_aux.c | 2 ++ .../fix-finalizer-error-handler-init.test.c | 30 +++++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 test/tarantool-c-tests/fix-finalizer-error-handler-init.test.c diff --git a/src/lib_aux.c b/src/lib_aux.c index d532a12f..ecdc67f9 100644 --- a/src/lib_aux.c +++ b/src/lib_aux.c @@ -352,6 +352,7 @@ LUALIB_API lua_State *luaL_newstate(void) lua_pushcfunction(L, error_finalizer); lua_rawseti(L, -2, VMEVENT_HASH(LJ_VMEVENT_ERRFIN)); G(L)->vmevmask = VMEVENT_MASK(LJ_VMEVENT_ERRFIN); + L->top--; #endif } return L; @@ -378,6 +379,7 @@ LUALIB_API lua_State *luaL_newstate(void) lua_pushcfunction(L, error_finalizer); lua_rawseti(L, -2, VMEVENT_HASH(LJ_VMEVENT_ERRFIN)); G(L)->vmevmask = VMEVENT_MASK(LJ_VMEVENT_ERRFIN); + L->top--; #endif } return L; diff --git a/test/tarantool-c-tests/fix-finalizer-error-handler-init.test.c b/test/tarantool-c-tests/fix-finalizer-error-handler-init.test.c new file mode 100644 index 00000000..44c1d96b --- /dev/null +++ b/test/tarantool-c-tests/fix-finalizer-error-handler-init.test.c @@ -0,0 +1,30 @@ +#include "lua.h" +#include "test.h" +#include "utils.h" + +#include "lj_obj.h" + +/* + * This test demonstrates an uncleared Lua stack after the + * initialization of the error handler for GC finalizers. + */ + +static int stack_is_clean(void *test_state) +{ + lua_State *L = test_state; + assert_ptr_equal(L->top, L->base); + return TEST_EXIT_SUCCESS; +} + +int main(void) +{ + lua_State *L = utils_lua_init(); + + const struct test_unit tgroup[] = { + test_unit_def(stack_is_clean) + }; + + const int test_result = test_run_group(tgroup, L); + utils_lua_close(L); + return test_result; +} -- 2.39.3 (Apple Git-145) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] Fix last commit. 2023-10-27 12:57 ` [Tarantool-patches] [PATCH luajit 2/2] Fix last commit Maksim Kokryashkin via Tarantool-patches @ 2023-11-01 8:56 ` Sergey Kaplun via Tarantool-patches 0 siblings, 0 replies; 7+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2023-11-01 8:56 UTC (permalink / raw) To: Maksim Kokryashkin; +Cc: tarantool-patches Hi, Maxim! Thanks for the patch! LGTM, just a few nits below. On 27.10.23, Maksim Kokryashkin wrote: > From: Mike Pall <mike> > > Reported by PluMGMK. > > (cherry-picked from commit 224129a8e64bfa219d35cd03055bf03952f167f6) > > The error handler for GC finalizers was not cleared from the It's not a handler itself, but "_VMEVENTS" table, where the handler is registered. > stack after the initialization. This commit adds stack cleanup. > > Maxim Kokryashkin: > * added the description and the test for the problem > > Part of tarantool/tarantool#9145 > --- > src/lib_aux.c | 2 ++ > .../fix-finalizer-error-handler-init.test.c | 30 +++++++++++++++++++ > 2 files changed, 32 insertions(+) > create mode 100644 test/tarantool-c-tests/fix-finalizer-error-handler-init.test.c > > diff --git a/src/lib_aux.c b/src/lib_aux.c > index d532a12f..ecdc67f9 100644 > --- a/src/lib_aux.c > +++ b/src/lib_aux.c <snipped> > diff --git a/test/tarantool-c-tests/fix-finalizer-error-handler-init.test.c b/test/tarantool-c-tests/fix-finalizer-error-handler-init.test.c > new file mode 100644 > index 00000000..44c1d96b > --- /dev/null > +++ b/test/tarantool-c-tests/fix-finalizer-error-handler-init.test.c Please, add "lj-991-" prefix and mention the issue in the comment below. > @@ -0,0 +1,30 @@ > +#include "lua.h" > +#include "test.h" > +#include "utils.h" > + > +#include "lj_obj.h" > + > +/* > + * This test demonstrates an uncleared Lua stack after the > + * initialization of the error handler for GC finalizers. > + */ > + > +static int stack_is_clean(void *test_state) > +{ > + lua_State *L = test_state; > + assert_ptr_equal(L->top, L->base); I suppose, that there is no need for internals to be used here. We can just use `lua_gettop(L)` and compare its result to 0. Side note: Also, this helper may be usefull later. See also the comment in [1]. > + return TEST_EXIT_SUCCESS; > +} > + > +int main(void) > +{ > + lua_State *L = utils_lua_init(); > + > + const struct test_unit tgroup[] = { > + test_unit_def(stack_is_clean) > + }; > + > + const int test_result = test_run_group(tgroup, L); > + utils_lua_close(L); > + return test_result; > +} > -- > 2.39.3 (Apple Git-145) > [1]: https://github.com/LuaJIT/LuaJIT/commit/03cd5aa7 -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH luajit 0/2] gc: handle errors in finalizers @ 2023-10-31 16:04 Maksim Kokryashkin via Tarantool-patches 0 siblings, 0 replies; 7+ messages in thread From: Maksim Kokryashkin via Tarantool-patches @ 2023-10-31 16:04 UTC (permalink / raw) To: tarantool-patches, sergeyb, skaplun, m.kokryashkin; +Cc: Maksim Kokryashkin This patchset introduces VM event for finalizer error handling. The first patch contains the VM event implementation, the second is just a minor fixup for the first one. Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-946-print-errors-from-gc-fin PR: https://github.com/tarantool/tarantool/pull/9315 Issue: https://github.com/LuaJIT/LuaJIT/pull/946 Mike Pall (2): Print errors from __gc finalizers instead of rethrowing them. Fix last commit. src/Makefile.dep.original | 14 +++---- src/lib_aux.c | 35 +++++++++++++++- src/lj_gc.c | 10 ++++- src/lj_vmevent.h | 7 ++-- test/PUC-Rio-Lua-5.1-tests/gc.lua | 12 +++--- .../fix-finalizer-error-handler-init.test.c | 30 +++++++++++++ .../lj-946-print-errors-from-gc-fin.test.lua | 42 +++++++++++++++++++ 7 files changed, 130 insertions(+), 20 deletions(-) create mode 100644 test/tarantool-c-tests/fix-finalizer-error-handler-init.test.c create mode 100644 test/tarantool-tests/lj-946-print-errors-from-gc-fin.test.lua -- 2.39.3 (Apple Git-145) ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH luajit 0/2] gc: handle errors in finalizers @ 2023-10-27 12:57 Maksim Kokryashkin via Tarantool-patches 0 siblings, 0 replies; 7+ messages in thread From: Maksim Kokryashkin via Tarantool-patches @ 2023-10-27 12:57 UTC (permalink / raw) To: tarantool-patches, sergeyb, skaplun, m.kokryashkin; +Cc: Maksim Kokryashkin This patchset introduces VM event for finalizer error handling. The first patch contains the VM event implementation, the second is just a minor fixup for the first one. Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-946-print-errors-from-gc-fin PR: https://github.com/tarantool/tarantool/pull/9315 Issue: https://github.com/LuaJIT/LuaJIT/pull/946 Mike Pall (2): Print errors from __gc finalizers instead of rethrowing them. Fix last commit. src/Makefile.dep.original | 14 +++---- src/lib_aux.c | 35 +++++++++++++++- src/lj_gc.c | 10 ++++- src/lj_vmevent.h | 7 ++-- test/PUC-Rio-Lua-5.1-tests/gc.lua | 12 +++--- .../fix-finalizer-error-handler-init.test.c | 30 +++++++++++++ .../lj-946-print-errors-from-gc-fin.test.lua | 42 +++++++++++++++++++ 7 files changed, 130 insertions(+), 20 deletions(-) create mode 100644 test/tarantool-c-tests/fix-finalizer-error-handler-init.test.c create mode 100644 test/tarantool-tests/lj-946-print-errors-from-gc-fin.test.lua -- 2.39.3 (Apple Git-145) ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-11-01 9:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-10-27 12:57 [Tarantool-patches] [PATCH luajit 0/2] gc: handle errors in finalizers Maksim Kokryashkin via Tarantool-patches 2023-10-27 12:57 ` [Tarantool-patches] [PATCH luajit 1/2] Print errors from __gc finalizers instead of rethrowing them Maksim Kokryashkin via Tarantool-patches 2023-11-01 8:15 ` Sergey Kaplun via Tarantool-patches 2023-10-27 12:57 ` [Tarantool-patches] [PATCH luajit 2/2] Fix last commit Maksim Kokryashkin via Tarantool-patches 2023-11-01 8:56 ` Sergey Kaplun via Tarantool-patches -- strict thread matches above, loose matches on Subject: below -- 2023-10-31 16:04 [Tarantool-patches] [PATCH luajit 0/2] gc: handle errors in finalizers Maksim Kokryashkin via Tarantool-patches 2023-10-27 12:57 Maksim Kokryashkin via Tarantool-patches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox