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 1962F50D036; Thu, 29 Jun 2023 14:16:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1962F50D036 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1688037365; bh=/GV2Vrx8SXCEkD/q8lkDI38TZkaAIU/TWE/mLmQ/4Bc=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=SUYr4GKEG93rtldqNKxCX4fRjS91+bXjeIx9EsQwFbbJq+apu0wQl2Q5HkgpEtaMc OZBfOIVYYcesW8Ma6R6qZ3mdRBUBk/HDYwkUpWt4z1hwnz+BJW3f8GR5CqaHo9cbUM io7Mg2TRcdGNJFL4+Drm0QCDbheG2/1ghcH9l3QA= Received: from smtp31.i.mail.ru (smtp31.i.mail.ru [95.163.41.72]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7D07F5057D4 for ; Thu, 29 Jun 2023 14:16:03 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7D07F5057D4 Received: by smtp31.i.mail.ru with esmtpa (envelope-from ) id 1qEpd4-00DruH-FM; Thu, 29 Jun 2023 14:16:03 +0300 Date: Thu, 29 Jun 2023 14:11:45 +0300 To: Maxim Kokryashkin Message-ID: References: <56e13650b2f5d7536c96c01bbbee3d4c42eedf67.1687439049.git.skaplun@tarantool.org> <1688028293.317381945@f341.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1688028293.317381945@f341.i.mail.ru> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD92F0A134B9DE851264EC9EE4107E35C6B6E52E87AB804C265182A05F5380850404C228DA9ACA6FE2799EEF330B856AA31D51D534FE692347E452BEBB7895AF212F7F54068594B5344 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B264C8851FD8E810EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375CC217B55A7C05578638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D86B2440BE9B82B820D88CCAA1C9373F4E117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BD6672DD12D5A8206A471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FC36C111B7E40517D43AA81AA40904B5D9CF19DD082D7633A078D18283394535A93AA81AA40904B5D98AA50765F790063700B3F27B1A195539D81D268191BDAD3D3666184CF4C3C14F3FC91FA280E0CE3D1A620F70A64A45A98AA50765F7900637F9425D8FA97DB4396D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637AD0424077D726551EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A58791967D5C116C3017A134C0E11EA7C23FF265662C5CF014F87CCE6106E1FC07E67D4AC08A07B9B0CE135D2742255B35CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFB92B8FC525714971E0F287D2F3D35F00153D73CA503AF365C0B0E6098B3CCF94FB9773CE30426B0E00E7A357E10733744F4CAFA2418E54007E8D4ED6E058FC0BE48CAC7CA610320002C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojD2/8pXBAXbLmDmfSZoSogw== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A7695C5E03C3BAFF978FD51D534FE692347E009DB4CE3002C7DBDEDBA653FF35249392D99EB8CC7091A70E183A470755BFD208F19895AA18418972D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/2] Fix lua_yield() from C hook. 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 Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maxim! Thanks for the review! I've fixed all typos you mentioned. Branch is force-pushed. On 29.06.23, Maxim Kokryashkin wrote: > > Hi, Sergey! > Thanks for the patch! > Please consider my comments below. >   > >  > >>From: Mike Pall > >> > >>Reported by Jason Carr. > >> > >>(cherry picked from commit dd5032ed844c56964347c7916db66b0eb11d8091) > >> > >>When we call `lua_yield()` from the C hook the additional continuation > >Typo: s/hook the/hook, an/ Fixed. > >>frame is added. This frame contains a continuation function, PC where we > >>should return, thread GC object to continue, and this frame type and > >Typo: s/thread/a thread/ > >Typo: s/this frame/the frame/ Fixed. > >>size (see details in ). For non-GC64 mode, when we set > >>the GC thread on the Lua stack, stack top isn't incremented, so the GC > >Typo: s/stack top/the stack top/ Fixed. > >>thread overwrites the PC to return. For the GC64 mode the increment is > >>missing before setting frame type and size. > >> > >>This patches fixes the behaviour by adding missing slot incrementing. > >Typo: s/patches/patch/ > >Typo: s/adding/adding the/ > >Typo: s/incrementing/incrementation/ Fixed. > >>Also, it hardens the conditions of using `lj_err_throw()`, according the > >Typo: s/according the/according to the/ Fixed. > >>availability of external unwinder. > >Side note: should we test that change too? I suppose not, because we have no exotic builds for them, and they are __really__ exotic (not gcc|clang, or build with disabled external unwinder). Also, build with internal unwinder only is really hard to test, according our tests for exceptions on traces. > >> > >>The behaviour for the GC64 mode is still wrong due to miscalculation of > >Typo: s/to miscalculation/to a miscalculation/ Fixed. > >>the slot of the GC thread object. This will be fixed in the next > >>commit. > >> > >>Sergey Kaplun: > >>* added the description and the test for the problem > >> > >>Part of tarantool/tarantool#8516 > >>--- > >> src/lj_api.c | 5 +- > >> .../fix-yield-c-hook-script.lua | 19 +++++++ > >> .../tarantool-c-tests/fix-yield-c-hook.test.c | 53 +++++++++++++++++++ > >> 3 files changed, 75 insertions(+), 2 deletions(-) > >> create mode 100644 test/tarantool-c-tests/fix-yield-c-hook-script.lua > >> create mode 100644 test/tarantool-c-tests/fix-yield-c-hook.test.c > >> > >>diff --git a/src/lj_api.c b/src/lj_api.c > >>index dccfe62e..89998815 100644 > >>--- a/src/lj_api.c > >>+++ b/src/lj_api.c > >>diff --git a/test/tarantool-c-tests/fix-yield-c-hook-script.lua b/test/tarantool-c-tests/fix-yield-c-hook-script.lua > >>new file mode 100644 > >>index 00000000..124eeb10 > >>--- /dev/null > >>+++ b/test/tarantool-c-tests/fix-yield-c-hook-script.lua > >>@@ -0,0 +1,19 @@ > >>+-- Auxiliary script to provide Lua functions to be used in the > >>+-- test . > >>+local M = {} > >>+ > >>+-- The function to call, when line hook (calls `lua_yield()`) is > >>+-- set. > >>+M.yield_in_c_hook = function() > >>+ local co = coroutine.create(function() > >>+ -- Just some payload, don't reaaly matter. > >Typo: s/reaaly/really/ Fixed. Thanks! > >>+ local _ = tostring(1) > >>+ end) > >>+ -- Enter coroutine and yield from the 1st line. > >>+ coroutine.resume(co) > >>+ -- Try to get the PC to return and continue to execute the first > >>+ -- line (still will yield from the hook). > >Typo: s/still will/it will still/ Fixed. Thanks! > >>+ coroutine.resume(co) > >>+end > >>+ > >>+return M > >>diff --git a/test/tarantool-c-tests/fix-yield-c-hook.test.c b/test/tarantool-c-tests/fix-yield-c-hook.test.c > >>new file mode 100644 > >>index 00000000..9068360e > >>--- /dev/null > >>+++ b/test/tarantool-c-tests/fix-yield-c-hook.test.c > >>@@ -0,0 +1,53 @@ > >>+#include "lua.h" > >>+ > >>+#include "test.h" > >>+#include "utils.h" > >>+ > >>+#define UNUSED(x) ((void)(x)) > >>+ > >>+/* > >>+ * This test demonstrates LuaJIT incorrect behaviour, when calling > >Typo: s/LuaJIT’s/ Fixed. > >>+ * `lua_yield()` inside C hook. > >Typo: s/inside/inside a/ Fixed. > >>+ * See https://www.freelists.org/post/luajit/BUG-Unable-to-yield-in-a-debug-hook-in-latest-21-beta > >>+ * for details. > >>+ */ > >>+ > >>+static lua_State *main_L = NULL; > >>+ > >>+static void yield(lua_State *L, lua_Debug *ar) > >>+{ > >>+ UNUSED(ar); > >>+ /* Wait for the other coroutine and yield. */ > >>+ if (L != main_L) > >>+ lua_yield(L, 0); > >>+} > >>+ > >>+/* > >>+ * XXX: This test still leads to core dump in the GC64 mode. > >Typo: s/to core/to a core/ Fixed. > >>+ * This will be fixed in the next commit. > >>+ */ See the iterative patch for all typos below. =================================================================== diff --git a/test/tarantool-c-tests/fix-yield-c-hook-script.lua b/test/tarantool-c-tests/fix-yield-c-hook-script.lua index 124eeb10..312c7df5 100644 --- a/test/tarantool-c-tests/fix-yield-c-hook-script.lua +++ b/test/tarantool-c-tests/fix-yield-c-hook-script.lua @@ -6,13 +6,13 @@ local M = {} -- set. M.yield_in_c_hook = function() local co = coroutine.create(function() - -- Just some payload, don't reaaly matter. + -- Just some payload, don't really matter. local _ = tostring(1) end) -- Enter coroutine and yield from the 1st line. coroutine.resume(co) -- Try to get the PC to return and continue to execute the first - -- line (still will yield from the hook). + -- line (it will still yield from the hook). coroutine.resume(co) end diff --git a/test/tarantool-c-tests/fix-yield-c-hook.test.c b/test/tarantool-c-tests/fix-yield-c-hook.test.c index 9068360e..a0de28ae 100644 --- a/test/tarantool-c-tests/fix-yield-c-hook.test.c +++ b/test/tarantool-c-tests/fix-yield-c-hook.test.c @@ -6,8 +6,8 @@ #define UNUSED(x) ((void)(x)) /* - * This test demonstrates LuaJIT incorrect behaviour, when calling - * `lua_yield()` inside C hook. + * This test demonstrates LuaJIT's incorrect behaviour, when + * calling `lua_yield()` inside a C hook. * See https://www.freelists.org/post/luajit/BUG-Unable-to-yield-in-a-debug-hook-in-latest-21-beta * for details. */ @@ -23,7 +23,7 @@ static void yield(lua_State *L, lua_Debug *ar) } /* - * XXX: This test still leads to core dump in the GC64 mode. + * XXX: This test still leads to a core dump in the GC64 mode. * This will be fixed in the next commit. */ static int yield_in_c_hook(void *test_state) =================================================================== > >Best regards, > >Maxim Kokryashkin > >  -- Best regards, Sergey Kaplun