Hi, Sergey, LGTM Sergey On 6/26/25 13:11, Sergey Kaplun wrote: > 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"); > =================================================================== Thanks! >>> >>> 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 >>> +#include >>> + >>> +/* >>> + * 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 > #include > @@ -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); > =================================================================== Thanks! >>> + }; >>> + const int test_result = test_run_group(tgroup, L); >>> + utils_lua_close(L); >>> + return test_result; >>> +#endif /* LUAJIT_DISABLE_SYSPROF */ >>> +}