Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maksim Kokryashkin <max.kokryashkin@gmail.com>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit v4 4/8] ARM64: Reorder interpreter stack frame and fix unwinding.
Date: Thu, 24 Nov 2022 14:37:55 +0300	[thread overview]
Message-ID: <Y39Xky/u08/ncaA/@root> (raw)
In-Reply-To: <20221028092638.11506-5-max.kokryashkin@gmail.com>

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

  parent reply	other threads:[~2022-11-24 11:41 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20221028092638.11506-1-max.kokryashkin@gmail.com>
     [not found] ` <20221028092638.11506-8-max.kokryashkin@gmail.com>
2022-11-24 11:56   ` [Tarantool-patches] [PATCH luajit v4 7/8] Fix build with busybox grep Sergey Kaplun via Tarantool-patches
2022-11-30 13:06     ` Maxim Kokryashkin via Tarantool-patches
2022-12-05 21:51   ` sergos via Tarantool-patches
     [not found] ` <20221028092638.11506-2-max.kokryashkin@gmail.com>
2022-12-05 16:01   ` [Tarantool-patches] [PATCH luajit v4 1/8] Cleanup and enable external unwinding for more platforms sergos via Tarantool-patches
     [not found] ` <20221028092638.11506-3-max.kokryashkin@gmail.com>
2022-12-05 16:06   ` [Tarantool-patches] [PATCH luajit v4 2/8] OSX: Fix build by hardcoding external frame unwinding sergos via Tarantool-patches
     [not found] ` <20221028092638.11506-4-max.kokryashkin@gmail.com>
2022-12-05 16:11   ` [Tarantool-patches] [PATCH luajit v4 3/8] OSX/ARM64: Disable external unwinding for now sergos via Tarantool-patches
     [not found] ` <20221028092638.11506-5-max.kokryashkin@gmail.com>
2022-11-24 11:37   ` Sergey Kaplun via Tarantool-patches [this message]
2022-11-30 13:04     ` [Tarantool-patches] [PATCH luajit v4 4/8] ARM64: Reorder interpreter stack frame and fix unwinding Maxim Kokryashkin via Tarantool-patches
2022-12-05 21:42   ` sergos via Tarantool-patches
     [not found] ` <20221028092638.11506-6-max.kokryashkin@gmail.com>
2022-11-24 11:41   ` [Tarantool-patches] [PATCH luajit v4 5/8] OSX/ARM64: Disable unwind info Sergey Kaplun via Tarantool-patches
2022-11-30 13:05     ` Maxim Kokryashkin via Tarantool-patches
2022-12-05 21:43   ` sergos via Tarantool-patches
     [not found] ` <20221028092638.11506-7-max.kokryashkin@gmail.com>
2022-11-24 11:49   ` [Tarantool-patches] [PATCH luajit v4 6/8] BSD: Fix build with BSD grep Sergey Kaplun via Tarantool-patches
2022-11-30 13:05     ` Maxim Kokryashkin via Tarantool-patches
2022-12-05 21:46   ` sergos via Tarantool-patches
     [not found] ` <20221028092638.11506-9-max.kokryashkin@gmail.com>
2022-11-24 13:10   ` [Tarantool-patches] [PATCH luajit v4 8/8] OSX/ARM64: Fix external unwinding Sergey Kaplun via Tarantool-patches
2022-11-30 13:21     ` Maxim Kokryashkin via Tarantool-patches
2022-12-01  8:52       ` Sergey Kaplun via Tarantool-patches
2022-12-01 12:28         ` Sergey Kaplun via Tarantool-patches
2022-12-06  5:58   ` sergos via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y39Xky/u08/ncaA/@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit v4 4/8] ARM64: Reorder interpreter stack frame and fix unwinding.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox