

<HTML><BODY><div>Hi!</div><div>Thanks for the review!</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16692900651911480915_BODY">Hi, Maksim!<br>Thanks for the fixes!<br><br>LGTM, with minor nits below.<br><br>On 28.10.22, Maksim Kokryashkin wrote:<br>> From: Mike Pall <mike><br>><br>> Reported by Yichun Zhang. Fixes #722.<br>> May help towards fixing #698, too.<br>><br>> (cherry picked from commit 421c4c798791d27b7f967df39891c4e4fa1d107c)<br>><br>> The `_Unwind_Find_FDE` fails to find the FDE (frame descriptor<br>> element) for `lj_vm_ffi_call` in DWARF unwind info, despite<br>> the presence of its data in the `.debug_frame` section.<br><br>Strictly saying, for these purposes the `.eh_frame` section is used, as<br>far as unwinder looks for its entries during unwinding. But, yes,<br>`.debug_frame` had incorrect entries, too.</div></div></div></div></blockquote><div>Fixed.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>><br>> LuaJIT emits its own DWARF entries for the CFI (call frame<br>> information, section 6.4.1 in DWARF standard)[1].The FP<br><br>Typo: s<].T><]. T></div></div></div></div></blockquote><div>Fixed.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> register value is vital to perform unwinding, and it is<br>> possible to restore that register using the Canonical<br>> Frame Address, or CFA. It can be obtained as `CFA - offset`.<br>> By default, the CFA register is SP, however, it can be<br>> changed to any other.<br>><br>> According to ARM's calling convention, the first eight<br><br>Minor: s/ARM's/ARM (A64)'s/</div></div></div></div></blockquote><div>Fixed.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> arguments of a function must be passed in x0-x7 registers,<br>> and all the remaining must be passed on the stack. The<br>> latter fact is important because it affects the SP and,<br>> because of that, the CFA invalidates. This patch changes<br>> the CFA register to the FP for the lj_vm_ffi_call, which<br><br>Minor: should it be `lj_vm_ffi_call`?</div></div></div></div></blockquote><div>Fixed.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> fixes the issue.<br>><br>> All the other changes are made just for refactoring purposes.<br>><br>> [1]: <a href="https://dwarfstd.org/doc/DWARF5.pdf" target="_blank">https://dwarfstd.org/doc/DWARF5.pdf</a><br>><br>> Maxim Kokryashkin:<br>> * added the description and the test case for the problem<br>><br>> Needed for tarantool/tarantool#6096<br>> Part of tarantool/tarantool#7230<br>> ---<br>> src/lj_frame.h | 12 +-<br>> src/vm_arm64.dasc | 189 ++++++++++++++----<br>> .../lj-698-arm-pcall-panic.test.lua | 18 ++<br>> 3 files changed, 170 insertions(+), 49 deletions(-)<br>> create mode 100644 test/tarantool-tests/lj-698-arm-pcall-panic.test.lua<br>><br>> diff --git a/src/lj_frame.h b/src/lj_frame.h<br>> index 9fd63fa2..1e4adaa3 100644<br>> --- a/src/lj_frame.h<br>> +++ b/src/lj_frame.h<br><br><snipped><br><br>> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc<br>> index 313cc94f..ad57bca3 100644<br>> --- a/src/vm_arm64.dasc<br>> +++ b/src/vm_arm64.dasc<br><br><snipped><br><br>> diff --git a/test/tarantool-tests/lj-698-arm-pcall-panic.test.lua b/test/tarantool-tests/lj-698-arm-pcall-panic.test.lua<br>> new file mode 100644<br>> index 00000000..88476d3e<br>> --- /dev/null<br>> +++ b/test/tarantool-tests/lj-698-arm-pcall-panic.test.lua<br>> @@ -0,0 +1,18 @@<br>> +local tap = require('tap')<br>> +<br>> +-- See also <a href="https://github.com/LuaJIT/LuaJIT/issues/698" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/698</a>.<br>> +local test = tap.test('lj-418-arm-pcall-panic')<br><br>Typo: s/418/698/<br>Also, it is better to mention (in the test name too) LuaJIT/LuaJIT#722<br>issue (it's already mentioned in the commit message), at least it's<br>given an idea about reproducing:<br><a href="https://github.com/LuaJIT/LuaJIT/issues/722" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/722</a></div></div></div></div></blockquote><div>Fixed.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> +test:plan(1)<br>> +<br>> +local ffi = require('ffi')<br>> +-- The test case below was taken from the LuaJIT-tests<br>> +-- suite (lib/ffi/ffi_callback.lua), and should be removed<br>> +-- after the integration of the mentioned suite.<br><br>Minor: I suppose that you mean "part of the suite".</div></div></div></div></blockquote><div>Fixed.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> +local runner = ffi.cast("int (*)(int, int, int, int, int, int, int, int, int)",<br><br>Minor: please use single quotes if it's possible.</div></div></div></div></blockquote><div>Fixed.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> + function() error("test") end<br>> + )<br><br>Nit: something strange with alignment. Can we just join these lines like<br>the follwing:<br>| local runner = ffi.cast('int (*)(int, int, int, int, int, int, int, int, int)',<br>| function() error('test') end)<br><br>It's good to mention the rationale of the choice this amount of<br>arguments (just copying description from the commit message is enough).<br><br>> +local st = pcall(runner, 1, 1, 1, 1, 1, 1, 1, 1, 1)<br><br>Minor: should we check the error message too?<br>Feel free to ignore.</div></div></div></div></blockquote><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> +test:ok(not st, 'error handling completed correctly')<br>> +<br>> +os.exit(test:check() and 0 or 1)<br>> --<br>> 2.37.0 (Apple Git-136)<br>><br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>