Re: [Tarantool-patches] [PATCH luajit] Fix write barrier for lua_setupvalue() and debug.setupvalue().

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergos!

Thanks for the review.

I updated commit message considerring your comments.
The branch is force-pushed.

| Fix write barrier for lua_setupvalue() and debug.setupvalue().
|
| (cherry picked from e613105ca92fe25e7bd63031b409faa8c908ac35)
|
| At the first GC state (`GCSpause`) all objects should be marked with the
| current white (`LJ_GC_WHITE0` or `LJ_GC_WHITE1`). The main lua_State,
| globals table, registry, and metatables are marked gray and added to the
| gray list. The state now changes to `GCSpropagate`.
|
| At `GCSpropagate` each object in the gray list is removed and marked
| black (`LJ_GC_BLACK`), then any white (type 0 or 1) objects it references are marked
| gray and added to the gray list. Once the gray list is empty the current
| white is switched to the other white. All objects marked with the old
| white type are now dead objects.
|
| Child function inherits parents' upvalues. Assume parent function is
| marked first (all closed upvalues and function are colored to black),
| and then `debug.setupvalue()`/`lua_setupvalue()` is called for an
| unmarked child function with inherited upvalues. The barrier is tried to
| move forward by marking object gray (but not actually move, due to the
| colors of operands) for a non-marked function (instead marked upvalue).
| Now black upvalue refers to a white object. Black objects can't refer
| white objects due to GC invariant, so the invariant is violated.
|
| This patch changes a function object to an upvalue for barrier movement.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| Part of tarantool/tarantool#6548

On 06.06.22, sergos wrote:
> Hi!
> 
> Thanks for the patch!
> 
> LGTM with minor updates to the description.
> 
> Sergos
> 
> > On 15 Dec 2021, at 13:17, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > From: Mike Pall <mike>
> > 
> > (cherry picked from e613105ca92fe25e7bd63031b409faa8c908ac35)
> > 
> > Child function inherits parents upvalues. Assume parent function is
> 
>                          parents’ upvalues.
> 
> > marked first (all closed upvalues and function are colored to black),
> 
> I believe coloring should be described, or at least GC mentioned
> 
> > and then `debug.setupvalue()`/`lua_setupvalue()` is called for an
> > unmarked child function with inherited upvalues. The barrier is tried to
> 
> Not quite clear which way it tries to move - mark the upvalue white?
> 
> > move forward (but not actually move, due to the colors of operands) for
> > a non-marked function (instead marked upvalue). Now black upvalue refers
> > to a white object. Black objects can't refer white objects due to GC
> > invariant, so the invariant is violated.
> > 
> > This patch changes a function object to an upvalue for barrier movement.
> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Part of tarantool/tarantool#6548
> > ---
> > Related issue: https://github.com/tarantool/tarantool/issues/6548
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-fix-gc-setupvalue-full-ci
> > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-fix-gc-setupvalue-full-ci
> > 

<snipped>

> > diff --git a/test/tarantool-tests/fix-gc-setupvalue.test.lua b/test/tarantool-tests/fix-gc-setupvalue.test.lua
> > new file mode 100644
> > index 00000000..8d83ee6e
> > --- /dev/null
> > +++ b/test/tarantool-tests/fix-gc-setupvalue.test.lua
> > @@ -0,0 +1,60 @@
> > +local tap = require('tap')
> > +local utils = require('utils')
> > +
> > +local test = tap.test('fix-gc-setupvalue')
> > +test:plan(1)
> > +
> > +-- Test file to demonstrate LuaJIT GC invariant violation
> > +-- for inherited upvalues.
> > +
> > +-- The bug is about the situation, when black upvalue refers to
> > +-- a white object. This happens due to parent function is marked
> > +-- first (all closed upvalues and function are colored to black),
> > +-- and then `debug.setupvalue()` is called for a child function
> > +-- with inherited upvalues. The barrier is move forward for a
> > +-- non-marked function (instead upvalue) and invariant is
> > +-- violated.
> > +

<snipped>

> > +-- Set created string (white) for the upvalue.
> > +debug.setupvalue(_G.f, 1, '4'..'1')
> > +_G.f = nil
> > +
> > +-- Lets finish it faster.
> > +collectgarbage('setstepmul', oldstepmul)
> > +-- Finish GC cycle to be sure that the object is collected.
> > +while not collectgarbage('step') do end
> > +
> 
> If I’m not missing the point, the below will never be executed since 
> invariant is violated. 

GC invariant is violated, but there is no defined behaviour in this case
(the object may be remarked later and assertion isn't failed). So the
simplest way is to create some other objects to reuse memory pointed by
dead object.

> 
> > +-- Generate some garbage to reuse freed memory.
> > +for i = 1, 1e2 do local _ = {string.rep('0', i)} end
> > +
> > +test:ok(_G.f_parent() == 42, 'correct set up of upvalue')
> > +
> > +os.exit(test:check() and 0 or 1)
> > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> > index 5bd42b30..68781f28 100644
> > --- a/test/tarantool-tests/utils.lua
> > +++ b/test/tarantool-tests/utils.lua

<snipped>

> > -- 
> > 2.34.1
> > 
> 

-- 
Best regards,
Sergey Kaplun