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 27F924B7133; Tue, 6 Jun 2023 17:14:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 27F924B7133 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1686060875; bh=b6FAtIKQYraVFyd+XiL3FZK4GL7dbQ72SesGr0HGzdw=; 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=t3sowBTWr1/e28gjW21yTE2r+HdqXWR5ZMjhh70MEec8SeiSOW/BrWiJAPw4Vv2CD F2bhDfn9I0QphrQgfkV0QdRqlhyqSSB6K+MIPn0+QWCalmy3UjhbLC2B5p4tNR1XHb d6snW5mIwUCQXHv1LECGFDJ6sEOHtMupebAl0A1M= Received: from smtp3.i.mail.ru (smtp3.i.mail.ru [95.163.41.67]) (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 C86934405A1 for ; Tue, 6 Jun 2023 17:14:33 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C86934405A1 Received: by smtp3.i.mail.ru with esmtpa (envelope-from ) id 1q6XSC-00E1wK-Sd; Tue, 06 Jun 2023 17:14:33 +0300 Message-ID: <5250a8cb-bf12-e481-1fe2-a1744fdb6652@tarantool.org> Date: Tue, 6 Jun 2023 17:14:32 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 To: Sergey Kaplun , Sergey Bronnikov References: <7be88fa24695ad2f3c15a9a23bd884ca0acc36f5.1685433737.git.sergeyb@tarantool.org> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9904297982398CD4F188262181E67C032E06F19955FADD9F81313CFAB8367EF908E2BE116634AD74DC919A55A654ADB419C869DB1C19FE1FEC2D51D74DCAD52D60422B37ABB051B9D X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7DB6A86BDF2D5A895EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006376DFCC587F0A0398D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D859EC15FCE3984C5D8C601D2A7D2E4BBC117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC47272755C61AA17BA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735200AC5B80A05675ACD6FD1C55BDD38FC3FD2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE91ADC097FE2C3A08C67AC315686ED4D3D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3D2AC72D04CD5349B6136E347CC761E07C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947C989FD0BDF65E50FB2E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F83C798A30B85E16B42539A7722CA490CB5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5488D07565A42B383862BE0C0FE48B7A95BD923491A7CF8A8F87CCE6106E1FC07E67D4AC08A07B9B01DAA61796BF5227BCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D3454548929AF40B4288BE5EE22C23FA2D91614C27C6DEEE09668D1CF0D30BB108C65101849A1B7B03C1D7E09C32AA3244CAB3AB3B2F3F932ED1F200C91150F34DAD9ADFF0C0BDB8D1FBAD658CF5C8AB4025DA084F8E80FEBD3FFA33E6B6B2F82C47A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojBFnFgsoZgk49kVBtGkWGPg== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769BB3FB9F24BC5D99E141C5A4D0D33C6CFC9F871340C5A6C40EBA65886582A37BD66FEC6BF5C9C28D98A98C1125256619760D574B6FC815AB872D6B4FCE48DF648AE208404248635DF 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! All comments fixed and force-pushed. Thanks! On 6/6/23 15:31, Sergey Kaplun wrote: > 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 :). Fixed. > >> 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. Added. >> 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.) Fixed. > >> +-- >> +-- $ 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/ Fixed. > > +-- 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 Good suggestion, thanks! (updated as you suggest) > >> +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. Removed "or false". > >> + >> +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 >