Re: [Tarantool-patches] [PATCH luajit v4 4/8] ARM64: Reorder interpreter stack frame and fix unwinding.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4388 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 6800 bytes --]