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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Igor,

I've fixed typos in the commit message and the comment.

On 26.03.21, Igor Munkin wrote:
> 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/.

Fixed.

> 
> > the case of reallocation. Therefore, the assertion in `debug_framepc()`
> > fails.
> > 
> > Resolves tarantool/tarantool#5842
> > Follows up tarantool/tarantool#5442
> > ===================================================================

The new commit message is the following:
===================================================================
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 the previous frame is invalid in
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

But not only in <lj_gc.c> some old value is saved to be reused later:

| $ grep -rn -P '\Wold[A-Za-z]' src/l*.c | grep -v -e define -e "\*\*" -e "\*/" -e "\*/" | wc -l
| 123

Instead one-letter abbreviation (grep out comments and most frequently
used constructions):

| $ grep -rn -P '\Wo[^l]' src/l*.c | grep -v -e "\Wo\W" -e "\Wo[12]" | grep -v \
|   -e obj -e open -e op -e opt -e octet -e out -e on \
|   -e ofs -e offs -e ok -e oddspill -e os -e orign \
|   -e define -e overhead -e or \
|   -e "\*/" -e "\*\*" -e "/\*" | wc -l
| 25

The huge part of them was introduced via memory profiler by us.

So, looks like your codestyle suggestion contradicts to codestyle
of LuaJIT as it is. It is not described anywhere else, so looks like
that source of true is the sources by themseles.

> that approach "intact", it shows that the semantics of your code are the

It's already the same, it is obvious by reading, plus comment describes
the behaviour.

> 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

None of them is usual in use for LuaJIT's C codebase, so they can't be
used.

> combinations. And nobody can stop contributor from this, since it is
> "more consistent for the current TU".

But with your approach naming will be choosed not like the most usual,
but "same in the other file". I think that it will lead to the code
inconsistency even in one 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.

Ignoring for now.

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

Fixed, force-pushed.

===================================================================
diff --git a/src/lj_state.c b/src/lj_state.c
index c86e098..5701572 100644
--- a/src/lj_state.c
+++ b/src/lj_state.c
@@ -70,7 +70,7 @@ static void resizestack(lua_State *L, MSize n)
 
   /*
   ** Lua stack is inconsistent durent reallocation, profilers
-  ** depends on vmstate during reports, so set vmstate to INTERP
+  ** depend on vmstate during reports, so set vmstate to INTERP
   ** to avoid inconsistent behaviour.
   */
   setvmstate(G(L), INTERP);
===================================================================

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

-- 
Best regards,
Sergey Kaplun