Re: [Tarantool-patches] [PATCH luajit] memprof: report stack resizing as internal event

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

On 26.03.21, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the review!
> 
> On 25.03.21, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for the patch! Please consider my comments below.
> > 

<snipped>

> 
> See the new commit message below, branch is force-pushed:
> 
> ===================================================================
> memprof: report stack resizing as internal event
> 
> Resizing of the Lua stack is not reported as internal allocation
> as it should. As a result, it may lead to crash inside Lua or FF
> frames.
> 
> When the memory profiler runs, reallocation occurs first, and after
> profiler reports the corresponding event. When the stack is resized for
> local function arguments, the link to previous the frame is invalid in

Typo: s/to previous the frame/to the previous frame/.

> the case of reallocation. Therefore, the assertion in `debug_framepc()`
> fails.
> 
> Resolves tarantool/tarantool#5842
> Follows up tarantool/tarantool#5442
> ===================================================================
> 
> > > 
> > > Resolves tarantool/tarantool#5842
> > > Follows up tarantool/tarantool#5442
> > > ---
> > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5842-memprof-core-on-resizestack
> > > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5842-memprof-core-on-resizestack
> > > Issue: https://github.com/tarantool/tarantool/issues/5842
> > > 
> > > 
> > >  src/lj_state.c                                 |  6 ++++++
> > >  .../misclib-memprof-lapi.test.lua              | 18 ++++++++++++++++++
> > >  2 files changed, 24 insertions(+)
> > > 
> > > diff --git a/src/lj_state.c b/src/lj_state.c
> > > index 1ed79a5..ea9abd4 100644
> > > --- a/src/lj_state.c
> > > +++ b/src/lj_state.c
> > > @@ -64,7 +64,11 @@ static void resizestack(lua_State *L, MSize n)
> > >    MSize oldsize = L->stacksize;
> > >    MSize realsize = n + 1 + LJ_STACK_EXTRA;
> > >    GCobj *up;
> > > +  int32_t old_vmstate = G(L)->vmstate;
> > 
> > Please consider the naming and the workflow in lj_gc.c for such
> > situations: G(L) is stored into a separate variable and <old_vmstate> is
> > named <ostate>. It makes grep for such spots much easier, doesn't it?
> 
> You can see more by grepping vmstate. Made naming more consistent

I did it before sending the review. I did it again now. My opinion is
not changed. This is "idiomatic" approach to "push" and "pop" vmstate
used only in lj_gc.c since this is not required elsewhere. If you move
that approach "intact", it shows that the semantics of your code are the
same. Otherwise, every other occurrence of such vmstate "pushing" and
"popping" allows to introduce own naming: pstate, prevstate,
prev_vmstate, sstate, save_state, savevmstate -- there are lots of
combinations. And nobody can stop contributor from this, since it is
"more consistent for the current TU".

"Feel free to prove the opposite"(c) :)

The current naming is much better than the previous one, but I still
propose to save the original on and save G(L) into a new variable.

> with the <lj_state.c>. s/old_vmstate/oldvmstate/g
> 
> See the iterative patch below.
> 
> > 
> > > +
> > >    lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1);
> > > +
> > > +  setvmstate(G(L), INTERP);
> > 
> > We didn't notice this before. Now you leave not a single word regarding
> > this hack. How come?
> 
> Added the comment. But I don't get how it is connected to our notice.
> May be it should be mentioned in docs?

I meant that we pushed this bug into the trunk and didn't notice it.
Comment is totally enough, thanks!

> 
> See the iterative patch below. Branch is force-pushed.
> ===================================================================
> diff --git a/src/lj_state.c b/src/lj_state.c
> index ea9abd4..c86e098 100644
> --- a/src/lj_state.c
> +++ b/src/lj_state.c
> @@ -64,10 +64,15 @@ static void resizestack(lua_State *L, MSize n)
>    MSize oldsize = L->stacksize;
>    MSize realsize = n + 1 + LJ_STACK_EXTRA;
>    GCobj *up;
> -  int32_t old_vmstate = G(L)->vmstate;
> +  int32_t oldvmstate = G(L)->vmstate;
>  
>    lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1);
>  
> +  /*
> +  ** Lua stack is inconsistent durent reallocation, profilers

Typo: s/profilers/profiler/ or s/depends/depend/.

> +  ** depends on vmstate during reports, so set vmstate to INTERP
> +  ** to avoid inconsistent behaviour.
> +  */
>    setvmstate(G(L), INTERP);
>    st = (TValue *)lj_mem_realloc(L, tvref(L->stack),
>                                  (MSize)(oldsize*sizeof(TValue)),
> @@ -85,7 +90,7 @@ static void resizestack(lua_State *L, MSize n)
>    for (up = gcref(L->openupval); up != NULL; up = gcnext(up))
>      setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta));
>  
> -  G(L)->vmstate = old_vmstate;
> +  G(L)->vmstate = oldvmstate;
>  }
>  
>  /* Relimit stack after error, in case the limit was overdrawn. */
> ===================================================================
> 
> > 
> > >    st = (TValue *)lj_mem_realloc(L, tvref(L->stack),
> > >  				(MSize)(oldsize*sizeof(TValue)),
> > >  				(MSize)(realsize*sizeof(TValue)));
> > > @@ -80,6 +84,8 @@ static void resizestack(lua_State *L, MSize n)
> > >    L->top = (TValue *)((char *)L->top + delta);
> > >    for (up = gcref(L->openupval); up != NULL; up = gcnext(up))
> > >      setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta));
> > > +
> > > +  G(L)->vmstate = old_vmstate;
> > >  }
> > >  
> > >  /* Relimit stack after error, in case the limit was overdrawn. */

<snipped>

> > > -- 
> > > 2.28.0
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM