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

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

[-- 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 --]