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

[Sergey Kaplun <skaplun@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 <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.

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