

<HTML><BODY><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 <sergeyb@tarantool.org>:<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></BODY></HTML>