From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 A44794765E0 for ; Wed, 30 Dec 2020 12:51:23 +0300 (MSK) Date: Wed, 30 Dec 2020 12:50:31 +0300 From: Sergey Kaplun Message-ID: <20201230095031.GR14702@root> References: <20201230093916.GX5396@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201230093916.GX5396@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH luajit 3/3] core: remove excess assertion inside memprof List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Igor, Thanks for the review! On 30.12.20, Igor Munkin wrote: > Sergey, > > Thanks for the patch! AFAICS, the issue relates to invalid writer to be > chosen but not to the return value. As we discussed > offline, all these allocations should be reported as internal, but now > the engine attempts to attribute them with a Lua function being > recorded. Please mention this fact in the commit message. > > I believe there is an issue with the VM states introduces in the > previous series and it should be fixed. For now I'm OK with this fix > though it is not quite relevant. Anyway, the world is better with it, > crashes are gone, so formally LGTM with two nits below. > > Please create a ticket for this to investigate the root cause. Sure. > > On 30.12.20, Sergey Kaplun wrote: > > lj_debug_frameline() may return -1 for Lua functions on top. > > In this case assertion inside memprof_write_lfunc() that returned line > > is not negative is incorrect. > > I propose the following wording: > | There are the cases when memory profiler attempts to attribute > | allocations triggered by JIT engine recording phase with a Lua > | function to be recorded. At this case lj_debug_frameline() may return > | BC_NOPOS (i.e. a negative value) so the assertion in the Lua writer > | memprof_write_lfunc() is violated. > > > > > This patch removes this assertion. For negative returned line value > > profiler is reported zero frameline. > > > > Follows up tarantool/tarantool#5442 > > --- I've reworded commit message to the following, as you purposed: > > > > I have not thought of any more correct check than: > > | -- Check alocations for Lua function on top. > > | -- This is not the best test case but the most simple. > > | -- Trace allocation on top leads to assertion. > > | jit.on() > > | for _ = 1, 100 do > > | local _ = tostring(_) > > | end > > | jit.off() > > | jit.flush() > > > > But this is not very fair: here we have tail call to cpcall() > > (with trace_state() as an argument) without creating a new guest stack > > frame. Also test looks redundant -- assertion obviously can't fail as > > far as it doesn't exist. But I can add this test case if you want. > > > > src/lj_memprof.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/src/lj_memprof.c b/src/lj_memprof.c > > index c4d2645..0568049 100644 > > --- a/src/lj_memprof.c > > +++ b/src/lj_memprof.c > > @@ -90,15 +90,15 @@ static void memprof_write_lfunc(struct lj_wbuf *out, uint8_t aevent, > > cTValue *nextframe) > > { > > const BCLine line = lj_debug_frameline(L, fn, nextframe); > > + lj_wbuf_addbyte(out, aevent | ASOURCE_LFUNC); > > + lj_wbuf_addu64(out, (uintptr_t)funcproto(fn)); > > /* > > - ** Line is always >= 0 if we are inside a Lua function. > > + ** Line is >= 0 if we are inside a Lua function. > > + ** An exception may be when the Lua function is on top. > > ** Equals to zero when LuaJIT is built with the > > Minor: Please make this wording clearer considering my proposal for > commit message. It doesn't refer to the original problem for now. Reworded. See the iterative patch below. Branch is force-pushed. | core: remove excess assertion inside memprof | | There are the cases when the memory profiler attempts to attribute | allocations triggered by JIT engine recording phase with a Lua function | to be recorded. At this case lj_debug_frameline() may return BC_NOPOS | (i.e. a negative value) so the assertion in the Lua writer | memprof_write_lfunc() is violated. | | This patch removes this assertion. For negative returned line value | profiler is reported zero frameline. | | Follows up tarantool/tarantool#5442 > > > ** -DLUAJIT_DISABLE_DEBUGINFO flag. > > */ > > - lua_assert(line >= 0); > > - lj_wbuf_addbyte(out, aevent | ASOURCE_LFUNC); > > - lj_wbuf_addu64(out, (uintptr_t)funcproto(fn)); > > - lj_wbuf_addu64(out, (uint64_t)line); > > + lj_wbuf_addu64(out, line >= 0 ? (uint64_t)line : 0); > > } > > > > static void memprof_write_cfunc(struct lj_wbuf *out, uint8_t aevent, > > -- > > 2.28.0 > > > > -- > Best regards, > IM =================================================================== diff --git a/src/lj_memprof.c b/src/lj_memprof.c index 0568049..37ec4c9 100644 --- a/src/lj_memprof.c +++ b/src/lj_memprof.c @@ -94,7 +94,10 @@ static void memprof_write_lfunc(struct lj_wbuf *out, uint8_t aevent, lj_wbuf_addu64(out, (uintptr_t)funcproto(fn)); /* ** Line is >= 0 if we are inside a Lua function. - ** An exception may be when the Lua function is on top. + ** There are the cases when the memory profiler attempts + ** to attribute allocations triggered by JIT engine recording + ** phase with a Lua function to be recorded. At this case + ** lj_debug_frameline() may return BC_NOPOS (i.e. a negative value). ** Equals to zero when LuaJIT is built with the ** -DLUAJIT_DISABLE_DEBUGINFO flag. */ =================================================================== -- Best regards, Sergey Kaplun