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

[Sergey Kaplun <skaplun@tarantool.org>]

Hi!

Thanks for the review!

On 30.12.20, Sergey Ostanevich wrote:
> 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'

Thanks! Fixed in the commit message and the comment. Branch is
force-pushed. See the iterative diff below.

> 		
> > | 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
> 

===================================================================
diff --git a/src/lj_memprof.c b/src/lj_memprof.c
index 37ec4c9..2c1ef3b 100644
--- a/src/lj_memprof.c
+++ b/src/lj_memprof.c
@@ -94,7 +94,7 @@ 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.
-  ** There are the cases when the memory profiler attempts
+  ** There are 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).
===================================================================

-- 
Best regards,
Sergey Kaplun