From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 52A891BE99F; Thu, 24 Nov 2022 14:41:07 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 52A891BE99F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1669290067; bh=wzwu3nYmIXYG7LiQbJJTN3/DdyadGiWOr2DvVjF3UfE=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=pyAxuK8up2XkRyoKwc7coktwf2h30yKOfxft7HK3QDb66x1UoL3wenvzKsMeFDzA1 nRJJJIhyOtLDR2VUBXul7cnYV2MsBosTszCRmD5m3e2uKWdyKw7hKlP2C29hkvxElj Sjpk3I1CVrDfuc69dZqtHROhy4iVFnEtAu6Hqibc= Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id ECD9E7B11C for ; Thu, 24 Nov 2022 14:41:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org ECD9E7B11C Received: by smtp56.i.mail.ru with esmtpa (envelope-from ) id 1oyAbJ-00082v-0H; Thu, 24 Nov 2022 14:41:05 +0300 Date: Thu, 24 Nov 2022 14:37:55 +0300 To: Maksim Kokryashkin Message-ID: References: <20221028092638.11506-1-max.kokryashkin@gmail.com> <20221028092638.11506-5-max.kokryashkin@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221028092638.11506-5-max.kokryashkin@gmail.com> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D4445358DD2C13E202F079B587147DF30515B7F7FF62DB98182A05F53808504071182DE7C108A64CE4709A10B6FE99D6BF9CE1B85147BB544B27EA0EFA14B1F6 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE789066434B85BF7C7EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374ECA27954A00B6C3EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B8859CA687ABA27BA9094C8A51DEDB41476B548AF1F062DC8CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F924B32C592EA89F389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC81E1736EDDE8D4790F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CD2B897A0B7B208E103F1AB874ED890284AD6D5ED66289B52698AB9A7B718F8C46E0066C2D8992A16725E5C173C3A84C3D8561A2FEC730ACEBA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CFCF36E64A7E3F8E581DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C39D7D3120FB43BDE335872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34FF70CCB59A6ED8702A4181473B352A64A6BA4156AAEA96F82E8F36856659ACB3B2966DEA383555741D7E09C32AA3244C3407B34390D0BD0166CE7D416E3D21003A92A9747B6CC886927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojZo+qSBrEvGTbP+EEYYRIYw== X-Mailru-Sender: F6248FDC0389C51188CF133EE5DC084F471DDC6E451340341088FB14CC8A68ACB7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v4 4/8] ARM64: Reorder interpreter stack frame and fix unwinding. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maksim! Thanks for the fixes! LGTM, with minor nits below. On 28.10.22, Maksim Kokryashkin wrote: > From: Mike Pall > > 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 > 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 > 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