[Tarantool-patches] [PATCH luajit v2 1/1] Fix saved bytecode encapsulated in ELF objects.
Sergey Kaplun
skaplun at tarantool.org
Tue Jun 6 15:31:57 MSK 2023
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 <sergeyb at tarantool.org>
>
> 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 <sergeyb at tarantool.org>
> Reviewed-by: Sergey Kaplun <skaplun at tarantool.org>
> Reviewed-by: Maxim Kokryashkin <m.kokryashkin at tarantool.org>
> ---
>
> 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
<snipped>
> 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 @@
<snipped>
> +-- 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 <src/jit/bcsave.lua>.
> +ffi.cdef[[
<snipped>
> +]]
> +
> +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)
<snipped>
> --
> 2.34.1
>
[1]: https://refspecs.linuxbase.org/elf/gabi4+/ch4.strtab.html
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list