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

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

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

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

> +--
> +-- $ 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