* [Tarantool-patches] [PATCH luajit] sysprof: fix sampling outside the VM @ 2025-06-25 10:03 Sergey Kaplun via Tarantool-patches 2025-06-25 13:45 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 1 reply; 4+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2025-06-25 10:03 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches 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; 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) +{ + 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) + }; + const int test_result = test_run_group(tgroup, L); + utils_lua_close(L); + return test_result; +#endif /* LUAJIT_DISABLE_SYSPROF */ +} -- 2.49.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] sysprof: fix sampling outside the VM 2025-06-25 10:03 [Tarantool-patches] [PATCH luajit] sysprof: fix sampling outside the VM Sergey Kaplun via Tarantool-patches @ 2025-06-25 13:45 ` Sergey Bronnikov via Tarantool-patches 2025-06-26 10:11 ` Sergey Kaplun via Tarantool-patches 0 siblings, 1 reply; 4+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2025-06-25 13:45 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 5322 bytes --] 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. > > 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 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. > + }; > + const int test_result = test_run_group(tgroup, L); > + utils_lua_close(L); > + return test_result; > +#endif /* LUAJIT_DISABLE_SYSPROF */ > +} [-- Attachment #2: Type: text/html, Size: 7048 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] sysprof: fix sampling outside the VM 2025-06-25 13:45 ` Sergey Bronnikov via Tarantool-patches @ 2025-06-26 10:11 ` Sergey Kaplun via Tarantool-patches 2025-06-27 13:00 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 1 reply; 4+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2025-06-26 10:11 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] sysprof: fix sampling outside the VM 2025-06-26 10:11 ` Sergey Kaplun via Tarantool-patches @ 2025-06-27 13:00 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 0 replies; 4+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2025-06-27 13:00 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 9201 bytes --] 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 <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); > =================================================================== Thanks! >>> + }; >>> + const int test_result = test_run_group(tgroup, L); >>> + utils_lua_close(L); >>> + return test_result; >>> +#endif /* LUAJIT_DISABLE_SYSPROF */ >>> +} [-- Attachment #2: Type: text/html, Size: 11192 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-06-27 13:00 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-06-25 10:03 [Tarantool-patches] [PATCH luajit] sysprof: fix sampling outside the VM Sergey Kaplun via Tarantool-patches 2025-06-25 13:45 ` Sergey Bronnikov via Tarantool-patches 2025-06-26 10:11 ` Sergey Kaplun via Tarantool-patches 2025-06-27 13:00 ` Sergey Bronnikov via Tarantool-patches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox