Re: [Tarantool-patches] [PATCH luajit v2 1/1] Fix saved bytecode encapsulated in ELF objects.

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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 <sergeyb@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 :).

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 <sergeyb@tarantool.org>
>> Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>
>> Reviewed-by: Maxim Kokryashkin <m.kokryashkin@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.)


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 <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.


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)
> <snipped>
>
>> -- 
>> 2.34.1
>>
> [1]: https://refspecs.linuxbase.org/elf/gabi4+/ch4.strtab.html
>