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 8DB0F4405A1; Tue, 6 Jun 2023 15:36:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8DB0F4405A1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1686054969; bh=0f3ZGLUaSpcQ7tJowZ+EvKYFuWuo2dREM1xsIntvSoQ=; 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=ce9slI5ZQaKLYvzJ1DmgNA5mCZIDvthGlF8hmJdfkePEy88AfiqDznFFaQR74mAix licNdSmCjPujlFT3r8P9PDW7TeM5uH86enA7SejYjYMBfkWkC7QCP0JV18Y8iWMvN7 nXw7vUN34LJSNuFf/RZPpZTpdIe7QPi5lcbHBfoU= Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [95.163.41.73]) (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 DB5B24405A1 for ; Tue, 6 Jun 2023 15:36:07 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DB5B24405A1 Received: by smtp32.i.mail.ru with esmtpa (envelope-from ) id 1q6Vuw-001GHY-TB; Tue, 06 Jun 2023 15:36:07 +0300 Date: Tue, 6 Jun 2023 15:31:57 +0300 To: Sergey Bronnikov Message-ID: References: <7be88fa24695ad2f3c15a9a23bd884ca0acc36f5.1685433737.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7be88fa24695ad2f3c15a9a23bd884ca0acc36f5.1685433737.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD93D74B10BAB639DE3EAF656F193F0678BFC1635858165C9A800894C459B0CD1B99DD736AD84D049FA6D384AC538EE4F089F842A013E2B3348512C6526D8CF0657 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7DB6A86BDF2D5A895EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374B6C65B7367884A58638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8F8D42AC4AD94C52587084973DB14B32E117882F44604297287769387670735200AC5B80A05675ACDF04B652EEC242312D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE042285CD7A5C321FB78CF848AE20165DD8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE30085B890FD2717DA2D242C3BD2E3F4C6C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CC6602A96AF88C6952E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F83C798A30B85E16B5E1C53F199C2BB95B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5FE5E8FB4DC19DA926E301F0BC1FE5569E63AC926411A53ECF87CCE6106E1FC07E67D4AC08A07B9B0DB8A315C1FF4794DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34B7CBFF60649FF266ADC83188D2732495C432D6FBCE5C90437835480C0317072B12D8980E9D95A2C71D7E09C32AA3244C3289B7B1DD0D07405767EF0EB029D827795D98D676DD64D0BAD658CF5C8AB4025DA084F8E80FEBD3202CD0F03380D9577A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojBFnFgsoZgk6s1eNp3uj9vA== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769EDFF8521FBDC87119B171B15DE5D10EC84876E3DC5E244DFDEDBA653FF35249392D99EB8CC7091A70E183A470755BFD208F19895AA18418972D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v2 1/1] Fix saved bytecode encapsulated in ELF objects. 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, Sergey! Thanks for the fixes! I've reviewed the branch version, since changes was made after Max's review. LGTM, with minor nits below. On 30.05.23, Sergey Bronnikov wrote: > From: Sergey Bronnikov > > Thanks to Dimitry Andric. > > (cherry picked from commit 7dbf0b05f1228c1c719866db5e5f3d58f87f74c8) > > Function `bcsave.lua:bcsave_elfobj()` produced an object file in ELF > format with a wrong size size of `.strtab`. Wrong .strtab size causes lld to Looks like this paragraph is too wide for git commit format. You may replace it with the following* : | Function `bcsave.lua:bcsave_elfobj()` produced an object file in ELF | format with a wrong size size of `.strtab`. Wrong .strtab size causes | lld to show an error message: *: I got this via the `V}gq` in vim -- it may be used for formatting according to permitted line width :). > show an error message: > > ``` > $ luajit -b -n "module_name" -e "print()" xxx.obj > $ ld.lld xxx.obj > ld.lld: error: xxx.obj: SHT_STRTAB string table section [index 3] is non-null terminated > ``` Minor: please, comment that the starting \0 byte [1] was forgotten to count. And the patch fixes the counting. > > Sergey Bronnikov: > * added the description and the test for the problem > > Signed-off-by: Sergey Bronnikov > Reviewed-by: Sergey Kaplun > Reviewed-by: Maxim Kokryashkin > --- > > Changes in v2: > - Fixed comments as per review by Sergey. > > Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-366-strtab-section-correct-size > Tarantool CI: https://github.com/ligurio/tarantool/tree/ligurio/gh-xxxx-strtab-section-correct-size Side note: CI results are available here: https://github.com/tarantool/tarantool/pull/8678 > > src/jit/bcsave.lua | 2 +- > .../lj-366-strtab-correct-size.test.lua | 202 ++++++++++++++++++ > 2 files changed, 203 insertions(+), 1 deletion(-) > create mode 100644 test/tarantool-tests/lj-366-strtab-correct-size.test.lua > > diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua > index c17c88e0..2553d97e 100644 > --- a/src/jit/bcsave.lua > +++ b/src/jit/bcsave.lua > diff --git a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua > new file mode 100644 > index 00000000..5dec6751 > --- /dev/null > +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua > @@ -0,0 +1,202 @@ > +-- with name 'luaJIT_BC_lango_team': Nit: Comment width is more than 66 symbols, here and below. (Obviously there is no need to change the output of utilities, but only comments themselves.) > +-- > +-- $ readelf --symbols xxx.obj > +-- > +-- Symbol table '.symtab' contains 2 entries: > +-- Num: Value Size Type Bind Vis Ndx Name > +-- 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND > +-- 1: 0000000000000000 66 OBJECT GLOBAL DEFAULT 4 luaJIT_BC_lango_team > +-- > +-- and displaying the information contained in the file's section headers, if > +-- it has any. For our purposes we are interesting in section .symtab, > +-- so other sections snipped in the output: According the latest version on the branch: =================================================================== +-- and to dsisplay the information contained in the file's section headers, if Typo: s/dsisplay/display/ +-- it has any. For our purposes we are interesting in .symtab section, Typo: s/For out purposes/For out purposes,/ Typo: s/we are interesting/we are interested/ +-- so other sections are snipped in the output: +-- +-- $ readelf --section-headers xxx.obj +-- There are 6 section headers, starting at offset 0x40: +-- =================================================================== > +-- > +-- $ readelf --section-headers xxx.obj > +-- There are 6 section headers, starting at offset 0x40: > +-- > +-- Section Headers: > +-- [Nr] Name Type Address Offset > +-- Size EntSize Flags Link Info Align > +-- ... > +-- > +-- [ 3] .strtab STRTAB 0000000000000000 00000223 > +-- 0000000000000016 0000000000000000 0 0 1 > +-- ... > +-- Reference numbers for strtab offset and size could be obtained with > +-- readelf(1). Note that number system of these numbers are hexadecimal. Typo: s/number system ... are/number system ... is/ > + > +local expected_strtab_size = 0x16 Minor: I suggest to make it more specific to be clearer to understand what is that magic number: | local SYM_NAME_EXPECTED = LJBC_PREFIX .. MODULE_NAME | -- The first \0 byte, which is index zero + length of the string | -- + terminating \0 byte = 0x16. | local EXPECTED_STRTAB_SIZE = 1 + #SYM_NAME_EXPECTED + 1 > +local expected_strtab_offset = 0x223 > +local module_name = 'lango_team' > + > +-- Symbol name prefix for LuaJIT bytecode defined in bcsave.lua. > +local LJBC_PREFIX = 'luaJIT_BC_' > + > +-- Defined in elf.h. > +local SHT_SYMTAB = 2 > + > +-- Using the same declarations as defined in . > +ffi.cdef[[ > +]] > + > +local is64_arch = { > + ['x64'] = true, > + ['arm64'] = true, > + ['arm64be'] = true, > + ['ppc'] = false, > + ['mips'] = false, > +} | I would left unsupported arch's here as well, it will simplify a bit work | for those who will port test to these arch's in a future. Minor: I'm not against this approach at all. I'm a little bit confused that we use `or false` for initializing `is64` variable below -- I suppose, it can be skipped, so we'll get `true` for well-known and supported arches (x64, arm64, arm64be), `false` for supported non-64-bit arches (ppc, mips) and `nil` for unknown arches. > + > +local is64 = is64_arch[jit.arch] or false I suggest to replace with | local is64 = is64_arch[jit.arch] Feel free to ignore. See comment above. > + > +local function create_obj_file(name) > -- > 2.34.1 > [1]: https://refspecs.linuxbase.org/elf/gabi4+/ch4.strtab.html -- Best regards, Sergey Kaplun