sysprof: fix crash during FFUNC stream

Sometimes, the Lua stack can be inconsistent during the FFUNC execution, which may lead to a sysprof crash during the stack unwinding. This patch replaces the `top_frame` property of `global_State` with `lj_sysprof_topframe` structure, which contains `top_frame` and `ffid` properties. `ffid` property makes sense only when the LuaJIT VM state is set to `FFUNC`. That property is set to the ffid of the fast function that VM is about to execute. In the same time, `top_frame` property is not updated now, so the top frame of the Lua stack can be streamed based on the ffid, and the rest of the Lua stack can be streamed as usual. Also, this patch fixes the build via Makefile.original by adding the `LJ_HASSYSPROF` flag support to it. Resolves tarantool/tarantool#8594 Reviewed-by: Sergey Kaplun <skaplun@tarantool.org> Reviewed-by: Sergey Bronnikov <sergeyb@tarantool.org> Signed-off-by: Igor Munkin <imun@tarantool.org> (cherry picked from commit 285a1b0)
tarantool · Nov 13, 2023 · 3267e77 · 3267e77
1 parent bec052d
commit 3267e77
Show file tree

Hide file tree

Showing 6 changed files with 129 additions and 13 deletions.
diff --git a/src/Makefile.original b/src/Makefile.original
@@ -438,6 +438,9 @@ ifneq (,$(findstring LJ_NO_UNWIND 1,$(TARGET_TESTARCH)))
   DASM_AFLAGS+= -D NO_UNWIND
   TARGET_ARCH+= -DLUAJIT_NO_UNWIND
 endif
+ifneq (,$(findstring LJ_HASSYSPROF 1,$(TARGET_TESTARCH)))
+  DASM_AFLAGS+= -D LJ_HASSYSPROF
+endif
 DASM_AFLAGS+= -D VER=$(subst LJ_ARCH_VERSION_,,$(filter LJ_ARCH_VERSION_%,$(subst LJ_ARCH_VERSION ,LJ_ARCH_VERSION_,$(TARGET_TESTARCH))))
 ifeq (Windows,$(TARGET_SYS))
   DASM_AFLAGS+= -D WIN

diff --git a/src/lj_obj.h b/src/lj_obj.h
@@ -598,6 +598,11 @@ enum {
   GCSmax
 };
 
+struct lj_sysprof_topframe {
+  uint8_t ffid; /* FFID of the fast function VM is about to execute. */
+  TValue *top_frame; /* Top frame for sysprof. */
+};
+
 typedef struct GCState {
   GCSize total;		/* Memory currently allocated. */
   GCSize threshold;	/* Memory threshold. */
@@ -675,7 +680,7 @@ typedef struct global_State {
   MRef ctype_state;	/* Pointer to C type state. */
   GCRef gcroot[GCROOT_MAX];  /* GC roots. */
 #ifdef LJ_HASSYSPROF
-  TValue *top_frame;	/* Top frame for sysprof. */
+  struct lj_sysprof_topframe top_frame_info;	/* Top frame info for sysprof. */
 #endif
 } global_State;
 

diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
@@ -109,6 +109,12 @@ static void stream_epilogue(struct sysprof *sp)
   lj_wbuf_addbyte(&sp->out, LJP_EPILOGUE_BYTE);
 }
 
+static void stream_ffunc_impl(struct lj_wbuf *buf, uint8_t ffid)
+{
+  lj_wbuf_addbyte(buf, LJP_FRAME_FFUNC);
+  lj_wbuf_addu64(buf, ffid);
+}
+
 static void stream_lfunc(struct lj_wbuf *buf, const GCfunc *func)
 {
   lj_assertX(isluafunc(func), "bad lua function in sysprof stream");
@@ -129,8 +135,7 @@ static void stream_cfunc(struct lj_wbuf *buf, const GCfunc *func)
 static void stream_ffunc(struct lj_wbuf *buf, const GCfunc *func)
 {
   lj_assertX(isffunc(func), "bad fast function in sysprof stream");
-  lj_wbuf_addbyte(buf, LJP_FRAME_FFUNC);
-  lj_wbuf_addu64(buf, func->c.ffid);
+  stream_ffunc_impl(buf, func->c.ffid);
 }
 
 static void stream_frame_lua(struct lj_wbuf *buf, const cTValue *frame)
@@ -148,7 +153,7 @@ static void stream_frame_lua(struct lj_wbuf *buf, const cTValue *frame)
     lj_assertX(0, "bad function type in sysprof stream");
 }
 
-static void stream_backtrace_lua(struct sysprof *sp)
+static void stream_backtrace_lua(struct sysprof *sp, uint32_t vmstate)
 {
   global_State *g = sp->g;
   struct lj_wbuf *buf = &sp->out;
@@ -158,8 +163,19 @@ static void stream_backtrace_lua(struct sysprof *sp)
   lj_assertX(g != NULL, "uninitialized global state in sysprof state");
   L = gco2th(gcref(g->cur_L));
   lj_assertG(L != NULL, "uninitialized Lua state in sysprof state");
+  /*
+  ** Lua stack may be inconsistent during the execution of a
+  ** fast-function, so instead of updating the `top_frame` for
+  ** it, its `ffid` is set instead. The first frame on the
+  ** result stack is streamed manually, and the rest of the
+  ** stack is streamed based on the previous `top_frame` value.
+  */
+  if (vmstate == LJ_VMST_FFUNC) {
+    uint8_t ffid = g->top_frame_info.ffid;
+    stream_ffunc_impl(buf, ffid);
+  }
 
-  top_frame = g->top_frame - 1; //(1 + LJ_FR2)
+  top_frame = g->top_frame_info.top_frame - 1;
 
   bot = tvref(L->stack) + LJ_FR2;
   /* Traverse frames backwards */
@@ -234,7 +250,7 @@ static void stream_trace(struct sysprof *sp, uint32_t vmstate)
 static void stream_guest(struct sysprof *sp, uint32_t vmstate)
 {
   lj_wbuf_addbyte(&sp->out, (uint8_t)vmstate);
-  stream_backtrace_lua(sp);
+  stream_backtrace_lua(sp, vmstate);
   stream_backtrace_host(sp);
 }
 

diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
@@ -53,6 +53,7 @@
 |.define RDL,		RCL
 |.define TMPR,		r10
 |.define TMPRd,		r10d
+|.define TMPRb,		r10b
 |.define ITYPE,		r11
 |.define ITYPEd,	r11d
 |
@@ -353,14 +354,29 @@
 |// it syncs with the BASE register only when the control is passed to
 |// user code. So we need to sync the BASE on each vmstate change to
 |// keep it consistent.
+|// The only exception are FFUNCs because sometimes even internal BASE
+|// stash is inconsistent for them. To address that issue, their ffid
+|// is stashed instead, so the corresponding frame can be streamed
+|// manually.
 |.macro set_vmstate_sync_base, st
 |.if LJ_HASSYSPROF
 |  set_vmstate INTERP  // Guard for non-atomic VM context restoration
-|  mov qword [DISPATCH+DISPATCH_GL(top_frame)], BASE
+|  mov qword [DISPATCH+DISPATCH_GL(top_frame_info.top_frame)], BASE
 |.endif
 |  set_vmstate st
 |.endmacro
 |
+|.macro set_vmstate_ffunc
+|.if LJ_HASSYSPROF
+|  set_vmstate INTERP
+|  mov TMPR, [BASE - 16]
+|  cleartp LFUNC:TMPR // Obtain plain address value. Equivalent of `gcval`.
+|  mov TMPRb, LFUNC:TMPR->ffid
+|  mov byte [DISPATCH+DISPATCH_GL(top_frame_info.ffid)], TMPRb
+|.endif
+|  set_vmstate FFUNC
+|.endmacro
+|
 |// Uses TMPRd (r10d).
 |.macro save_vmstate
 |.if not WIN
@@ -376,7 +392,7 @@
 |  set_vmstate INTERP
 |  mov TMPR, SAVE_L
 |  mov TMPR, L:TMPR->base
-|  mov qword [DISPATCH+DISPATCH_GL(top_frame)], TMPR
+|  mov qword [DISPATCH+DISPATCH_GL(top_frame_info.top_frame)], TMPR
 |.endif
 |  mov TMPRd, SAVE_VMSTATE
 |  mov dword [DISPATCH+DISPATCH_GL(vmstate)], TMPRd
@@ -1183,7 +1199,7 @@ static void build_subroutines(BuildCtx *ctx)
   |
   |.macro .ffunc, name
   |->ff_ .. name:
-  |  set_vmstate_sync_base FFUNC
+  |  set_vmstate_ffunc
   |.endmacro
   |
   |.macro .ffunc_1, name

diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
@@ -101,6 +101,7 @@
 |.define FCARG2,	CARG2d
 |
 |.define XCHGd,		r11d		// TMP on x64, used for exchange.
+|.define XCHGb,		r11b		// TMPRb on x64, used for exchange.
 |.endif
 |
 |// Type definitions. Some of these are only used for documentation.
@@ -451,14 +452,36 @@
 |// it syncs with the BASE register only when the control is passed to
 |// user code. So we need to sync the BASE on each vmstate change to
 |// keep it consistent.
+|// The only exception are FFUNCs because sometimes even internal BASE
+|// stash is inconsistent for them. To address that issue, their ffid
+|// is stashed instead, so the corresponding frame can be streamed
+|// manually.
 |.macro set_vmstate_sync_base, st
 |.if LJ_HASSYSPROF
 |  set_vmstate INTERP  // Guard for non-atomic VM context restoration
-|  mov dword [DISPATCH+DISPATCH_GL(top_frame)], BASE
+|  mov dword [DISPATCH+DISPATCH_GL(top_frame_info.top_frame)], BASE
 |.endif
 |  set_vmstate st
 |.endmacro
 |
+|.macro set_vmstate_ffunc
+|.if LJ_HASSYSPROF
+|  set_vmstate INTERP
+|.if not X64
+|  mov SPILLECX, ecx
+|  mov LFUNC:ecx, [BASE - 4]
+|  mov cl, LFUNC:ecx->ffid
+|  mov byte [DISPATCH+DISPATCH_GL(top_frame_info.ffid)], cl
+|  mov ecx, SPILLECX
+|.else // X64
+|  mov LFUNC:XCHGd, [BASE - 8]
+|  mov XCHGb, LFUNC:XCHGd->ffid
+|  mov byte [DISPATCH+DISPATCH_GL(top_frame_info.ffid)], XCHGb
+|.endif // X64
+|.endif // LJ_HASSYSPROF
+|  set_vmstate FFUNC
+|.endmacro
+|
 |// Uses spilled ecx on x86 or XCHGd (r11d) on x64.
 |.macro save_vmstate
 |.if not WIN
@@ -485,7 +508,7 @@
 |.if LJ_HASSYSPROF
 |  mov ecx, SAVE_L
 |  mov ecx, L:ecx->base
-|  mov dword [DISPATCH+DISPATCH_GL(top_frame)], ecx
+|  mov dword [DISPATCH+DISPATCH_GL(top_frame_info.top_frame)], ecx
 |.endif
 |  mov ecx, SAVE_VMSTATE
 |  mov dword [DISPATCH+DISPATCH_GL(vmstate)], ecx
@@ -494,7 +517,7 @@
 |.if LJ_HASSYSPROF
 |  mov XCHGd, SAVE_L
 |  mov XCHGd, L:XCHGd->base
-|  mov dword [DISPATCH+DISPATCH_GL(top_frame)], XCHGd
+|  mov dword [DISPATCH+DISPATCH_GL(top_frame_info.top_frame)], XCHGd
 |.endif
 |  mov XCHGd, SAVE_VMSTATE
 |  mov dword [DISPATCH+DISPATCH_GL(vmstate)], XCHGd
@@ -1483,7 +1506,7 @@ static void build_subroutines(BuildCtx *ctx)
   |
   |.macro .ffunc, name
   |->ff_ .. name:
-  |  set_vmstate_sync_base FFUNC
+  |  set_vmstate_ffunc
   |.endmacro
   |
   |.macro .ffunc_1, name

diff --git a/test/tarantool-tests/gh-8594-sysprof-ffunc-crash.test.lua b/test/tarantool-tests/gh-8594-sysprof-ffunc-crash.test.lua
@@ -0,0 +1,53 @@
+local tap = require('tap')
+local test = tap.test('gh-8594-sysprof-ffunc-crash'):skipcond({
+  ['Sysprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
+                                               jit.arch ~= 'x64',
+  ['Sysprof is implemented for Linux only'] = jit.os ~= 'Linux',
+})
+
+test:plan(1)
+
+jit.off()
+-- XXX: Run JIT tuning functions in a safe frame to avoid errors
+-- thrown when LuaJIT is compiled with JIT engine disabled.
+pcall(jit.flush)
+
+local TMP_BINFILE = '/dev/null'
+
+-- XXX: The best way to test the issue is to set the profile
+-- interval to be as short as possible. However, our CI is
+-- not capable of handling such intense testing, so it was a
+-- forced decision to reduce the sampling frequency for it. As a
+-- result, it is now less likely to reproduce the issue
+-- statistically, but the test case is still valid.
+
+-- GitHub always sets[1] the `CI` environment variable to `true`
+-- for every step in a workflow.
+-- [1]: https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables
+local CI = os.getenv('CI') == 'true'
+
+-- Profile interval and number of iterations for CI are
+-- empirical. Non-CI profile interval is set to be as short
+-- as possible, so the issue is more likely to reproduce.
+-- Non-CI number of iterations is greater for the same reason.
+local PROFILE_INTERVAL = CI and 3 or 1
+local N_ITERATIONS = CI and 1e5 or 1e6
+
+local res, err = misc.sysprof.start{
+  mode = 'C',
+  interval = PROFILE_INTERVAL,
+  path = TMP_BINFILE,
+}
+assert(res, err)
+
+for i = 1, N_ITERATIONS do
+  -- XXX: `tostring` is FFUNC.
+  tostring(i)
+end
+
+res, err = misc.sysprof.stop()
+assert(res, err)
+
+test:ok(true, 'FFUNC frames were streamed correctly')
+
+os.exit(test:check() and 0 or 1)