From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 7299F504BEF; Wed, 8 Nov 2023 11:25:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7299F504BEF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1699431947; bh=R6trTnmkPLzH62nEb1X7YliQuVxGahvzldXg448eYq0=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=mql9/fo2qWRmeyr5uGWfglNSPWiDRi+IeVFnJuy58lq7QBKOfxUqp4XiUmLFjfFkG wPTF0lkpxb2Z4RzGz6mMCn2CKxWac25sbQu2EA9rkLoU0+/RmmLSQVHQWrwdtTD9QN HHcUcZd7O1a2/x7xUflFjRnggDBNfFQ5iXAeHiq4= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 3534C1B7EEF for ; Wed, 8 Nov 2023 11:25:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3534C1B7EEF Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1r0dsf-0001mP-6z; Wed, 08 Nov 2023 11:25:45 +0300 Date: Wed, 8 Nov 2023 11:21:12 +0300 To: Maksim Kokryashkin Message-ID: References: <20231107210616.53138-1-max.kokryashkin@gmail.com> <20231107210616.53138-2-max.kokryashkin@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231107210616.53138-2-max.kokryashkin@gmail.com> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9CDA3EE4AC542069D395AC74FA62DEBE1A85E743BFEE5863A182A05F5380850404C228DA9ACA6FE274128F7E6991C0855B18592DEB64FF927A97A99597602F0584598982F3545300E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE76D34FAA3D8B31588C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE79173C6E970493712EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B73AB1701401CD871D9D093815EDCFEA82FF837EAF200E82ACC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8D2DCF9CF1F528DBCF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE33AC447995A7AD189C87C2358C8A9BAF3A03B725D353964B2FFDA4F57982C5F435872C767BF85DA227C277FBC8AE2E8BF8ED170A0960FA5675ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A5CD951574C180FB312F889611DF71155B51DD2ACCA27A8390F87CCE6106E1FC07E67D4AC08A07B9B0A6C7FFFE744CA7FBCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF4341A228756219CEFFD34AB2EA02C1E3E0FB2D767C0DB3597272565CCAB6516F77ACD0092D6ACCC82B33B16BC67043E02615CC52934F7EAEC4FCCE671AB878C6A74DFFEFA5DC0E7F02C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojGx6zOZpnTNDqRHEsySUHwg== X-DA7885C5: 8434F9E132CB40A24B2131F92673A4EDA129A01EC2BA3A9A64F1305276F04E15262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE33AC3D9F893128EB88C77097C79DBF3A350FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v2 1/2] Print errors from __gc finalizers instead of rethrowing them. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maksim! Thanks for the fixes! LGTM, just some minor nits below. On 08.11.23, Maksim Kokryashkin wrote: > From: Mike Pall > > 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 default handler for finalizer errors is set during the > Lua initialization. Namely, in the `luaL_newstate`. > > Along with the introduction of the new `ERRFIN` VM event, the high > bits for the old VM events are removed since they are scratched > anyway by the bitwise operation `(hash)<<3` in the `VMEVENT_DEF` > macro. > > This patch results in a regression in the PUC-Rio test suite. The > test in the suite for the error in the GC finalizer fails after > the patch because the error is now handled with the VM event > handler instead of being rethrown. Hence, the `collectgarbage` > finishes successfully despite the error in the GC finalizer. > Considering this change, the test was disabled. > > There is also another regression in the `misclib-getmetrics-capi`, > because there are a few test cases reliant on the `lua_gettop(L)` > value, which is broken after this patch. The `_VMEVENTS` table, > where the error handler for GC finalizers is set, was not cleared > from the stack after the initialization. This issue is fixed in > the following patch. > > Maxim Kokryashkin: > * added the test for the problem and the description for the patch > > Part of tarantool/tarantool#9145 > --- > Q: > >> +-- 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? > > A: Well, that is not going to fix the test case anyway. See, the > `lj_vmevent_call` is a protected call and any errros which happened > during the vmevent handling are silently printed into the stderr. So > the collectgarbage call here won't fail even in this case. OK, lets left it as is. > > Q: > >> +local function errfin_handler() > >> + error_in_finalizer = true > >> +end > > > >Is it better just to add `test:ok(true, 'error handler called')` here? > > A: Nope, because I want to test not only that there is no error, but > also that the finalizer error handler was called. So, the test will be passed, if the `errfin_handler()` is called. If it isn't, we got bad plan error, so test fails. As a bonus, we check that handler is called only once for each error :). > > > Q: 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? > > A: I've added a test for the default handler. I doubt that testing > for other cases you have mentioned is meaningful, because all of > these errors are going to be silently printed into the stderr. OK, thanks! > > 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 +++--- > ...6-print-errors-from-gc-fin-custom.test.lua | 42 +++++++++++++++++++ > ...-print-errors-from-gc-fin-default.test.lua | 11 +++++ > .../script.lua | 24 +++++++++++ > 8 files changed, 133 insertions(+), 20 deletions(-) > create mode 100644 test/tarantool-tests/lj-946-print-errors-from-gc-fin-custom.test.lua > create mode 100644 test/tarantool-tests/lj-946-print-errors-from-gc-fin-default.test.lua > create mode 100644 test/tarantool-tests/lj-946-print-errors-from-gc-fin-default/script.lua > > diff --git a/test/tarantool-tests/lj-946-print-errors-from-gc-fin-custom.test.lua b/test/tarantool-tests/lj-946-print-errors-from-gc-fin-custom.test.lua > new file mode 100644 > index 00000000..71efc260 > --- /dev/null > +++ b/test/tarantool-tests/lj-946-print-errors-from-gc-fin-custom.test.lua > @@ -0,0 +1,42 @@ > +local tap = require('tap') > +local test = tap.test('lj-946-print-errors-from-gc-fin-custom'):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 the atomic phase before > + -- exiting the trace. > + collectgarbage('setstepmul', 400) > + -- The number of iterations is empirical, just big enough for the Nit: comment line length is more than 66 symbols. > + -- 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. > +-- It 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) > diff --git a/test/tarantool-tests/lj-946-print-errors-from-gc-fin-default.test.lua b/test/tarantool-tests/lj-946-print-errors-from-gc-fin-default.test.lua > new file mode 100644 > index 00000000..dfef11e5 > --- /dev/null > +++ b/test/tarantool-tests/lj-946-print-errors-from-gc-fin-default.test.lua > @@ -0,0 +1,11 @@ > +local tap = require('tap') > +local test = tap.test('lj-flush-on-trace'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +test:plan(1) > + > +local script = require('utils').exec.makecmd(arg, { redirect = '2>&1' }) > +local output = script() > +test:like(output, '.*ERROR in finalizer:.*') Minor: '.*' aren't necessary here. I suppose, that regex 'ERROR in finalizer:' is much readable and has the same meaning. > +test:done(true) > diff --git a/test/tarantool-tests/lj-946-print-errors-from-gc-fin-default/script.lua b/test/tarantool-tests/lj-946-print-errors-from-gc-fin-default/script.lua > new file mode 100644 > index 00000000..fdd9ced1 > --- /dev/null > +++ b/test/tarantool-tests/lj-946-print-errors-from-gc-fin-default/script.lua > @@ -0,0 +1,24 @@ > +local ffi = require('ffi') > + > +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 the atomic phase before > + -- exiting the trace. > + collectgarbage('setstepmul', 400) > + -- The number of iterations is empirical, just big enough for the Nit: comment line length is more than 66 symbols. > + -- issue to strike. > + for _ = 1, 4000 do > + new_bad_cdata() > + end > +end > + > +jit.opt.start('hotloop=1') > +local status = pcall(test_f) > +-- We have to stop GC now because any step raises the error due to > +-- cursed cdata objects. > +collectgarbage('stop') > +assert(status, 'error is not rethrown') > -- > 2.39.3 (Apple Git-145) > -- Best regards, Sergey Kaplun