From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 2EC616ECE6; Mon, 6 Jun 2022 18:15:11 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2EC616ECE6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1654528511; bh=CYEhNjcKjNh1ZOkG+arfD9YaHpDtJoMYTms9wWQwZts=; h=In-Reply-To:Date:References:To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=WoKoON7YgyUb5yl2C1ArtkyVUohVvmTpHdqCHgVcv6EjiJfGTUBAEdnLdgiPi4XgT lKIy6s9f1GeTTplo4WIATMrGZHTQvnypbTZ7vOQRRx2ily8HMK7QTlk3kziF3uv4AH g3GYg4AXo6FaVSgPNo35kDIZJlO0qkrRj+wPULP8= Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id AB9BC6ECE6 for ; Mon, 6 Jun 2022 18:15:07 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org AB9BC6ECE6 Received: by smtp44.i.mail.ru with esmtpa (envelope-from ) id 1nyERe-00071m-Rx; Mon, 06 Jun 2022 18:15:07 +0300 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.80.82.1.1\)) In-Reply-To: <20211215101734.6065-1-skaplun@tarantool.org> Date: Mon, 6 Jun 2022 18:15:06 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <68FC92D8-C62F-4625-8331-5AB943D9E7FF@tarantool.org> References: <20211215101734.6065-1-skaplun@tarantool.org> To: Sergey Kaplun X-Mailer: Apple Mail (2.3696.80.82.1.1) X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9445BFF90255AB1296AFFC5C214CAEDBBE2FF917483013089182A05F538085040B358FEEC2211A833C3C73772F42AD1257D0BA1A495094815813A216511240D9C X-8FC586DF: 6EFBBC1D9D64D975 X-C1DE0DAB: 0D63561A33F958A5887E41F0A07C4D12CDE82E425BBC480813CD0D22F69C3D78D59269BC5F550898D99A6476B3ADF6B4886A5961035A09600383DAD389E261318FB05168BE4CE3AF X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3463DBE2ADA183F62F1BCBC7DCA5DA26DA0B9E7BAD50644BCDC08D0E436032E22FE858DCEA206062881D7E09C32AA3244C0590D020B9320E7A7919DEE7EB9294F4F26BFA4C8A6946B8FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojw5CbW4eaNzLufmg5/lRkCw== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE40755B558D799293731D651962AAA9C62FEC5AC0816EEA6E42A19381EE24192DF5555834048F03EF5D4C9A814A92B2E3B1BA4250FC3964EA4964198E0F3ECE9B5443453F38A29522196 X-Mras: OK Subject: Re: [Tarantool-patches] [PATCH luajit] Fix write barrier for lua_setupvalue() and debug.setupvalue(). X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: sergos via Tarantool-patches Reply-To: sergos Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the patch! LGTM with minor updates to the description. Sergos > On 15 Dec 2021, at 13:17, Sergey Kaplun wrote: >=20 > From: Mike Pall >=20 > (cherry picked from e613105ca92fe25e7bd63031b409faa8c908ac35) >=20 > Child function inherits parents upvalues. Assume parent function is parents=E2=80=99 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. >=20 > This patch changes a function object to an upvalue for barrier = movement. >=20 > Sergey Kaplun: > * added the description and the test for the problem >=20 > 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-setupv= alue-full-ci > Tarantool branch: = https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-fix-gc-set= upvalue-full-ci >=20 > Note: CI is red. But this job is red on master too... >=20 > 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 >=20 > 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 =3D lj_debug_uvnamev(index2adr(L, idx), = (uint32_t)(n-1), &val); > + GCobj *o; > + const char *name =3D 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 =3D index2adr(L, idx); > TValue *val; > + GCobj *o; > const char *name; > api_checknelems(L, 1); > - name =3D lj_debug_uvnamev(f, (uint32_t)(n-1), &val); > + name =3D 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) > } >=20 > /* 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 =3D funcV(o); > if (isluafunc(fn)) { > GCproto *pt =3D funcproto(fn); > if (idx < pt->sizeuv) { > - *tvp =3D uvval(&gcref(fn->l.uvptr[idx])->uv); > + GCobj *uvo =3D gcref(fn->l.uvptr[idx]); > + *tvp =3D uvval(&uvo->uv); > + *op =3D uvo; > return lj_debug_uvname(pt, idx); > } > } else { > if (idx < fn->c.nupvalues) { > *tvp =3D &fn->c.upvalue[idx]; > + *op =3D 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 =3D require('tap') > +local utils =3D require('utils') > + > +local test =3D 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 =3D 1 > + local function f_parent() > + local function f() > + return uv + 1 > + end > + _G.f =3D f > + return uv + 1 > + end > + -- Set up `f()`. > + f_parent() > + _G.f_parent =3D f_parent > +end > + > +-- Set GC on start. > +collectgarbage() > +-- Set minimally possible stepmul. > +-- 1024/10 * stepmul =3D=3D 10 < sizeof(GCfuncL), so it guarantees, > +-- that 2 functions will be marked in different time. > +local oldstepmul =3D 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 =3D 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=E2=80=99m not missing the point, the below will never be executed = since=20 invariant is violated.=20 > +-- Generate some garbage to reuse freed memory. > +for i =3D 1, 1e2 do local _ =3D {string.rep('0', i)} end > + > +test:ok(_G.f_parent() =3D=3D 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 =3D {} > local ffi =3D require('ffi') > local tap =3D require('tap') > local bc =3D require('jit.bc') > +local bit =3D require('bit') > + > +local GCRef =3D ffi.abi('gc64') and 'uint64_t' or 'uint32_t' > +local LJ_GC_BLACK =3D 0x04 >=20 > 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; > ]]) >=20 > +function M.gc_isblack(obj) > + local objtype =3D type(obj) > + assert(objtype ~=3D 'number' and objtype ~=3D 'boolean', > + 'can proceed only with GC objects') > + local address > + if objtype =3D=3D 'string' then > + -- XXX: get strdata first and go back to GCHeader. > + address =3D ffi.cast('char *', obj) > + address =3D 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 =3D tostring(obj):gsub(objtype .. ': ', '') > + address =3D tonumber(str_address) > + end > + local marked =3D ffi.cast('GCHeader *', address).marked > + return bit.band(marked, LJ_GC_BLACK) =3D=3D LJ_GC_BLACK > +end > + > local function luacmd(args) > -- arg[-1] is guaranteed to be not nil. > local idx =3D -2 > --=20 > 2.34.1 >=20