From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id B1A6B145657D; Thu, 26 Jun 2025 13:11:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B1A6B145657D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1750932686; bh=SjkOadR/ls29q51oWS2aZHRadWOeDjQpBsH6v1VgTvY=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=hoH21zUKI0n2H5ASdrqkLGqu0ezKeyd/YroGV0gvP9lL4DkX7UXgseAVDQXkA34aC 4GjimPXT1hxSvvcD5KXRoS8HgnURYXfdOMOy0Y3XNk0z4DTKR0+9C33DurTPHGVGL+ tpKA+036urUgRLRA0GS+rggDgDBwUMMumtfu0Toc= Received: from send172.i.mail.ru (send172.i.mail.ru [95.163.59.11]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 18EF9145657D for ; Thu, 26 Jun 2025 13:11:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 18EF9145657D Received: by exim-smtp-68f89ddb46-zmckr with esmtpa (envelope-from ) id 1uUjZk-00000000S15-37Cb; Thu, 26 Jun 2025 13:11:25 +0300 Date: Thu, 26 Jun 2025 13:11:25 +0300 To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Message-ID: References: <20250625100327.5563-1-skaplun@tarantool.org> <4569e398-f93d-4192-8eae-5408eeaed9e7@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4569e398-f93d-4192-8eae-5408eeaed9e7@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9BA089F59B72579E45603C27C9928FDF5113BE55D6337BF5F182A05F5380850404C228DA9ACA6FE2742B1C75C63E69C413DE06ABAFEAF6705C2171A7540497770698AC385C22250E2F99B2959C0506FF8 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7D6964C9E324ABA58EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB553375669C5B23AD11DEB7B9D1F5F9FE749CF1AB38BE53E39E69B1A1781F2F09459C8A5D389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B64854413538E1713FCC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB86D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE732FCE54C4D9A645443847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A533B8B0A0885EFC5D5002B1117B3ED696982EB76A61D865ED361FAC1196A180DE823CB91A9FED034534781492E4B8EEAD8D8BB953E4894305BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF23BE3A976540C1D0E3F1FBAD82C2037C4A1D60EB7010114F7F2D415C21FC463B3B77A83432DF81E4DA027526E6D8A3D2074A3984D907BA95F5C345E804811C8549B6B30A87F3CC6E111DC66A97D0BFE2913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVVWXk7QTiVzHJFmUdeOcZh4= X-DA7885C5: F1389399DC042006F255D290C0D534F9F10251FD2B343237DDE7FB2C9366C4F32D357B915F88F4F55B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393FE9E42A757851DB6AA4075091BBBE1512770490C69446544C227BBA2BD0EDC61E49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] sysprof: fix sampling outside the VM X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 > > +#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); =================================================================== > > > + }; > > + const int test_result = test_run_group(tgroup, L); > > + utils_lua_close(L); > > + return test_result; > > +#endif /* LUAJIT_DISABLE_SYSPROF */ > > +} -- Best regards, Sergey Kaplun