Re: [Tarantool-patches] [PATCH luajit] Fix frame traversal for __gc handler frames.

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey has already sent v2[1], but I decided to leave a few notes for the
history here.

On 08.11.21, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the review!
> 
> On 02.11.21, Igor Munkin wrote:

<snipped>

> > > > > diff --git a/test/tarantool-tests/lj-601-fix-gc-finderrfunc.test.lua b/test/tarantool-tests/lj-601-fix-gc-finderrfunc.test.lua
> > > > > new file mode 100644
> > > > > index 00000000..d8d79100
> > > > > --- /dev/null
> > > > > +++ b/test/tarantool-tests/lj-601-fix-gc-finderrfunc.test.lua
> > > > 
> > > > Unfortunately the test passes on the ’tarantool’ branch 
> > > > 
> > > > s-ostanevich:luajit s.ostanevich$ git checkout tarantool
> > > > Switched to branch 'tarantool'
> > > > s-ostanevich:luajit s.ostanevich$ git clean -xdff
> > > > […]
> > > > s-ostanevich:luajit s.ostanevich$ cmake .
> > > > […]
> > > > s-ostanevich:luajit s.ostanevich$ make
> > > > […]
> > > > [100%] Built target libluajit_shared
> > > > [100%] Built target libluajit
> > > > [100%] Built target luajit
> > > > s-ostanevich:luajit s.ostanevich$ git checkout   skaplun/lj-601-fix-gc-finderrfunc
> > > > s-ostanevich:luajit s.ostanevich$ cd test/tarantool-tests
> > > > s-ostanevich:tarantool-tests s.ostanevich$ ../../src/luajit lj-601-fix-gc-finderrfunc.test.lua 
> > > > TAP version 13
> > > > 1..1
> > > > ok - successfully collectgarbage with error
> > > 
> > > Wild guess: it doesn't fail on Mac due to GC64 ;).
> > > See CI [1] to check my hypothesis.
> > 
> > Is it possible to fix the test chunk making it check the error even with
> > GC64 enabled?
> 
> Yes, may be, but it is hard to forecast its behaviour:
> We need to generate some garbage on the C stack to be used as an errfunc
> value from the unprotected C frame suggested as C protected frame (since
> `cframe_prev()` unwinding is missing for C protected frame (*).
> 
> (*) I.e. C frames are the following CP|C|CP and during handling the second
> CP frame in finderrfunc we return the errfunc for the unprotected C
> frame.

In v2[1] Sergey provided a test where the desired stack layout is made
via Lua C API and garbage is generated with the area allocated on the
host stack. As a result the test also unconditionally fails with GC64
mode enabled (when the patch is not applied, obviously).

> 

<snipped>

> > > > > +getmetatable(a).__gc = function()
> > > > > +  -- Function to raise error via `lj_err_run()` inside __gc.
> > 
> > What does exactly raise an error in this function?
> 
> `collectgarbage()` returns number and attemt to call this number raises
> an error.

PEBKAC, I totally forgot what <load> function does :)

> 
> > 
> > > > > +  local _ = load(function() collectgarbage()() end)
> > > > > +end
> > > > > +
> > > > > +-- XXX: Generate a small bunch of proxies. Need several to call
> > > > > +-- `collectgarbage()` on another proxy inside __gc. N cycles is
> > > > > +-- empirical number.
> > 
> > Why do you even need this loop? Why can't you just assign nil to <a>?
> 
> We need to create a chain with C|CP frames to use errfunction value
> (that is garbage) from C frame as it was from C protected frame.
> Add the corresponding comment.

Again, this chain is created with no loop at all but via Lua C in v2[1].

> 

<snipped>

> 
> Side note: 4 cycles is not enough sometimes (test is flaky) -- change
> them to 6. See no false positives oks after the changes without fix.

AFAICS this value can vary from 4 to 10000 depending on the number of
garbage to be collected. This was the main reason to reimplement the
test, since it was more a reproducer rather than a good test.

> 

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

[1]: https://lists.tarantool.org/tarantool-patches/20211119164157.18344-1-skaplun@tarantool.org/

-- 
Best regards,
IM