<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>LGTM, except for a few comments below.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16854341921485736637_BODY">From: Sergey Bronnikov <<a href="/compose?To=sergeyb@tarantool.org">sergeyb@tarantool.org</a>><br><br>Thanks to Dimitry Andric.<br><br>(cherry picked from commit 7dbf0b05f1228c1c719866db5e5f3d58f87f74c8)<br><br>Function `bcsave.lua:bcsave_elfobj()` produced an object file in ELF<br>format with a wrong size size of `.strtab`. Wrong .strtab size causes lld to<br>show an error message:<br><br>```<br>$ luajit -b -n "module_name" -e "print()" xxx.obj<br>$ ld.lld xxx.obj<br>ld.lld: error: xxx.obj: SHT_STRTAB string table section [index 3] is non-null terminated<br>```<br><br>Sergey Bronnikov:<br>* added the description and the test for the problem<br><br>Signed-off-by: Sergey Bronnikov <<a href="/compose?To=sergeyb@tarantool.org">sergeyb@tarantool.org</a>><br>Reviewed-by: Sergey Kaplun <<a href="/compose?To=skaplun@tarantool.org">skaplun@tarantool.org</a>><br>Reviewed-by: Maxim Kokryashkin <<a href="/compose?To=m.kokryashkin@tarantool.org">m.kokryashkin@tarantool.org</a>><br>---<br><br>Changes in v2:<br>- Fixed comments as per review by Sergey.<br><br>Branch: <a href="https://github.com/tarantool/luajit/tree/ligurio/gh-366-strtab-section-correct-size" target="_blank">https://github.com/tarantool/luajit/tree/ligurio/gh-366-strtab-section-correct-size</a><br>Tarantool CI: <a href="https://github.com/ligurio/tarantool/tree/ligurio/gh-xxxx-strtab-section-correct-size" target="_blank">https://github.com/ligurio/tarantool/tree/ligurio/gh-xxxx-strtab-section-correct-size</a><br><br> src/jit/bcsave.lua | 2 +-<br> .../lj-366-strtab-correct-size.test.lua | 202 ++++++++++++++++++<br> 2 files changed, 203 insertions(+), 1 deletion(-)<br> create mode 100644 test/tarantool-tests/lj-366-strtab-correct-size.test.lua<br><br>diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua<br>index c17c88e0..2553d97e 100644<br>--- a/src/jit/bcsave.lua<br>+++ b/src/jit/bcsave.lua<br>@@ -275,7 +275,7 @@ typedef struct {<br> o.sect[2].size = fofs(ofs)<br> o.sect[3].type = f32(3) -- .strtab<br> o.sect[3].ofs = fofs(sofs + ofs)<br>- o.sect[3].size = fofs(#symname+1)<br>+ o.sect[3].size = fofs(#symname+2)<br> ffi.copy(o.space+ofs+1, symname)<br> ofs = ofs + #symname + 2<br> o.sect[4].type = f32(1) -- .rodata<br>diff --git a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua<br>new file mode 100644<br>index 00000000..5dec6751<br>--- /dev/null<br>+++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua<br>@@ -0,0 +1,202 @@<br>+local tap = require('tap')<br>+local test = tap.test('lj-366-strtab-correct-size'):skipcond({<br>+ -- The test is ELF-specific, and because LuaJIT exports object<br>+ -- files in ELF format for all operating systems except macOS<br>+ -- and Windows we skip test on these OSes.</div></div></div></div></blockquote><div>Typo: s/we skip test/we skip it/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+ -- See src/jit/bcsave.lua:bcsave_obj.<br>+ ['Disabled on Windows'] = jit.os == 'Windows',<br>+ ['Disabled on macOS'] = jit.os == 'OSX',<br>+})<br>+<br>+local ffi = require 'ffi'<br>+<br>+-- Command below exports bytecode as an object file in ELF format:<br>+-- $ luajit -b -n 'lango_team' -e 'print()' xxx.obj<br>+-- $ file xxx.obj<br>+-- xxx.obj: ELF 64-bit LSB relocatable, x86-64, version 1 (SYSV), not stripped<br>+--<br>+-- With read_elf(1) it is possible displaying the entries in symbol table</div></div></div></div></blockquote><div>Typo: s/possible displaying the entries/possible to display entries/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+-- section of the file, if it has one. Object file contains a single symbol</div></div></div></div></blockquote><div>Typo: s/a single symbol/the single symbol/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+-- with name 'luaJIT_BC_lango_team':<br>+--<br>+-- $ readelf --symbols xxx.obj<br>+--<br>+-- Symbol table '.symtab' contains 2 entries:<br>+-- Num: Value Size Type Bind Vis Ndx Name<br>+-- 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND<br>+-- 1: 0000000000000000 66 OBJECT GLOBAL DEFAULT 4 luaJIT_BC_lango_team<br>+--<br>+-- and displaying the information contained in the file's section headers, if</div></div></div></div></blockquote><div>Typo: s/and to display/</div><div> </div><div>Also, that sentence seems to be a continuation for the previous sentence about</div><div>`readelf`’s functionality, however it is completely detached from it both by the</div><div>setence inbetween them and grammatically. I suggest moving the demonstration</div><div>of `.symtab` contents after the introduction of readelf’s functionality, to improve</div><div>readability.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+-- it has any. For our purposes we are interesting in section .symtab,</div></div></div></div></blockquote><div>Typo: s/are interesting in section .symtab/are interested in .symtab section/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+-- so other sections snipped in the output:</div></div></div></div></blockquote><div>Typo: s/sections snipped/sections are snipped/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+--<br>+-- $ readelf --section-headers xxx.obj<br>+-- There are 6 section headers, starting at offset 0x40:<br>+--<br>+-- Section Headers:<br>+-- [Nr] Name Type Address Offset<br>+-- Size EntSize Flags Link Info Align<br>+-- ...<br>+--<br>+-- [ 3] .strtab STRTAB 0000000000000000 00000223<br>+-- 0000000000000016 0000000000000000 0 0 1<br>+-- ...<br>+-- Reference numbers for strtab offset and size could be obtained with<br>+-- readelf(1). Note that number system of these numbers are hexadecimal.<br>+<br>+local expected_strtab_size = 0x16<br>+local expected_strtab_offset = 0x223<br>+local module_name = 'lango_team'</div></div></div></div></blockquote><div>Simple value constants should be uppercased.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+<br>+-- Symbol name prefix for LuaJIT bytecode defined in bcsave.lua.<br>+local LJBC_PREFIX = 'luaJIT_BC_'<br>+<br>+-- Defined in elf.h.<br>+local SHT_SYMTAB = 2<br>+<br>+-- Using the same declarations as defined in <src/jit/bcsave.lua>.<br>+ffi.cdef[[<br>+typedef struct {<br>+ uint8_t emagic[4], eclass, eendian, eversion, eosabi, eabiversion, epad[7];<br>+ uint16_t type, machine;<br>+ uint32_t version;<br>+ uint32_t entry, phofs, shofs;<br>+ uint32_t flags;<br>+ uint16_t ehsize, phentsize, phnum, shentsize, shnum, shstridx;<br>+} ELF32header;<br>+<br>+typedef struct {<br>+ uint8_t emagic[4], eclass, eendian, eversion, eosabi, eabiversion, epad[7];<br>+ uint16_t type, machine;<br>+ uint32_t version;<br>+ uint64_t entry, phofs, shofs;<br>+ uint32_t flags;<br>+ uint16_t ehsize, phentsize, phnum, shentsize, shnum, shstridx;<br>+} ELF64header;<br>+<br>+typedef struct {<br>+ uint32_t name, type, flags, addr, ofs, size, link, info, align, entsize;<br>+} ELF32sectheader;<br>+<br>+typedef struct {<br>+ uint32_t name, type;<br>+ uint64_t flags, addr, ofs, size;<br>+ uint32_t link, info;<br>+ uint64_t align, entsize;<br>+} ELF64sectheader;<br>+<br>+typedef struct {<br>+ uint32_t name, value, size;<br>+ uint8_t info, other;<br>+ uint16_t sectidx;<br>+} ELF32symbol;<br>+<br>+typedef struct {<br>+ uint32_t name;<br>+ uint8_t info, other;<br>+ uint16_t sectidx;<br>+ uint64_t value, size;<br>+} ELF64symbol;<br>+<br>+typedef struct {<br>+ ELF32header hdr;<br>+ ELF32sectheader sect[6];<br>+ ELF32symbol sym[2];<br>+ uint8_t space[4096];<br>+} ELF32obj;<br>+<br>+typedef struct {<br>+ ELF64header hdr;<br>+ ELF64sectheader sect[6];<br>+ ELF64symbol sym[2];<br>+ uint8_t space[4096];<br>+} ELF64obj;<br>+]]<br>+<br>+local is64_arch = {<br>+ ['x64'] = true,<br>+ ['arm64'] = true,<br>+ ['arm64be'] = true,<br>+ ['ppc'] = false,<br>+ ['mips'] = false,<br>+}<br>+<br>+local is64 = is64_arch[jit.arch] or false<br>+<br>+local function create_obj_file(name)<br>+ local elf_filename = os.tmpname() .. '.obj'<br>+ local lua_path = os.getenv('LUA_PATH')<br>+ local lua_bin = require('utils').luacmd(arg):match('%S+')<br>+ local cmd_fmt = 'LUA_PATH="%s" %s -b -n "%s" -e "print()" %s'<br>+ local cmd = (cmd_fmt):format(lua_path, lua_bin, name, elf_filename)<br>+ local ret = os.execute(cmd)<br>+ assert(ret == 0, 'create an object file')<br>+ return elf_filename<br>+end<br>+<br>+-- Reads a file located in a specified path and returns its content.<br>+local function read_file(path)<br>+ local file = assert(io.open(path), 'cannot open an object file')<br>+ local content = file:read('*a')<br>+ file:close()<br>+ return content<br>+end<br>+<br>+-- Parses a buffer in an ELF format and returns an offset and a size of strtab<br>+-- and symtab sections.<br>+local function read_elf(elf_content)<br>+ local ELFobj_type = ffi.typeof(is64 and 'ELF64obj *' or 'ELF32obj *')<br>+ local ELFsectheader_type = ffi.typeof(is64 and 'ELF64sectheader *' or 'ELF32sectheader *')<br>+ local elf = ffi.cast(ELFobj_type, elf_content)<br>+ local symtab_hdr, strtab_hdr<br>+ -- Iterate by section headers.<br>+ for i = 0, elf.hdr.shnum do<br>+ local sec = ffi.cast(ELFsectheader_type, elf.sect[i])<br>+ if sec.type == SHT_SYMTAB then<br>+ symtab_hdr = sec<br>+ strtab_hdr = ffi.cast(ELFsectheader_type, elf.sect[symtab_hdr.link])<br>+ break<br>+ end<br>+ end<br>+<br>+ assert(strtab_hdr ~= nil, 'section .strtab was not found')<br>+ assert(symtab_hdr ~= nil, 'section .symtab was not found')<br>+<br>+ return strtab_hdr, symtab_hdr<br>+end<br>+<br>+test:plan(3)<br>+<br>+local elf_filename = create_obj_file(module_name)<br>+local elf_content = read_file(elf_filename)<br>+assert(#elf_content ~= 0, 'cannot read an object file')<br>+<br>+local strtab, symtab = read_elf(elf_content)<br>+local strtab_size = tonumber(strtab.size)<br>+local strtab_offset = tonumber(strtab.ofs)<br>+local symtab_size = tonumber(symtab.size)<br>+local sym_cnt = tonumber(symtab_size / symtab.entsize)<br>+assert(sym_cnt ~= 0, 'number of symbols is zero')<br>+<br>+test:is(strtab_size, expected_strtab_size, 'check .strtab size')<br>+test:is(strtab_offset, expected_strtab_offset, 'check .strtab offset')</div></div></div></div></blockquote><div>As I already said offline, I don’t like that approach. ELF is made to by</div><div>as flexible as possible, so, generally, you have no guarantees about</div><div>order and, hence, offsets of any sections. The only reason why we are</div><div>getting away with that here, is because this ELF is generated</div><div>in `bcsave.lua` in a particular manner. It is ok for now, but no one</div><div>knows if this will ever change. I think that the symbol name search</div><div>below is sufficient, because it is unlikely to pass with an incorrect offset.</div><div> </div><div>All of the checks are valid for now, though, so feel free to ignore.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+<br>+local strtab_str = string.sub(elf_content, strtab_offset, strtab_offset + strtab_size)<br>+<br>+local strtab_p = ffi.cast('char *', strtab_str)<br>+local sym_name_expected = LJBC_PREFIX .. module_name<br>+local sym_is_found = false<br>+for i = 1, sym_cnt do<br>+ local sym_name = ffi.string(strtab_p + i)<br>+ if sym_name_expected == sym_name then<br>+ sym_is_found = true<br>+ break<br>+ end<br>+end<br>+<br>+test:ok(sym_is_found == true, 'symbol is found')<br>+<br>+local ret = os.remove(elf_filename)<br>+assert(ret == true, 'cannot remove an object file')<br>+<br>+os.exit(test:check() and 0 or 1)<br>--<br>2.34.1</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div></blockquote></BODY></HTML>