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