Re: [Tarantool-patches] [PATCH luajit 3/3] core: remove excess assertion inside memprof

[Igor Munkin <imun@tarantool.org>]

Sergey,

Thanks for the patch! AFAICS, the issue relates to invalid writer to be
chosen but not to the <lj_debug_frameline> 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