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

[Sergey Ostanevich <sergos@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]

Hi!

Thanks for the patch!
Just one nit in comments.

LGTM.
Sergos

> 
> 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
	      ^^^ remove, just 'there are cases'
		
> | 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


[-- Attachment #2: Type: text/html, Size: 29367 bytes --]