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 6E953ED111E; Wed, 5 Feb 2025 12:49:01 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6E953ED111E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1738748941; bh=DRcKk9OxvMXquhv3dapGHw3iw19sp4+fB2FsWdDBHlo=; 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=gMX3y5mcYEC/+CSUg1GCZ8Ll+qFoY3VriiwHQXMNrSdZMUNJS9rkvmB8vrzZ7xrhi QmS7FW9hgLICqV60Va5bah9Ogn8psYDIyHxJGLrD/eKvRRXZ7Q5sdofcjuwee5bf9c iwBAh/W5B+j4F/IGkhBepMMSazn+sKVQ0Iz4/4oU= Received: from send150.i.mail.ru (send150.i.mail.ru [89.221.237.245]) (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 EBD835836EF for ; Wed, 5 Feb 2025 12:48:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EBD835836EF Received: by exim-smtp-6d97ff8cf4-5pjct with esmtpa (envelope-from ) id 1tfc1i-000000008SY-3c0U; Wed, 05 Feb 2025 12:48:59 +0300 Date: Wed, 5 Feb 2025 12:48:17 +0300 To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD985D89FF3B425BBEF98DB62D66459834914625F44760CB589CD62213F67905E7A3B5E2CDB81D6F3AC64A7670B50F81A754329CFB779FE5AF273A8E281587227C524DAF05A372A3159 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7EED5D2FAB4CEB1EDEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C8CB211537E021E78638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D89AFC1AE4261456DE293EEDE8DD5D8263C5D8A5ABCB6B883FCC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8A9FF340AA05FB58CF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8BAEB924C2B054B06E75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A562871BC28622DC6B5002B1117B3ED696E8793041E9D797A392212597CCBD6D77823CB91A9FED034534781492E4B8EEAD220496FFA5CD4785BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFBB6C7FCDA6C03AB52104CC7831E34EF0E1B6275F33C911E8D77C9E3D0AC49287C7834276CD052397C56FE0738BD31C046DA88CB9E170BE05D4BB05108EFC43C7F95C7E7EA1A8A48F111DC66A97D0BFE2913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojUWzrp+XPhZdfuV4LDPy5QQ== X-DA7885C5: 163DC91CD7AA3143F255D290C0D534F95B6B4A50A4B700F26A5BBD32847E366651BE50A96D5725E95B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F739381B31377CF4CA2190FCA11B5D76BB6C1F42C119F8FD1EDD618A7651712E54073E49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Handle all stack layouts in (delayed) TRACE vmevent. 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, Segey! Thanks for the patch! It is really nice that we covered all branches, nice work! LGTM, after fixing a bunch of nits below. Side note: it is better to send v2 in the separate letter to glance it if necessary. On 29.01.25, Sergey Bronnikov wrote: > From: Mike Pall > > Thanks to Sergey Bronnikov and Peter Cawley. > > (cherry picked from commit b138ccfa918518a152bc830fef3d53cd0a922e36) > > When recording a trace using a Lua code like > `repeat until `a >= 'b' > 'b'` a Lua error is encountered > (`attempt to compare string with nil`), which (along with > raising the error) causes an asynchronous trace abort. > The trace abort remains pending until the call of > `lua_pcall(L, 2, 0, 0)` that actually calls `jit.attach(trace_cb, nil)`. I would rather say: | ... until the next reentrance to the LuaJIT VM via `lj_vm_call()` or | `lj_vm_pcall(). Also, please add this comment to the C test header to make it clear why do we use additional `lua_pcall()`-s with dummy functions. > > On handling abort LuaJIT is searching for the topmost Lua frame > on the stack, that was active when the trace abort happened, > it is needed to generate a better error message. > Unfortunately, because the abort was due to an error, and > the error was caught by a `lua_pcall` with unspecified Typo: s/unspecified/an unspecified/ > error function (4th argument), the Lua frame that caused the abort > was already removed as part of error processing, so the search > cannot find it. Furthermore, in this particular case, there are > no Lua frames on the stack, which isn't something the search code > had considered possible. > > Sergey Bronnikov: > * added the description and the test for the problem > > Part of tarantool/tarantool#10709 This is 11055 ticket now. > --- > Branch: https://github.com/tarantool/luajit/tree/ligurio/lj-1087-vm-handler-call > > Related issues: > - https://github.com/LuaJIT/LuaJIT/issues/1087 > - https://github.com/tarantool/tarantool/issues/10709 > > src/lj_trace.c | 26 +-- > test/tarantool-c-tests/CMakeLists.txt | 4 + > .../lj-1087-vm-handler-call.test.c | 170 ++++++++++++++++++ > 3 files changed, 190 insertions(+), 10 deletions(-) > create mode 100644 test/tarantool-c-tests/lj-1087-vm-handler-call.test.c > > diff --git a/src/lj_trace.c b/src/lj_trace.c > index 94cb27e5..6b97cc13 100644 > --- a/src/lj_trace.c > +++ b/src/lj_trace.c > diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt > index c4a402d0..0e7c4698 100644 > --- a/test/tarantool-c-tests/CMakeLists.txt > +++ b/test/tarantool-c-tests/CMakeLists.txt > @@ -36,6 +36,10 @@ add_test_suite_target(tarantool-c-tests > DEPENDS libluajit libtest tarantool-c-tests-build > ) > > +if(NOT LUAJIT_DISABLE_JIT) > + AppendFlags(TESTS_C_FLAGS "-DLJ_HASJIT") > +endif(NOT LUAJIT_DISABLE_JIT) > + It is better just to include in the test instead. See the example of usage in the . Therefore this part is excess. > set(CTEST_SRC_SUFFIX ".test.c") > file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}") > foreach(test_source ${tests}) > diff --git a/test/tarantool-c-tests/lj-1087-vm-handler-call.test.c b/test/tarantool-c-tests/lj-1087-vm-handler-call.test.c > new file mode 100644 > index 00000000..b7db9857 > --- /dev/null > +++ b/test/tarantool-c-tests/lj-1087-vm-handler-call.test.c > @@ -0,0 +1,170 @@ > +#include It is better to undef the `NDEBUG` macro for the non-Debug build to avoid hidden errors in the tests. See other C tests for examples. > +#include This include looks excess. > + > +#include "lua.h" > +#include "lauxlib.h" > + > +#include "test.h" > +#include "utils.h" > + > +/* > + * Test file to demonstrate a segmentation fault under > + * AddressSanitizer, when C function is used as a VM handler I suppose ASan isn't required to reproduce the issue. > + * in LuaJIT: > + * > + * Program received signal SIGSEGV, Segmentation fault. > + * 0x000055555557e77d in trace_abort (J=0x7ffff7f9b6b8) at lj_trace.c:615 > + * 615 lj_vmevent_send(L, TRACE, > + * (gdb) bt > + * > + * See details in https://github.com/LuaJIT/LuaJIT/issues/1087. > + */ > + > +#define UNUSED(x) ((void)(x)) Friendly reminder: You may rebase on the current master and not define this macro here. > + Minor: Please add the comment that `event == NULL` disables the corresponding handler. > +static void jit_attach(lua_State *L, void *cb, const char *event) Nit: Why don't you use `lua_CFunction` here to avoid the cast to it below? > +{ > + lua_getglobal(L, "jit"); > + lua_getfield(L, -1, "attach"); > + lua_pushcfunction(L, (lua_CFunction)cb); > + if (event != NULL) { Nit: Brackets are excess. Feel free to ignore. > + lua_pushstring(L, event); > + } else { > + lua_pushnil(L); > + } > + lua_pcall(L, 2, 0, 0); > +} > + > +static int trace_cb(lua_State *L) { > + UNUSED(L); > + return 0; > +} > + > +static int handle_luafunc_frame(void *test_state) > +{ > +} > + > +static int nop(lua_State *L) > +{ Looks like here should be `UNUSED(L)` too. > + return 0; > +} This function is similar to the `trace_cb()` maybe we just use `nop()` as a trace callback always? > + > +static int cframe(lua_State *L) > +{ > + int rc = luaL_dostring(L, "repeat until nil > 1"); > + assert(rc == 1); > + lua_pop(L, 1); /* Remove errmsg. */ Nit: Please move the comment to the previous line. > + > + lua_pushcfunction(L, nop); > + lua_call(L, 0, 0); > + > + return 0; > +} > + > +static int handle_c_frame(void *test_state) > +{ > + /* Setup. */ > + lua_State *L = test_state; > + jit_attach(L, (void *)trace_cb, "trace"); > + > + lua_pushcfunction(L, cframe); > + lua_call(L, 0, 0); > + > + /* Teardown. */ > + lua_settop(L, 0); > + > + return TEST_EXIT_SUCCESS; > +} > + > +static int global_f(lua_State* L) > +{ > + lua_pushstring(L, "М"); > + lua_pushstring(L, "И"); > + lua_pushstring(L, "Р"); Please use only ASCII characters in the strings. > + lua_concat(L, 3); All this content looks excess, may we just use `nop()` as a `global_f()` instead? > + > + return 1; > +} > + > +static int handle_cont_frame(void *test_state) > +{ > + const char lua_chunk[] = > + "local t = setmetatable({}, { __index = global_f })" > + "for i = 1, 4 do" > + " _ = t[1]" > + "end"; > + > + /* Setup. */ > + lua_State *L = test_state; > + jit_attach(L, (void *)trace_cb, "trace"); > + int res = luaL_dostring(L, "jit.opt.start('minstitch=16')"); It is better to use the 32767 (REF_DROP - REF_BIAS) instead of 16 (with the corresponding comment). See the commit 0fdf06b456e6a8492308e53c96316a25eb058f30 ("test: relax JIT setup in misc.getmetrics test") for the details. > + assert(res == 0); > + lua_pushcfunction(L, global_f); > + lua_setglobal(L, "global_f"); > + > + res = luaL_loadstring(L, lua_chunk); > + assert(res == 0); > + lua_pcall(L, 0, LUA_MULTRET, 0); > + > + /* Teardown. */ > + lua_settop(L, 0); It is better to restore the default minstitch value here too. > + > + return TEST_EXIT_SUCCESS; > +} > + > +static int handle_bottom_frame(void *test_state) > +{ > + lua_State *L = test_state; > + > + /* Attach VM call handler. */ > + jit_attach(L, (void *)trace_cb, "trace"); > + > + /* Load a Lua code that generate a trace abort. */ > + int rc = luaL_dostring(L, "repeat until nil > 1"); > + assert(rc == 1); > + > + /* Triggers segmentation fault. */ > + jit_attach(L, (void *)trace_cb, NULL); > + > + /* Teardown. */ > + lua_settop(L, 0); > + > + return TEST_EXIT_SUCCESS; > +} > + > +int main(void) > +{ > +#if !defined(LJ_HASJIT) It is better to use early return here `if (!LJ_HASJIT)`. > + return skip_all("JIT is disabled"); > +#else /* LJ_HASJIT */ > + lua_State *L = utils_lua_init(); > + const struct test_unit tgroup[] = { > + test_unit_def(handle_luafunc_frame), > + test_unit_def(handle_bottom_frame), > + test_unit_def(handle_cont_frame), > + test_unit_def(handle_c_frame), > + }; > + luaL_openlibs(L); `luaL_openlibs()` is already called inside `utils_lua_init()`. > + int res = luaL_dostring(L, "jit.opt.start('hotloop=1')"); > + assert(res == 0); > + const int test_result = test_run_group(tgroup, L); > + utils_lua_close(L); > + return test_result; > +#endif /* LJ_HASJIT */ > +} > -- > 2.34.1 > -- Best regards, Sergey Kaplun