From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 167FA4765E0 for ; Wed, 30 Dec 2020 12:39:23 +0300 (MSK) Date: Wed, 30 Dec 2020 12:39:16 +0300 From: Igor Munkin Message-ID: <20201230093916.GX5396@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org 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. 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 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. > ** -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