[Tarantool-patches] [PATCH luajit] Fix frame traversal for __gc handler frames.
imun at tarantool.org
Tue Nov 23 15:57:21 MSK 2021
Sergey has already sent v2, but I decided to leave a few notes for the
On 08.11.21, Sergey Kaplun wrote:
> Thanks for the review!
> On 02.11.21, Igor Munkin wrote:
> > > > > 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  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
In v2 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).
> > > > > +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.
> 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.
> Best regards,
> Sergey Kaplun
More information about the Tarantool-patches