<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hello, Max! Thanks for comments!</p>
<p>Fixed all comments except checks for constant size and offset,
fixes force-pushed.</p>
<p><br>
</p>
<p>Sergey<br>
</p>
<div class="moz-cite-prefix">On 5/30/23 12:05, Maxim Kokryashkin
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1685437504.620113247@f480.i.mail.ru">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<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><br>
</div>
</blockquote>
</blockquote>
<snipped><br>
<blockquote type="cite"
cite="mid:1685437504.620113247@f480.i.mail.ru">
<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">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>
</div>
</blockquote>
</blockquote>
<p>Fixed!</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:1685437504.620113247@f480.i.mail.ru">
<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>
<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>
</div>
</blockquote>
</blockquote>
<p><br>
</p>
<p>Fixed!</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:1685437504.620113247@f480.i.mail.ru">
<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>
<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>
</blockquote>
</blockquote>
<p>Fixed!</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:1685437504.620113247@f480.i.mail.ru">
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<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>
</div>
</blockquote>
</blockquote>
<p><br>
</p>
<p>Fixed!</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:1685437504.620113247@f480.i.mail.ru">
<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>
<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>
</div>
</blockquote>
</blockquote>
<p>Fixed!</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:1685437504.620113247@f480.i.mail.ru">
<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>
<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>
</div>
</blockquote>
</blockquote>
<p>Fixed.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:1685437504.620113247@f480.i.mail.ru">
<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>
<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 = <a class="moz-txt-link-freetext" href="file:read('*a')">file:read('*a')</a><br>
+ <a class="moz-txt-link-freetext" href="file:close()">file:close()</a><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>
</blockquote>
</blockquote>
<p>Sure, I remember your thoughts regarding checking with constant
size and offset.</p>
<p>I left this for double-checking change proposed in patch.</p>
<p>I'll leave this as as without changes and will wait review from
Sergey</p>
<p>and then we will decide remove these checks or not.<br>
</p>
<blockquote type="cite"
cite="mid:1685437504.620113247@f480.i.mail.ru">
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<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>
</blockquote>
</body>
</html>