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 E54221426930; Wed, 25 Jun 2025 16:45:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E54221426930 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1750859135; bh=qTBX0ooCwTPF9wBtZk6Husy056nqUUJZO4M9HDfCFfk=; 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=nbFTN49mChJWXAcVMBBDmu2WxWubBtuPUz/20j6Myufv5FE7Q4J1N9iCwKqoJ1qyI YQ4rK3ez04f5JN9UsSZQLvURONXfXBwzQAKPWrIA3j9sStxF7pe2XPG+OFMmY00IPR 2vbe9WpX8+3RSg2H42P8qZHv7nR0SN4jYK+PQ1gU= Received: from send264.i.mail.ru (send264.i.mail.ru [95.163.59.103]) (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 12E391426932 for ; Wed, 25 Jun 2025 16:45:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 12E391426932 Received: by exim-smtp-5f9ff66d98-z9ck8 with esmtpa (envelope-from ) id 1uUQRR-00000000DiR-0cBe; Wed, 25 Jun 2025 16:45:33 +0300 Content-Type: multipart/alternative; boundary="------------7fPXSUXJLpGaXgETpFqEacit" Message-ID: <4569e398-f93d-4192-8eae-5408eeaed9e7@tarantool.org> Date: Wed, 25 Jun 2025 16:45:28 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org References: <20250625100327.5563-1-skaplun@tarantool.org> In-Reply-To: <20250625100327.5563-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D919194CF4FC6604E5E3B29DCE494BB4CE71BFEF5FFFF1A200894C459B0CD1B9C45F6F25FEE16B42AC8EDD30083ED68E138F7238C610F314260E3852774D77414F193C0F926A29E4 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7922E451CE6E839B1EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB553375669C5B23AD11DEB7B95BD116816FF71A2FAB13C6E9BB9B9C304070DB3D78D39A96389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6957A4DEDD2346B42CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C2249537E5CF43EE7B3DE76E601842F6C81A12EF20D2F80756B5FB606B96278B59C4276E601842F6C81A127C277FBC8AE2E8B2B4B37CDEA75E1A13AA81AA40904B5D99C9F4D5AE37F343AD1F44FA8B9022EA23BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B2FFDA4F57982C5F435872C767BF85DA227C277FBC8AE2E8BEC7ECBD12528B28AEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5021C6BAA40ABC7575002B1117B3ED6961F6BF3380766F5EE14DB8790748E3E77823CB91A9FED034534781492E4B8EEAD220496FFA5CD4785BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF77153131EA2AC6557A63740A83A32E85C669ECB134CFCEC1354DBFBC96EC0F964F3A61774D94DB0CDA027526E6D8A3D2BF3DF693FCA9E773B35BF49862FDE91F138393AA7CA5A221111DC66A97D0BFE2913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVVWXk7QTiVzHXvOWzLIyXTI= X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D6140C45F6F25FEE16B42AC8EDD30083ED68E82EF3B60509DD4240152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------7fPXSUXJLpGaXgETpFqEacit Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 > +#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 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 */ > +} --------------7fPXSUXJLpGaXgETpFqEacit Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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 */
+}
--------------7fPXSUXJLpGaXgETpFqEacit--