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 E13FF2ACFC1; Tue, 23 May 2023 16:13:18 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E13FF2ACFC1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1684847599; bh=JoqS2s84gxlTzwMb8KRRUvIsnPcLI6VkkR9fHmwSxho=; 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=hWrDvyEDaMjlkijf3RTqAG8Me0u04cnGpUdRSrEn55uiPrSIGneHH792MllRz/nlO 0bVpyGA1AnBnqUKM0Etz67LCpAzl24zwKp3M9Mo0nbSfsG+TXNeS+Nrv9ti5qVhZsg Rnwt8JPi74v6l/UOGOLMeYQudYCvuyt3T/YDBRG4= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (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 158CB2ACFC1 for ; Tue, 23 May 2023 16:13:17 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 158CB2ACFC1 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1q1RpD-0000Hi-UG; Tue, 23 May 2023 16:13:16 +0300 Date: Tue, 23 May 2023 16:09:10 +0300 To: Sergey Bronnikov Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9318AAE2601AA39B8601669E4CE03F6320DD3A1AFA91DFCD400894C459B0CD1B96834FC5A8019957A328ABADD19D3C1A9D8D162C36FA888EB29C25ED172725C63 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75909A206F8DB96D1EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006379E389E29EFC3C6B78638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8EEC59B8C80C175B09139086F5AA37ED8117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC0F49EF363AAD6E82A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735204B6963042765DA4BCB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B613439FA09F3DCB32089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A54B9AEAEB7C3994F3D3008E0CE4A7BE8C898C19ADAC7B6156F87CCE6106E1FC07E67D4AC08A07B9B0DB8A315C1FF4794DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF77DD89D51EBB7742D3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF6C6311296CC403F3C6361FC57D5D9B6159C4CDBD82ECB5B80EFD38DED6A1F022B2CCF8B0FF448A89D5191E2618EE498C492C0AB26861569C129AD8D5ED2C4BD6F4E8A8FB6BF8EBF5 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojzY8FQurh3bCBvZ4nZutRXw== X-DA7885C5: 56A375C61FA49C33725FE0185E3EA9AB96397641A3AAD5CFA6CB8D5738F15BCB262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986BA8F309E764F5F6BC38B067860CED1EE00FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] 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 patch! Please consider my comments below. On 22.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 size causes lld to > show an error message similar to: `ld: error: obj/bytecode.o: string > table non-null terminated`. > > Sergey Bronnikov: > * added the description and the test for the problem > > Signed-off-by: Sergey Bronnikov > --- > Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-366-strtab-section-correct-size > PR: https://github.com/tarantool/tarantool/pull/8678 > --- > src/jit/bcsave.lua | 2 +- > .../lj-366-strtab-correct-size.test.lua | 148 ++++++++++++++++++ > 2 files changed, 149 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 > @@ -275,7 +275,7 @@ typedef struct { > o.sect[2].size = fofs(ofs) > o.sect[3].type = f32(3) -- .strtab > o.sect[3].ofs = fofs(sofs + ofs) > - o.sect[3].size = fofs(#symname+1) > + o.sect[3].size = fofs(#symname+2) > ffi.copy(o.space+ofs+1, symname) > ofs = ofs + #symname + 2 > o.sect[4].type = f32(1) -- .rodata > 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..d4b51537 > --- /dev/null > +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua > @@ -0,0 +1,148 @@ > +local tap = require('tap') > +local test = tap.test('lj-366-strtab-correct-size'):skipcond({ > + -- Test is ELF-specific and because LuaJIT exports object files in ELF format Typo: s/Test/The test/ Typo: s/, and/and/ Nit: Comment line is wider than 66 symbols. Here and below. > + -- for all operating systems except macOS and Windows we skip test on > + -- these OSes. See src/jit/bcsave.lua:bcsave_obj. > + ['Disabled on Windows'] = jit.os == 'Windows', Side note: I'm not sure, that we shall mention Windows, when we still don't support it. Still, this skipcond is necessary, so I'm OK with it. > + ['Disabled on macOS'] = jit.os == 'OSX', > +}) > + > +local ffi = require 'ffi' > + > +-- Reference numbers for strtab offset and size could be obtained with > +-- readelf(1). Note that number system of these number is hexadecimal. Typo: s/number/numbers/ > +-- > +-- $ luajit -b -n "module_name" -e "print()" xxx.obj > +-- $ readelf --section-headers xxx.obj Minor: May you also provide the linker command with the failure mentioned in the issue? Side note: I suppose that the starting \0 byte [1] was forgotten to count. :) > +-- 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 > +-- 0000000000000017 0000000000000000 0 0 1 > +-- ... > + > +local expected_strtab_size = 23 -- == 0x17 Minor: Also, comment about this size (length of `LJBC_PREFIX..ctx.modname` + 2) is desirable. Maybe we should copy this string and calculate the size here as well. Feel free to ignore. > +local expected_strtab_offset = 547 -- == 0x223 Why don't declare as 0x17, 0x223? > + > +-- Defined in elf.h. > +local sht_symtab = 2 Minor: Maybe it is better to use UPPER-CASE for constants? Feel free to ignore. > + > +ffi.cdef[[ > +typedef struct { > + uint8_t emagic[4], eclass, eendian, eversion, eosabi, eabiversion, epad[7]; > + uint16_t type, machine; > + uint32_t version; > + uint32_t entry, phofs, shofs; > + uint32_t flags; > + uint16_t ehsize, phentsize, phnum, shentsize, shnum, shstridx; > +} ELF32header; I suggest to add additional empty line between each declaration for better readability. > +typedef struct { > + uint8_t emagic[4], eclass, eendian, eversion, eosabi, eabiversion, epad[7]; > + uint16_t type, machine; > + uint32_t version; > + uint64_t entry, phofs, shofs; > + uint32_t flags; > + uint16_t ehsize, phentsize, phnum, shentsize, shnum, shstridx; > +} ELF64header; > +typedef struct { > + uint32_t name, type, flags, addr, ofs, size, link, info, align, entsize; > +} ELF32sectheader; > +typedef struct { > + uint32_t name, type; > + uint64_t flags, addr, ofs, size; > + uint32_t link, info; > + uint64_t align, entsize; > +} ELF64sectheader; > +typedef struct { > + uint32_t name, value, size; > + uint8_t info, other; > + uint16_t sectidx; > +} ELF32symbol; I have the following declaration in , maybe we should use the the same declarations ad litteram? Just to avoid inconsistencies in naming? The same for all other structures defined. | typedef struct | { | Elf32_Word st_name; /* Symbol name (string tbl index) */ | Elf32_Addr st_value; /* Symbol value */ | Elf32_Word st_size; /* Symbol size */ | unsigned char st_info; /* Symbol type and binding */ | unsigned char st_other; /* Symbol visibility */ | Elf32_Section st_shndx; /* Section index */ | } Elf32_Sym; Or we may use the same as defined in (as you did), but the comment about it is desirable. > +typedef struct { > + uint32_t name; > + uint8_t info, other; > + uint16_t sectidx; > + uint64_t value, size; > +} ELF64symbol; > +typedef struct { > + ELF32header hdr; > + ELF32sectheader sect[6]; > + ELF32symbol sym[2]; > + uint8_t space[4096]; > +} ELF32obj; > +typedef struct { > + ELF64header hdr; > + ELF64sectheader sect[6]; > + ELF64symbol sym[2]; > + uint8_t space[4096]; > +} ELF64obj; > +]] > + > +local is64_arch = { > + ["x64"] = true, Nit: please, use single-quotes for strings, here and below. > + ["arm64"] = true, > + ["arm64be"] = true, > + ["ppc"] = false, > + ["mips"] = false, Why do we need ppc, and mips definitions here? > +} > + > +local is64 = is64_arch[jit.arch] or false > +local ELFobj_type = ffi.typeof(is64 and "ELF64obj *" or "ELF32obj *") > +local ELFsectheader_type = ffi.typeof(is64 and "ELF64sectheader *" or "ELF32sectheader") Should it be `ELF32sectheader *`? > + > +-- Reads a file located in a specified path and returns its content. > +local function read_file(path) > + local file = assert(io.open(path), 'cannot open an object file') > + local content = file:read("*a") > + file:close() > + return content > +end > + > +-- Parses a buffer in an ELF format and returns an offset and a size of strtab > +-- section for a symtab. > +local function read_elf_strtab(elf_content) > + local elf = ffi.cast(ELFobj_type, elf_content) > + local elf_header = elf.hdr > + local sec_strtab > + for i = 0, elf_header.shnum do > + local sec = ffi.cast(ELFsectheader_type, elf.sect[i]) > + if sec.type == sht_symtab then Why don't just to find .strtab instead? LuaJIT emits really a small obj file, (it's literally the third section). > + sec_strtab = ffi.cast(ELFsectheader_type, elf.sect[sec.link]) > + break > + end > + end > + return sec_strtab.size, sec_strtab.ofs > +end > + > +local function lua_bin_path(arg) Don't get why do we need this function to get luabin? Why just `luacmd` is not enough? > + local args_str = require('utils').luacmd(arg) > + local args = {} > + for a in args_str:gmatch("%S+") do > + table.insert(args, a); > + end > + return args[1] Why do we need to parse the whole string if we return only first occurence of %S+? > +end > + > +test:plan(5) > + > +local elf_filename = os.tmpname() .. '.obj' > +local lua_path = os.getenv('LUA_PATH') > +local lua_bin = lua_bin_path(arg) > +local cmd = ('LUA_PATH="%s" %s -b -n "module_name" -e "print()" %s'):format(lua_path, lua_bin, elf_filename) Nit: Code line is wider than 80 symbols. > +local ret = os.execute(cmd) > +test:ok(ret == 0, 'create an object file') I suggest to use `assert()` for these checks for test correcness. `test:*()` functions -- to check some bugs `assert()` -- to verify test correctness or validity So, we have only 2 test to plan: for size and offset. > + > +local elf_content = read_file(elf_filename) > +test:ok(#elf_content ~= 0, 'read an object file') > +local strtab_size, strtab_offset = read_elf_strtab(elf_content) > +test:ok(strtab_size == expected_strtab_size, 'check .strtab size') > +test:ok(strtab_offset == expected_strtab_offset, 'check .strtab offset') > + > +ret = os.execute(("rm -f %s"):format(elf_filename)) I suppose, that we may use `os.remove()` here. > +test:ok(ret == 0, 'remove an object file') > + > +os.exit(test:check() and 0 or 1) > -- > 2.34.1 > [1]: https://refspecs.linuxbase.org/elf/gabi4+/ch4.strtab.html -- Best regards, Sergey Kaplun