<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi, Max</p>
<p><br>
</p>
<p>the problem is reproduced, thanks!</p>
<p>LGTM</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 12/6/23 18:07, Maxim Kokryashkin
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1701875230.476933162@f762.i.mail.ru">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div>Hi, Sergey!</div>
<div>Thanks for the review!</div>
<div>The test case indeed fails only for the GC64 mode before the
patch.</div>
<div>I've tried to make it fail for non-GC64 mode, but the same
issue arised —</div>
<div>an arbitrary amount of slots needed, and that amount needs to
be modified</div>
<div>each time there is a minor change in the Lua stack behavior.
With that in</div>
<div>mind, I’ve decided to leave it as it is. Added a comment and
modified the</div>
<div>commit message. The new commit message:</div>
<div>===</div>
<div>
<div>
<div>Improve error reporting on stack overflow.</div>
</div>
<div> </div>
<div>
<div>Thanks to Nicolas Lebedenco.</div>
</div>
<div> </div>
<div>
<div>(cherry-picked from commit
8135de2a0204e6acd92b231131c4a1e0be03ac1c)</div>
</div>
<div> </div>
<div>
<div>The `lj_state_growstack` doesn't account for a potential
error</div>
<div>handler invocation by `xpcall`, which may lead to the
second</div>
<div>error while handling a stack overflow, resulting in a
misleading</div>
<div>error "error in error handling", while the real issue is
a stack</div>
<div>overflow. This patch addresses this issue by fixing the
condition</div>
<div>at which stack overflow errors are thrown. Now it's
thrown if the</div>
<div>stack size is at least at the limit, instead of when it
is over</div>
<div>the limit.</div>
</div>
<div> </div>
<div>
<div>The test case is very fragile, and the issue is hard to
reproduce</div>
<div>in a robust way. The provided reproducer failed before
the patch</div>
<div>for GC64 mode only. Non-GC64 mode requires an arbitrary
number</div>
<div>of stack slots to be filled, which is even more fragile.</div>
</div>
<div> </div>
<div>
<div>This commit also disables the second test from</div>
<div>`lj-603-err-snap-restore`, since after this patch and the
two</div>
<div>follow-ups for it, there is no such amount of stack slots
with</div>
<div>which the test works the way it should.</div>
</div>
<div> </div>
<div>
<div>Lastly, this patch adds an alternative to `luacmd` to the</div>
<div>`utils.exec` module, which is called `luabin`. That
function is</div>
<div>similar to the `luacmd`, with the only difference of
returning</div>
<div>only the executable itself without any additional CLI
options</div>
<div>passed.</div>
</div>
<div> </div>
<div>
<div>Maxim Kokryashkin:</div>
<div>* added the description and the test for the problem</div>
</div>
<div> </div>
<div>
<div>Part of tarantool/tarantool#9145</div>
<div>===</div>
<div> </div>
<div>And the diff for the comment:</div>
<div>===</div>
<div>
<div>
<div>diff --git
a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua</div>
<div>index 45a888f4..e5f9d9a4 100644</div>
<div>---
a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua</div>
<div>+++
b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua</div>
<div>@@ -3,6 +3,12 @@ local tap = require('tap')</div>
<div> local test =
tap.test('lj-962-stack-overflow-report')</div>
<div> test:plan(1)</div>
</div>
<div> </div>
<div>
<div>+-- XXX: The test case is very fragile, and the issue
is hard to</div>
<div>+-- reproduce in a robust way. The provided
reproducer failed</div>
<div>+-- before the patch for GC64 mode only. Non-GC64
mode requires</div>
<div>+-- an arbitrary number of stack slots to be filled,
which is</div>
<div>+-- even more fragile.</div>
<div>+</div>
<div> local LUABIN = require('utils').exec.luabin(arg)</div>
<div> local SCRIPT = arg[0]:gsub('%.test%.lua$',
'/script.lua')</div>
<div> local output = io.popen(LUABIN .. ' 2>&1 ' ..
SCRIPT, 'r'):read('*all')</div>
<div> </div>
</div>
</div>
<div>===</div>
</div>
</div>
<div data-signature-widget="container">
<div data-signature-widget="content">
<div>--<br>
Best regards,</div>
<div>Maxim Kokryashkin</div>
</div>
</div>
<div> </div>
<div> </div>
<blockquote
style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Пятница,
24 ноября 2023, 15:22 +03:00 от Sergey Bronnikov
<a class="moz-txt-link-rfc2396E" href="mailto:sergeyb@tarantool.org"><sergeyb@tarantool.org></a>:<br>
<div id="">
<div class="js-helper js-readmsg-msg">
<div>
<div id="style_17008285690419145462_BODY">Hello, Max<br>
<br>
thanks for the patch!<br>
<br>
proposed test is passed after reverting a fix<br>
<br>
<br>
On 11/22/23 17:35, Maksim Kokryashkin wrote:
<div class="mail-quote-collapse">> From: Mike Pall
<mike><br>
><br>
> Thanks to Nicolas Lebedenco.<br>
><br>
> (cherry-picked from commit
8135de2a0204e6acd92b231131c4a1e0be03ac1c)<br>
><br>
> The `lj_state_growstack` doesn't account for a
potential error<br>
> handler invocation by `xpcall`, which may lead to
the second<br>
> error while handling a stack overflow, resulting
in a misleading<br>
> error "error in error handling", while the real
issue is a stack<br>
> overflow. This patch addresses this issue by
fixing the condition<br>
> at which stack overflow errors are thrown. Now
it's thrown if the<br>
> stack size is at least at the limit, instead of
when it is over<br>
> the limit.<br>
><br>
> This commit also disables the second test from<br>
> `lj-603-err-snap-restore`, since after this patch
and the two<br>
> follow-ups for it, there is no such amount of
stack slots with<br>
> which the test works the way it should.<br>
><br>
> Lastly, this patch adds an alternative to
`luacmd` to the<br>
> `utils.exec` module, which is called `luabin`.
That function is<br>
> similar to the `luacmd`, with the only difference
of returning<br>
> only the executable itself without any additional
CLI options<br>
> passed.<br>
><br>
> Maxim Kokryashkin:<br>
> * added the description and the test for the
problem<br>
><br>
> Part of tarantool/tarantool#9145<br>
> ---<br>
> src/lj_state.c | 2 +-<br>
> .../lj-603-err-snap-restore.test.lua | 1 +<br>
> .../lj-962-stack-overflow-report.test.lua | 10
++++++++++<br>
> .../lj-962-stack-overflow-report/script.lua | 3
+++<br>
> test/tarantool-tests/utils/exec.lua | 15
++++++++++++---<br>
> 5 files changed, 27 insertions(+), 4 deletions(-)<br>
> create mode 100644
test/tarantool-tests/lj-962-stack-overflow-report.test.lua<br>
> create mode 100644
test/tarantool-tests/lj-962-stack-overflow-report/script.lua<br>
><br>
> diff --git a/src/lj_state.c b/src/lj_state.c<br>
> index 684336d5..76153bad 100644<br>
> --- a/src/lj_state.c<br>
> +++ b/src/lj_state.c<br>
> @@ -132,7 +132,7 @@ void LJ_FASTCALL
lj_state_growstack(lua_State *L, MSize need)<br>
> n = LJ_STACK_MAX;<br>
> }<br>
> resizestack(L, n);<br>
> - if (L->stacksize > LJ_STACK_MAXEX)<br>
> + if (L->stacksize >= LJ_STACK_MAXEX)<br>
> lj_err_msg(L, LJ_ERR_STKOV);<br>
> }<br>
><br>
> diff --git
a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
b/test/tarantool-tests/lj-603-err-snap-restore.test.lua<br>
> index 96ebf92c..f5c8474f 100644<br>
> ---
a/test/tarantool-tests/lj-603-err-snap-restore.test.lua<br>
> +++
b/test/tarantool-tests/lj-603-err-snap-restore.test.lua<br>
> @@ -36,6 +36,7 @@ local function do_test()<br>
> -- Tarantool at start, so just skip test for it.<br>
> -- luacheck: no global<br>
> ['Disable test for Tarantool'] = _TARANTOOL,<br>
> + ['Stack overflow is now handled differently'] =
true,<br>
> })<br>
><br>
> test:ok(not handler_is_called)<br>
> diff --git
a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua<br>
> new file mode 100644<br>
> index 00000000..45a888f4<br>
> --- /dev/null<br>
> +++
b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua<br>
> @@ -0,0 +1,10 @@<br>
> +local tap = require('tap')<br>
> +-- The test reproduces the problem only for GC64
mode with enabled JIT.<br>
> +local test =
tap.test('lj-962-stack-overflow-report')<br>
> +test:plan(1)<br>
> +<br>
> +local LUABIN = require('utils').exec.luabin(arg)<br>
> +local SCRIPT = arg[0]:gsub('%.test%.lua$',
'/script.lua')<br>
> +local output = io.popen(LUABIN .. ' 2>&1
' .. SCRIPT, 'r'):read('*all')<br>
> +test:like(output, 'stack overflow', 'stack
overflow reported correctly')<br>
> +test:done(true)<br>
> diff --git
a/test/tarantool-tests/lj-962-stack-overflow-report/script.lua
b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua<br>
> new file mode 100644<br>
> index 00000000..31c5ca33<br>
> --- /dev/null<br>
> +++
b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua<br>
> @@ -0,0 +1,3 @@<br>
> +-- XXX: Function `f` is global to avoid using an
additional stack slot.<br>
> +-- luacheck: no global<br>
> +f = function() f() end; f()<br>
> diff --git a/test/tarantool-tests/utils/exec.lua
b/test/tarantool-tests/utils/exec.lua<br>
> index a56ca2dc..48a08ba5 100644<br>
> --- a/test/tarantool-tests/utils/exec.lua<br>
> +++ b/test/tarantool-tests/utils/exec.lua<br>
> @@ -1,14 +1,23 @@<br>
> local M = {}<br>
><br>
> -function M.luacmd(args)<br>
> +local function executable_idx(args)<br>
> -- arg[-1] is guaranteed to be not nil.<br>
> local idx = -2<br>
> while args[idx] do<br>
> assert(type(args[idx]) == 'string', 'Command part
have to be a string')<br>
> idx = idx - 1<br>
> end<br>
> - -- return the full command with flags.<br>
> - return table.concat(args, ' ', idx + 1, -1)<br>
> + return idx + 1<br>
> +end<br>
> +<br>
> +function M.luabin(args)<br>
> + -- Return only the executable.<br>
> + return args[executable_idx(args)]<br>
> +end<br>
> +<br>
> +function M.luacmd(args)<br>
> + -- Return the full command with flags.<br>
> + return table.concat(args, ' ',
executable_idx(args), -1)<br>
> end<br>
><br>
> local function makeenv(tabenv)</div>
</div>
</div>
</div>
</div>
</blockquote>
<div> </div>
</blockquote>
</body>
</html>