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 E82A3978FA2; Thu, 18 Jan 2024 16:18:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E82A3978FA2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1705583901; bh=glx4k0GPQD57LP4+6vCbZTYThoVnIDsIsY5u8gYzfV0=; 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=M2fV0ZK0oddp2dR4ilbWWbu0zIGd5XK6i6iBhQRIwcvK32OzflmpDPFfb53V4mPrs /kz+o9HBf8ytnM6xHGMF+iycfLoVWwRzboklG6JUzZ7zV7sq5GmbDWXdo57kWbmVC9 tTay6UTgYnFz5Pwir/KDAx1BsLen5OrdG8nvOBVU= Received: from smtp17.i.mail.ru (smtp17.i.mail.ru [95.163.41.70]) (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 13699978FA2 for ; Thu, 18 Jan 2024 16:18:19 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 13699978FA2 Received: by smtp17.i.mail.ru with esmtpa (envelope-from ) id 1rQSHh-000000027O6-2nZ0; Thu, 18 Jan 2024 16:18:18 +0300 Content-Type: multipart/alternative; boundary="------------NXfPBsjeGMwXcmifD697jC6E" Message-ID: Date: Thu, 18 Jan 2024 16:18:17 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Maxim Kokryashkin References: <20231122143534.11330-1-max.kokryashkin@gmail.com> <20231122143534.11330-2-max.kokryashkin@gmail.com> <45b0a53a-c85a-44cd-ba43-6df62d76befa@tarantool.org> <1701875230.476933162@f762.i.mail.ru> In-Reply-To: <1701875230.476933162@f762.i.mail.ru> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9AE5B4AFB3AE2A5909B38FFCAAE1FB38606686B0B5065941A182A05F538085040EB1DE77B1CD837DFF378A8CA21F699D684BC6785A28E033529CAE96B067714AB94981B86E6AE2D10 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75DF2B1F23425CAE5EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D07BBD2EBFB7BF888638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8CAA95E5B9B67D9420F015411F133E016D46DAC5CD2BCA45CCC7F00164DA146DAFE8445B8C89999728AA50765F790063783E00425F71A4181389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC82FFDA4F57982C5F4F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C62B3BD3CC35DA5882D242C3BD2E3F4C64AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3494F3C0D6DD222D9BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CFED8438A78DFE0A9E1DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3C8F21CEC4765490D35872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5235C56AC91F63BAD5002B1117B3ED6967D6993C82790E18C4869453249F34FA4823CB91A9FED034534781492E4B8EEAD09122B91796FF21FBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFF73E6893F3F01D98B8BDD949D6A123EC307BADF207DC9784DB9F126E04B2717D96A3CD8EA40B8F976197189FEE4B1C7ACE3F47F7B3CE79ADD61D956C99E847B3117370DFAADCEF06C226CC413062362A913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojjkiC0IEfsqJbtvcc4fOcZQ== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F25884580A357A040B509E5FE693A2F4CD4B1000BCDB5C0691DF710E40F01C8290AE55DC645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v3 1/3] Improve error reporting on stack overflow. 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 Cc: Maksim Kokryashkin , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------NXfPBsjeGMwXcmifD697jC6E Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Max the problem is reproduced, thanks! LGTM On 12/6/23 18:07, Maxim Kokryashkin wrote: > Hi, Sergey! > Thanks for the review! > The test case indeed fails only for the GC64 mode before the patch. > I've tried to make it fail for non-GC64 mode, but the same issue arised — > an arbitrary amount of slots needed, and that amount needs to be modified > each time there is a minor change in the Lua stack behavior. With that in > mind, I’ve decided to leave it as it is. Added a comment and modified the > commit message. The new commit message: > === > Improve error reporting on stack overflow. > Thanks to Nicolas Lebedenco. > (cherry-picked from commit 8135de2a0204e6acd92b231131c4a1e0be03ac1c) > The `lj_state_growstack` doesn't account for a potential error > handler invocation by `xpcall`, which may lead to the second > error while handling a stack overflow, resulting in a misleading > error "error in error handling", while the real issue is a stack > overflow. This patch addresses this issue by fixing the condition > at which stack overflow errors are thrown. Now it's thrown if the > stack size is at least at the limit, instead of when it is over > the limit. > The test case is very fragile, and the issue is hard to reproduce > in a robust way. The provided reproducer failed before the patch > for GC64 mode only. Non-GC64 mode requires an arbitrary number > of stack slots to be filled, which is even more fragile. > This commit also disables the second test from > `lj-603-err-snap-restore`, since after this patch and the two > follow-ups for it, there is no such amount of stack slots with > which the test works the way it should. > Lastly, this patch adds an alternative to `luacmd` to the > `utils.exec` module, which is called `luabin`. That function is > similar to the `luacmd`, with the only difference of returning > only the executable itself without any additional CLI options > passed. > Maxim Kokryashkin: > * added the description and the test for the problem > Part of tarantool/tarantool#9145 > === > And the diff for the comment: > === > diff --git > a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua > b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua > index 45a888f4..e5f9d9a4 100644 > --- a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua > +++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua > @@ -3,6 +3,12 @@ local tap = require('tap') >  local test = tap.test('lj-962-stack-overflow-report') >  test:plan(1) > +-- XXX: The test case is very fragile, and the issue is hard to > +-- reproduce in a robust way. The provided reproducer failed > +-- before the patch for GC64 mode only. Non-GC64 mode requires > +-- an arbitrary number of stack slots to be filled, which is > +-- even more fragile. > + >  local LUABIN = require('utils').exec.luabin(arg) >  local SCRIPT = arg[0]:gsub('%.test%.lua$', '/script.lua') >  local output = io.popen(LUABIN .. ' 2>&1 ' .. SCRIPT, 'r'):read('*all') > === > -- > Best regards, > Maxim Kokryashkin > > Пятница, 24 ноября 2023, 15:22 +03:00 от Sergey Bronnikov > : > Hello, Max > > thanks for the patch! > > proposed test is passed after reverting a fix > > > On 11/22/23 17:35, Maksim Kokryashkin wrote: > > From: Mike Pall > > > > Thanks to Nicolas Lebedenco. > > > > (cherry-picked from commit 8135de2a0204e6acd92b231131c4a1e0be03ac1c) > > > > The `lj_state_growstack` doesn't account for a potential error > > handler invocation by `xpcall`, which may lead to the second > > error while handling a stack overflow, resulting in a misleading > > error "error in error handling", while the real issue is a stack > > overflow. This patch addresses this issue by fixing the condition > > at which stack overflow errors are thrown. Now it's thrown if the > > stack size is at least at the limit, instead of when it is over > > the limit. > > > > This commit also disables the second test from > > `lj-603-err-snap-restore`, since after this patch and the two > > follow-ups for it, there is no such amount of stack slots with > > which the test works the way it should. > > > > Lastly, this patch adds an alternative to `luacmd` to the > > `utils.exec` module, which is called `luabin`. That function is > > similar to the `luacmd`, with the only difference of returning > > only the executable itself without any additional CLI options > > passed. > > > > Maxim Kokryashkin: > > * added the description and the test for the problem > > > > Part of tarantool/tarantool#9145 > > --- > > src/lj_state.c | 2 +- > > .../lj-603-err-snap-restore.test.lua | 1 + > > .../lj-962-stack-overflow-report.test.lua | 10 ++++++++++ > > .../lj-962-stack-overflow-report/script.lua | 3 +++ > > test/tarantool-tests/utils/exec.lua | 15 ++++++++++++--- > > 5 files changed, 27 insertions(+), 4 deletions(-) > > create mode 100644 > test/tarantool-tests/lj-962-stack-overflow-report.test.lua > > create mode 100644 > test/tarantool-tests/lj-962-stack-overflow-report/script.lua > > > > diff --git a/src/lj_state.c b/src/lj_state.c > > index 684336d5..76153bad 100644 > > --- a/src/lj_state.c > > +++ b/src/lj_state.c > > @@ -132,7 +132,7 @@ void LJ_FASTCALL > lj_state_growstack(lua_State *L, MSize need) > > n = LJ_STACK_MAX; > > } > > resizestack(L, n); > > - if (L->stacksize > LJ_STACK_MAXEX) > > + if (L->stacksize >= LJ_STACK_MAXEX) > > lj_err_msg(L, LJ_ERR_STKOV); > > } > > > > diff --git > a/test/tarantool-tests/lj-603-err-snap-restore.test.lua > b/test/tarantool-tests/lj-603-err-snap-restore.test.lua > > index 96ebf92c..f5c8474f 100644 > > --- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua > > +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua > > @@ -36,6 +36,7 @@ local function do_test() > > -- Tarantool at start, so just skip test for it. > > -- luacheck: no global > > ['Disable test for Tarantool'] = _TARANTOOL, > > + ['Stack overflow is now handled differently'] = true, > > }) > > > > test:ok(not handler_is_called) > > diff --git > a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua > b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua > > new file mode 100644 > > index 00000000..45a888f4 > > --- /dev/null > > +++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua > > @@ -0,0 +1,10 @@ > > +local tap = require('tap') > > +-- The test reproduces the problem only for GC64 mode with > enabled JIT. > > +local test = tap.test('lj-962-stack-overflow-report') > > +test:plan(1) > > + > > +local LUABIN = require('utils').exec.luabin(arg) > > +local SCRIPT = arg[0]:gsub('%.test%.lua$', '/script.lua') > > +local output = io.popen(LUABIN .. ' 2>&1 ' .. SCRIPT, > 'r'):read('*all') > > +test:like(output, 'stack overflow', 'stack overflow reported > correctly') > > +test:done(true) > > diff --git > a/test/tarantool-tests/lj-962-stack-overflow-report/script.lua > b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua > > new file mode 100644 > > index 00000000..31c5ca33 > > --- /dev/null > > +++ b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua > > @@ -0,0 +1,3 @@ > > +-- XXX: Function `f` is global to avoid using an additional > stack slot. > > +-- luacheck: no global > > +f = function() f() end; f() > > diff --git a/test/tarantool-tests/utils/exec.lua > b/test/tarantool-tests/utils/exec.lua > > index a56ca2dc..48a08ba5 100644 > > --- a/test/tarantool-tests/utils/exec.lua > > +++ b/test/tarantool-tests/utils/exec.lua > > @@ -1,14 +1,23 @@ > > local M = {} > > > > -function M.luacmd(args) > > +local function executable_idx(args) > > -- arg[-1] is guaranteed to be not nil. > > local idx = -2 > > while args[idx] do > > assert(type(args[idx]) == 'string', 'Command part have to be a > string') > > idx = idx - 1 > > end > > - -- return the full command with flags. > > - return table.concat(args, ' ', idx + 1, -1) > > + return idx + 1 > > +end > > + > > +function M.luabin(args) > > + -- Return only the executable. > > + return args[executable_idx(args)] > > +end > > + > > +function M.luacmd(args) > > + -- Return the full command with flags. > > + return table.concat(args, ' ', executable_idx(args), -1) > > end > > > > local function makeenv(tabenv) > --------------NXfPBsjeGMwXcmifD697jC6E Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi, Max


the problem is reproduced, thanks!

LGTM


On 12/6/23 18:07, Maxim Kokryashkin wrote:
Hi, Sergey!
Thanks for the review!
The test case indeed fails only for the GC64 mode before the patch.
I've tried to make it fail for non-GC64 mode, but the same issue arised —
an arbitrary amount of slots needed, and that amount needs to be modified
each time there is a minor change in the Lua stack behavior. With that in
mind, I’ve decided to leave it as it is. Added a comment and modified the
commit message. The new commit message:
===
Improve error reporting on stack overflow.
 
Thanks to Nicolas Lebedenco.
 
(cherry-picked from commit 8135de2a0204e6acd92b231131c4a1e0be03ac1c)
 
The `lj_state_growstack` doesn't account for a potential error
handler invocation by `xpcall`, which may lead to the second
error while handling a stack overflow, resulting in a misleading
error "error in error handling", while the real issue is a stack
overflow. This patch addresses this issue by fixing the condition
at which stack overflow errors are thrown. Now it's thrown if the
stack size is at least at the limit, instead of when it is over
the limit.
 
The test case is very fragile, and the issue is hard to reproduce
in a robust way. The provided reproducer failed before the patch
for GC64 mode only. Non-GC64 mode requires an arbitrary number
of stack slots to be filled, which is even more fragile.
 
This commit also disables the second test from
`lj-603-err-snap-restore`, since after this patch and the two
follow-ups for it, there is no such amount of stack slots with
which the test works the way it should.
 
Lastly, this patch adds an alternative to `luacmd` to the
`utils.exec` module, which is called `luabin`. That function is
similar to the `luacmd`, with the only difference of returning
only the executable itself without any additional CLI options
passed.
 
Maxim Kokryashkin:
* added the description and the test for the problem
 
Part of tarantool/tarantool#9145
===
 
And the diff for the comment:
===
diff --git a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
index 45a888f4..e5f9d9a4 100644
--- a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
+++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
@@ -3,6 +3,12 @@ local tap = require('tap')
 local test = tap.test('lj-962-stack-overflow-report')
 test:plan(1)
 
+-- XXX: The test case is very fragile, and the issue is hard to
+-- reproduce in a robust way. The provided reproducer failed
+-- before the patch for GC64 mode only. Non-GC64 mode requires
+-- an arbitrary number of stack slots to be filled, which is
+-- even more fragile.
+
 local LUABIN = require('utils').exec.luabin(arg)
 local SCRIPT = arg[0]:gsub('%.test%.lua$', '/script.lua')
 local output = io.popen(LUABIN .. ' 2>&1 ' .. SCRIPT, 'r'):read('*all')
 
===
--
Best regards,
Maxim Kokryashkin
 
 
Пятница, 24 ноября 2023, 15:22 +03:00 от Sergey Bronnikov <sergeyb@tarantool.org>:
 
Hello, Max

thanks for the patch!

proposed test is passed after reverting a fix


On 11/22/23 17:35, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
>
> Thanks to Nicolas Lebedenco.
>
> (cherry-picked from commit 8135de2a0204e6acd92b231131c4a1e0be03ac1c)
>
> The `lj_state_growstack` doesn't account for a potential error
> handler invocation by `xpcall`, which may lead to the second
> error while handling a stack overflow, resulting in a misleading
> error "error in error handling", while the real issue is a stack
> overflow. This patch addresses this issue by fixing the condition
> at which stack overflow errors are thrown. Now it's thrown if the
> stack size is at least at the limit, instead of when it is over
> the limit.
>
> This commit also disables the second test from
> `lj-603-err-snap-restore`, since after this patch and the two
> follow-ups for it, there is no such amount of stack slots with
> which the test works the way it should.
>
> Lastly, this patch adds an alternative to `luacmd` to the
> `utils.exec` module, which is called `luabin`. That function is
> similar to the `luacmd`, with the only difference of returning
> only the executable itself without any additional CLI options
> passed.
>
> Maxim Kokryashkin:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9145
> ---
> src/lj_state.c | 2 +-
> .../lj-603-err-snap-restore.test.lua | 1 +
> .../lj-962-stack-overflow-report.test.lua | 10 ++++++++++
> .../lj-962-stack-overflow-report/script.lua | 3 +++
> test/tarantool-tests/utils/exec.lua | 15 ++++++++++++---
> 5 files changed, 27 insertions(+), 4 deletions(-)
> create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report.test.lua
> create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report/script.lua
>
> diff --git a/src/lj_state.c b/src/lj_state.c
> index 684336d5..76153bad 100644
> --- a/src/lj_state.c
> +++ b/src/lj_state.c
> @@ -132,7 +132,7 @@ void LJ_FASTCALL lj_state_growstack(lua_State *L, MSize need)
> n = LJ_STACK_MAX;
> }
> resizestack(L, n);
> - if (L->stacksize > LJ_STACK_MAXEX)
> + if (L->stacksize >= LJ_STACK_MAXEX)
> lj_err_msg(L, LJ_ERR_STKOV);
> }
>
> diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
> index 96ebf92c..f5c8474f 100644
> --- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
> +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
> @@ -36,6 +36,7 @@ local function do_test()
> -- Tarantool at start, so just skip test for it.
> -- luacheck: no global
> ['Disable test for Tarantool'] = _TARANTOOL,
> + ['Stack overflow is now handled differently'] = true,
> })
>
> test:ok(not handler_is_called)
> diff --git a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
> new file mode 100644
> index 00000000..45a888f4
> --- /dev/null
> +++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
> @@ -0,0 +1,10 @@
> +local tap = require('tap')
> +-- The test reproduces the problem only for GC64 mode with enabled JIT.
> +local test = tap.test('lj-962-stack-overflow-report')
> +test:plan(1)
> +
> +local LUABIN = require('utils').exec.luabin(arg)
> +local SCRIPT = arg[0]:gsub('%.test%.lua$', '/script.lua')
> +local output = io.popen(LUABIN .. ' 2>&1 ' .. SCRIPT, 'r'):read('*all')
> +test:like(output, 'stack overflow', 'stack overflow reported correctly')
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-962-stack-overflow-report/script.lua b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua
> new file mode 100644
> index 00000000..31c5ca33
> --- /dev/null
> +++ b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua
> @@ -0,0 +1,3 @@
> +-- XXX: Function `f` is global to avoid using an additional stack slot.
> +-- luacheck: no global
> +f = function() f() end; f()
> diff --git a/test/tarantool-tests/utils/exec.lua b/test/tarantool-tests/utils/exec.lua
> index a56ca2dc..48a08ba5 100644
> --- a/test/tarantool-tests/utils/exec.lua
> +++ b/test/tarantool-tests/utils/exec.lua
> @@ -1,14 +1,23 @@
> local M = {}
>
> -function M.luacmd(args)
> +local function executable_idx(args)
> -- arg[-1] is guaranteed to be not nil.
> local idx = -2
> while args[idx] do
> assert(type(args[idx]) == 'string', 'Command part have to be a string')
> idx = idx - 1
> end
> - -- return the full command with flags.
> - return table.concat(args, ' ', idx + 1, -1)
> + return idx + 1
> +end
> +
> +function M.luabin(args)
> + -- Return only the executable.
> + return args[executable_idx(args)]
> +end
> +
> +function M.luacmd(args)
> + -- Return the full command with flags.
> + return table.concat(args, ' ', executable_idx(args), -1)
> end
>
> local function makeenv(tabenv)
 
--------------NXfPBsjeGMwXcmifD697jC6E--