From: sergos via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit] Fix write barrier for lua_setupvalue() and debug.setupvalue(). Date: Mon, 6 Jun 2022 18:15:06 +0300 [thread overview] Message-ID: <68FC92D8-C62F-4625-8331-5AB943D9E7FF@tarantool.org> (raw) In-Reply-To: <20211215101734.6065-1-skaplun@tarantool.org> 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 > > Note: CI is red. But this job is red on master too... > > src/lj_api.c | 8 ++- > src/lj_debug.c | 7 ++- > src/lj_debug.h | 3 +- > .../fix-gc-setupvalue.test.lua | 60 +++++++++++++++++++ > test/tarantool-tests/utils.lua | 32 ++++++++++ > 5 files changed, 104 insertions(+), 6 deletions(-) > create mode 100644 test/tarantool-tests/fix-gc-setupvalue.test.lua > > diff --git a/src/lj_api.c b/src/lj_api.c > index c7a0b327..ba38881f 100644 > --- a/src/lj_api.c > +++ b/src/lj_api.c > @@ -943,7 +943,8 @@ LUA_API int lua_next(lua_State *L, int idx) > LUA_API const char *lua_getupvalue(lua_State *L, int idx, int n) > { > TValue *val; > - const char *name = lj_debug_uvnamev(index2adr(L, idx), (uint32_t)(n-1), &val); > + GCobj *o; > + const char *name = lj_debug_uvnamev(index2adr(L, idx), (uint32_t)(n-1), &val, &o); > if (name) { > copyTV(L, L->top, val); > incr_top(L); > @@ -1129,13 +1130,14 @@ LUA_API const char *lua_setupvalue(lua_State *L, int idx, int n) > { > cTValue *f = index2adr(L, idx); > TValue *val; > + GCobj *o; > const char *name; > api_checknelems(L, 1); > - name = lj_debug_uvnamev(f, (uint32_t)(n-1), &val); > + name = lj_debug_uvnamev(f, (uint32_t)(n-1), &val, &o); > if (name) { > L->top--; > copyTV(L, val, L->top); > - lj_gc_barrier(L, funcV(f), L->top); > + lj_gc_barrier(L, o, L->top); > } > return name; > } > diff --git a/src/lj_debug.c b/src/lj_debug.c > index bb9ab288..8eb5983b 100644 > --- a/src/lj_debug.c > +++ b/src/lj_debug.c > @@ -221,19 +221,22 @@ const char *lj_debug_uvname(GCproto *pt, uint32_t idx) > } > > /* Get name and value of upvalue. */ > -const char *lj_debug_uvnamev(cTValue *o, uint32_t idx, TValue **tvp) > +const char *lj_debug_uvnamev(cTValue *o, uint32_t idx, TValue **tvp, GCobj **op) > { > if (tvisfunc(o)) { > GCfunc *fn = funcV(o); > if (isluafunc(fn)) { > GCproto *pt = funcproto(fn); > if (idx < pt->sizeuv) { > - *tvp = uvval(&gcref(fn->l.uvptr[idx])->uv); > + GCobj *uvo = gcref(fn->l.uvptr[idx]); > + *tvp = uvval(&uvo->uv); > + *op = uvo; > return lj_debug_uvname(pt, idx); > } > } else { > if (idx < fn->c.nupvalues) { > *tvp = &fn->c.upvalue[idx]; > + *op = obj2gco(fn); > return ""; > } > } > diff --git a/src/lj_debug.h b/src/lj_debug.h > index a157d284..e037728a 100644 > --- a/src/lj_debug.h > +++ b/src/lj_debug.h > @@ -29,7 +29,8 @@ typedef struct lj_Debug { > LJ_FUNC cTValue *lj_debug_frame(lua_State *L, int level, int *size); > LJ_FUNC BCLine LJ_FASTCALL lj_debug_line(GCproto *pt, BCPos pc); > LJ_FUNC const char *lj_debug_uvname(GCproto *pt, uint32_t idx); > -LJ_FUNC const char *lj_debug_uvnamev(cTValue *o, uint32_t idx, TValue **tvp); > +LJ_FUNC const char *lj_debug_uvnamev(cTValue *o, uint32_t idx, TValue **tvp, > + GCobj **op); > LJ_FUNC const char *lj_debug_slotname(GCproto *pt, const BCIns *pc, > BCReg slot, const char **name); > LJ_FUNC const char *lj_debug_funcname(lua_State *L, cTValue *frame, > 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. > + > +-- Create to functions with closed upvalue. > +do > + local uv = 1 > + local function f_parent() > + local function f() > + return uv + 1 > + end > + _G.f = f > + return uv + 1 > + end > + -- Set up `f()`. > + f_parent() > + _G.f_parent = f_parent > +end > + > +-- Set GC on start. > +collectgarbage() > +-- Set minimally possible stepmul. > +-- 1024/10 * stepmul == 10 < sizeof(GCfuncL), so it guarantees, > +-- that 2 functions will be marked in different time. > +local oldstepmul = collectgarbage('setstepmul', 1) > + > +-- `f_parent()` function is marked before `f()`, so wait until > +-- it becomes black and proceed with the test. > +while not utils.gc_isblack(_G.f_parent) do > + collectgarbage('step') > +end > + > +-- 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. > +-- 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 > @@ -3,11 +3,43 @@ local M = {} > local ffi = require('ffi') > local tap = require('tap') > local bc = require('jit.bc') > +local bit = require('bit') > + > +local GCRef = ffi.abi('gc64') and 'uint64_t' or 'uint32_t' > +local LJ_GC_BLACK = 0x04 > > ffi.cdef([[ > int setenv(const char *name, const char *value, int overwrite); > + typedef struct { > + ]]..GCRef..[[ nextgc; > + uint8_t marked; > + uint8_t gct; > + /* Need this fields for correct alignment and sizeof. */ > + uint8_t misc1; > + uint8_t misc2; > + } GCHeader; > ]]) > > +function M.gc_isblack(obj) > + local objtype = type(obj) > + assert(objtype ~= 'number' and objtype ~= 'boolean', > + 'can proceed only with GC objects') > + local address > + if objtype == 'string' then > + -- XXX: get strdata first and go back to GCHeader. > + address = ffi.cast('char *', obj) > + address = address - (ffi.sizeof('GCHeader') + 8) > + else > + -- XXX: FFI ABI forbids to cast functions objects > + -- to non-functional pointers, but we can get their address > + -- via tostring. > + local str_address = tostring(obj):gsub(objtype .. ': ', '') > + address = tonumber(str_address) > + end > + local marked = ffi.cast('GCHeader *', address).marked > + return bit.band(marked, LJ_GC_BLACK) == LJ_GC_BLACK > +end > + > local function luacmd(args) > -- arg[-1] is guaranteed to be not nil. > local idx = -2 > -- > 2.34.1 >
next prev parent reply other threads:[~2022-06-06 15:15 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-12-15 10:17 Sergey Kaplun via Tarantool-patches 2022-06-06 15:15 ` sergos via Tarantool-patches [this message] 2022-06-10 8:25 ` Sergey Kaplun via Tarantool-patches 2022-06-28 15:28 ` Igor Munkin via Tarantool-patches 2022-06-30 12:10 ` Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=68FC92D8-C62F-4625-8331-5AB943D9E7FF@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=sergos@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit] Fix write barrier for lua_setupvalue() and debug.setupvalue().' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox