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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.

> 
> 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>

> 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/

> 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`?

> 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

> +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".

> +local runner = ffi.cast("int (*)(int, int, int, int, int, int, int, int, int)",

Minor: please use single quotes if it's possible.

> +                        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