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

[Sergey Kaplun]

Igor,

Thanks for the review!

On 25.03.21, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! Please consider my comments below.
> 
> On 09.03.21, Sergey Kaplun wrote:
> > Resizing of the Lua stack is not reported as internal allocation.
> 
> Was it done intentionally? You claim this as a plain fact with no
> rationale. If this is a bug (by design) then mention it explicitly here.

It is obviously a bug.

> Otherwise, you need to provide a reason why you change the semantics of
> Lua stack reallocations in terms of memprof. For me it looks like a bug
> we missed in the previous release: user has nothing to do with guest
> stack reallocations (like with JIT related ones), hence these memory
> manipulations are totally internal. Maybe you have another vision
> regarding this.
> 
> > Moreover, it may lead to crash inside Lua or FF frames.
> 
> This is a symptom, not another reason. So "As a result" fits better than
> "Moreover" here.

Reworded.

> 
> > 
> > Profiler performs reallocation first and after reports corresponding
> 
> Profiler makes no allocations. I believe you would like to say that
> reallocation occurs at first and after memprof reports the event.
> 
> Typo: s/reports corresponding/reports the corresponding/.

Fixed.

> 
> > event. When the stack is resized for local function arguments, the link
> > to previous frame is invalid in the cause of reallocation. Therefore,
> > assertion in `debug_framepc()` failes, because of invalid function
> > reference at previous frame.
> 
> Typo: s/at previous/at the previous/.

Fixed.

> 

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

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
+  ** 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. */
> > diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> > index 1c36c8a..93cc348 100644
> > --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
> > +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> > @@ -125,5 +125,23 @@ test:ok(check_alloc_report(alloc, 25, 18, 100))
> >  -- Collect all previous allocated objects.
> >  test:ok(free.INTERNAL.num == 102)
> >  
> > +-- Test for https://github.com/tarantool/tarantool/issues/5842.
> > +-- We do not interested in report itself.
> > +misc.memprof.start("/dev/null")
> > +-- We need to cause stack resize for local variables at function
> > +-- call. Let's create a new coroutine (all slots are free).
> > +-- It has 1 slot for dummy frame + 39 free slots + 5 extra slots
> > +-- (so-called red zone) + 2 * LJ_FR2 slots. So 50 local variables
> > +-- is enough.
> > +local payload_str = ""
> > +for i = 1, 50 do
> > +  payload_str = payload_str..("local v%d = %d\n"):format(i, i)
> > +end
> > +local f, errmsg = loadstring(payload_str)
> > +assert(f, errmsg)
> 
> Minor: you can use a TAP check here, but feel free to leave a plain
> assert, since you expect <loadstring> works fine always.

We test not `loadstring()` here, assert is for sure that there is no
syntax error in the generated chunk.
Ignoring.

> 
> > +local co = coroutine.create(f)
> > +coroutine.resume(co)
> > +misc.memprof.stop()
> > +
> >  jit.on()
> >  os.exit(test:check() and 0 or 1)
> > -- 
> > 2.28.0
> > 
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun