Re: [Tarantool-patches] [PATCH luajit v2 1/2] Print errors from __gc finalizers instead of rethrowing them.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review!
See my answers below.

On Thu, Nov 09, 2023 at 03:03:46PM +0300, Sergey Bronnikov wrote:
> Hi, Max
> 
> 
> Thanks for the patch! LGTM
> 
> See my comments below.
> 
> Sergey
> 
> On 11/8/23 00:06, Maksim Kokryashkin wrote:
> 
> 
> <snipped>
> 
> > 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
> > +  -- issue to strike.
> > +  for _ = 1, 4000 do
> 
> Works fine with a number of iterations equal to 300,
> 
> checked locally by running test 1000 times.
Yep, works fine with smaller number of iterations under LuaJIT,
but the story is different under Tarantool, that's why the number
is so large.
> 
> > +    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:.*')
> 
> testcase description is missed:
Fixed.
> 
> 
> TAP version 13
> 1..1
> ok - nil
> 
> > +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
> > +  -- 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)
> > 
Here is the diff:
===
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
index f0d11f6f..f22b3997 100644
--- 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
@@ -7,5 +7,5 @@ test:plan(1)
 
 local script = require('utils').exec.makecmd(arg, { redirect = '2>&1' })
 local output = script()
-test:like(output, 'ERROR in finalizer:')
+test:like(output, 'ERROR in finalizer:', 'error handler called')
 test:done(true)