[Tarantool-patches] [PATCH luajit] sysprof: fix sampling outside the VM
Sergey Kaplun
skaplun at tarantool.org
Thu Jun 26 13:11:25 MSK 2025
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
More information about the Tarantool-patches
mailing list