Tarantool development patches archive
 help / color / mirror / Atom feed
From: Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "Sergey Kaplun" <skaplun@tarantool.org>
Cc: "Maksim Kokryashkin" <max.kokryashkin@gmail.com>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches]  [PATCH luajit v4 4/8] ARM64: Reorder interpreter stack frame and fix unwinding.
Date: Wed, 30 Nov 2022 16:04:30 +0300	[thread overview]
Message-ID: <1669813470.556703695@f155.i.mail.ru> (raw)
In-Reply-To: <Y39Xky/u08/ncaA/@root>

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

  reply	other threads:[~2022-11-30 13:04 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-5-max.kokryashkin@gmail.com>
2022-11-24 11:37   ` Sergey Kaplun via Tarantool-patches
2022-11-30 13:04     ` Maxim Kokryashkin via Tarantool-patches [this message]
2022-12-05 21:42   ` 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-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-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-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=1669813470.556703695@f155.i.mail.ru \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.kokryashkin@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