Re: [Tarantool-patches] [PATCH luajit] sysprof: fix sampling outside the VM

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

Hi, Sergey!
Thanks for the review!
Fixed your comments and force-pushed the branch.

On 25.06.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch! Please see my comments below.
> 
> Sergey
> 
> On 6/25/25 13:03, Sergey Kaplun wrote:
> > If the signal by timer is handled outside the VM, the `g->vmstate`
> > equals zero. This was interpreted by the sysprof as the trace with the
> > corresponding number and leads to the assertion failure.
> >
> > This patch fixes that by checking this case and dumping only the host
> > stack outside the VM.
> >
> > Resolves tarantool/tarantool#11185
> > Resolves tarantool/tarantool#11429
> > ---
> >
> > Branch:https://github.com/tarantool/luajit/tree/skaplun/gh-11185-stream-trace-assert
> > Related issues:
> > *https://github.com/tarantool/tarantool/issues/11185
> > *https://github.com/tarantool/tarantool/issues/11429
> >
> > Mentinoned in the test:
> > *https://github.com/tarantool/tarantool/issues/10803
> >
> >   src/lj_sysprof.c                              |  4 +-
> >   .../gh-11185-stream-trace-assert.test.c       | 54 +++++++++++++++++++
> >   2 files changed, 57 insertions(+), 1 deletion(-)
> >   create mode 100644 test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c
> >
> > diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
> > index cf6161a5..013df2cd 100644
> > --- a/src/lj_sysprof.c
> > +++ b/src/lj_sysprof.c
> > @@ -297,7 +297,9 @@ static void sysprof_record_sample(struct sysprof *sp, siginfo_t *info)
> >   {
> >     global_State *g = sp->g;
> >     uint32_t _vmstate = ~(uint32_t)(g->vmstate);
> > -  uint32_t vmstate = _vmstate < LJ_VMST_TRACE ? _vmstate : LJ_VMST_TRACE;
> > +  /* `g->vmstate` is 0 outside the VM. Hence, dump only the host stack. */
> > +  uint32_t vmstate = ~_vmstate == 0 ? LJ_VMST_INTERP :
> > +		     _vmstate < LJ_VMST_TRACE ? _vmstate : LJ_VMST_TRACE;
> 
> Nested ternary operators is not convenient for reading, I would probably 
> convert at least outer condition to
> 
> if-else. Feel free to ignore.

Fixed. See the iterative patch below:

===================================================================
diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
index 013df2cd..4c109074 100644
--- a/src/lj_sysprof.c
+++ b/src/lj_sysprof.c
@@ -297,9 +297,15 @@ static void sysprof_record_sample(struct sysprof *sp, siginfo_t *info)
 {
   global_State *g = sp->g;
   uint32_t _vmstate = ~(uint32_t)(g->vmstate);
+  uint32_t vmstate;
+
   /* `g->vmstate` is 0 outside the VM. Hence, dump only the host stack. */
-  uint32_t vmstate = ~_vmstate == 0 ? LJ_VMST_INTERP :
-                     _vmstate < LJ_VMST_TRACE ? _vmstate : LJ_VMST_TRACE;
+  if (g->vmstate == 0)
+    vmstate = LJ_VMST_INTERP;
+  else if (_vmstate < LJ_VMST_TRACE)
+    vmstate = _vmstate;
+  else
+    vmstate = LJ_VMST_TRACE;
 
   lj_assertX(pthread_self() == sp->thread,
              "bad thread during sysprof record sample");
===================================================================

> 
> >   
> >     lj_assertX(pthread_self() == sp->thread,
> >   	     "bad thread during sysprof record sample");
> > diff --git a/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c b/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c
> > new file mode 100644
> > index 00000000..c4d7ea67
> > --- /dev/null
> > +++ b/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c
> > @@ -0,0 +1,54 @@
> > +#include "lua.h"
> > +#include "lauxlib.h"
> > +
> > +/* Need for skipcond for OS and ARCH. */
> > +#include "lj_arch.h"
> > +
> > +#include "test.h"
> > +#include "utils.h"
> > +
> > +#include <signal.h>
> > +#include <unistd.h>
> > +
> > +/*
> > + * Check that there is no assertion failure during the dump of the
> > + * sample outside the VM.
> > + */
> > +static int gh_11185_stream_trace_assert(void *test_state)
> > +{
> 
> I got the following warnings produced by compiler:
> 
> [ 97%] Building C object 
> test/tarantool-c-tests/CMakeFiles/gh-11185-stream-trace-assert.dir/gh-11185-stream-trace-assert.test.c.o
> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c:17:12: 
> warning: ‘gh_11185_stream_trace_assert’ defined but not used 
> [-Wunused-function]
>     17 | static int gh_11185_stream_trace_assert(void *test_state)
>        |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from 
> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c:8:
> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-c-tests/utils.h:34:13: 
> warning: ‘utils_lua_close’ defined but not used [-Wunused-function]
>     34 | static void utils_lua_close(lua_State *L)
>        |             ^~~~~~~~~~~~~~~
> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-c-tests/utils.h:17:19: 
> warning: ‘utils_lua_init’ defined but not used [-Wunused-function]
>     17 | static lua_State *utils_lua_init(void)
>        |                   ^~~~~~~~~~~~~~
> [100%] Linking C executable gh-11185-stream-trace-assert.c_test

Fixed like the following:

===================================================================
diff --git a/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c b/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c
index 78f08cde..69cd4da1 100644
--- a/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c
+++ b/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c
@@ -5,7 +5,10 @@
 #include "lj_arch.h"
 
 #include "test.h"
+
+#if LJ_HASSYSPROF && !defined(LUAJIT_USE_VALGRIND)
 #include "utils.h"
+#endif /* LJ_HASSYSPROF && !defined(LUAJIT_USE_VALGRIND) */
 
 #include <signal.h>
 #include <unistd.h>
@@ -32,8 +35,10 @@ static int stream_trace_assert(void *test_state)
 int main(void)
 {
 #if LUAJIT_USE_VALGRIND
+        UNUSED(stream_trace_assert);
         return skip_all("Disabled due to #10803");
-#elif LUAJIT_DISABLE_SYSPROF
+#elif !LJ_HASSYSPROF
+        UNUSED(stream_trace_assert);
         return skip_all("Sysprof is disabled");
 #else /* LUAJIT_DISABLE_SYSPROF */
         if (LUAJIT_OS != LUAJIT_OS_LINUX)
===================================================================

> 
> CMake config string: cmake -S . -B build -DLUAJIT_USE_VALGRIND=ON 
> -DLUAJIT_USE_ASAN=OFF -DLUAJIT_USE_SYSMALLOC=ON -DLUAJIT_ENABLE_GC64=ON  
> -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON -DCMAKE_BUILD_TYPE=Debug
> 
> > +	lua_State *L = test_state;
> > +	(void)luaL_dostring(L,
> > +		"misc.sysprof.start({mode = 'C', path = '/dev/null'})");
> > +
> > +	pid_t self_pid = getpid();
> > +	/* Dump the single sample outside the VM. */
> > +	kill(self_pid, SIGPROF);
> > +
> > +	/* No assertion fail -- stop the profiler and exit. */
> > +	(void)luaL_dostring(L, "misc.sysprof.stop()");
> > +	return TEST_EXIT_SUCCESS;
> > +}
> > +
> > +int main(void)
> > +{
> > +#if LUAJIT_USE_VALGRIND
> > +	return skip_all("Disabled due to #10803");
> > +#elif LUAJIT_DISABLE_SYSPROF
> > +	return skip_all("Sysprof is disabled");
> > +#else /* LUAJIT_DISABLE_SYSPROF */
> > +	if (LUAJIT_OS != LUAJIT_OS_LINUX)
> > +		return skip_all("Sysprof is implemented for Linux only");
> > +	if (LUAJIT_TARGET != LUAJIT_ARCH_X86
> > +	    && LUAJIT_TARGET != LUAJIT_ARCH_X64)
> > +		return skip_all("Sysprof is implemented for x86_64 only");
> > +
> > +	lua_State *L = utils_lua_init();
> > +
> > +	const struct test_unit tgroup[] = {
> > +		test_unit_def(gh_11185_stream_trace_assert)
> 
> Do we really need a prefix "gh_11185_" when it is already present in the 
> filename?
> 
> I would omit it.

Removed prefix:

===================================================================
diff --git a/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c b/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c
index c4d7ea67..78f08cde 100644
--- a/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c
+++ b/test/tarantool-c-tests/gh-11185-stream-trace-assert.test.c
@@ -14,7 +14,7 @@
  * Check that there is no assertion failure during the dump of the
  * sample outside the VM.
  */
-static int gh_11185_stream_trace_assert(void *test_state)
+static int stream_trace_assert(void *test_state)
 {
         lua_State *L = test_state;
         (void)luaL_dostring(L,
@@ -45,7 +45,7 @@ int main(void)
         lua_State *L = utils_lua_init();
 
         const struct test_unit tgroup[] = {
-                test_unit_def(gh_11185_stream_trace_assert)
+                test_unit_def(stream_trace_assert)
         };
         const int test_result = test_run_group(tgroup, L);
         utils_lua_close(L);
===================================================================

> 
> > +	};
> > +	const int test_result = test_run_group(tgroup, L);
> > +	utils_lua_close(L);
> > +	return test_result;
> > +#endif /* LUAJIT_DISABLE_SYSPROF */
> > +}

-- 
Best regards,
Sergey Kaplun