Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the fixes!
Please consider my answers below.

On Tue, Mar 19, 2024 at 11:19:19AM +0300, Sergey Bronnikov wrote:
> Max,
>
> thanks for review! see comments below
>
>
> On 3/18/24 15:53, Maxim Kokryashkin wrote:
> > Hi, Sergey, thanks for the patch!
> > Please consider my comments below.
> > On Thu, Mar 14, 2024 at 02:39:49PM +0300, Sergey Bronnikov wrote:
<snipped>
> > >   .github/workflows/exotic-builds-testing.yml   |   5 +-
> > >   src/jit/bcsave.lua                            |   6 +-
> > >   .../lj-366-strtab-correct-size.test.lua       |  10 +-
> > >   ...generation_of_mach-o_object_files.test.lua | 271 ++++++++++++++++++
> > >   test/tarantool-tests/utils/tools.lua          |   8 +
> > >   5 files changed, 287 insertions(+), 13 deletions(-)
> > >   create mode 100644 test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
> > >
> > > diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
> > > index a9ba5fd5..df4bc2e9 100644
> > > --- a/.github/workflows/exotic-builds-testing.yml
> > > +++ b/.github/workflows/exotic-builds-testing.yml
> > > @@ -32,6 +32,7 @@ jobs:
> > >         fail-fast: false
> > >         matrix:
> > >           BUILDTYPE: [Debug, Release]
> > > +        OS: [Linux, macOS]
> > >           ARCH: [ARM64, x86_64]
> > >           GC64: [ON, OFF]
> > >           FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
> > > @@ -50,13 +51,15 @@ jobs:
> > >               FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
> > https://github.com/tarantool/luajit/actions/runs/8279362128
> > >             - FLAVOR: nounwind
> > >               FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
> > > +          - FLAVOR: avx512
> > > +            CMAKEFLAGS: -DCMAKE_C_FLAGS=skylake-avx512 -DCMAKE_C_COMPILER=gcc
> > >           exclude:
> > >             - ARCH: ARM64
> > >               GC64: OFF
> > >             # DUALNUM is default for ARM64, no need for additional testing.
> > >             - FLAVOR: dualnum
> > >               ARCH: ARM64
> > > -    runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
> > > +    runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}', '${ matrix.OS }']
> > The matrix.OS variable should be wrapped with double curly braces,
> > instead of singular ones. So, it should be like this:
> > | '${{ matrix.OS }}'
>
> Fixed, thanks!
>
>
> > Currently, exotic build testing fails to start because of this mistake.
> > https://github.com/tarantool/luajit/actions/runs/8279362128

That's unfortunate, but the workflow run still fails to start. It seems
like double quotation marks are required around the computed properties.
At least, they are mentioned specifically in the documentation:
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idruns-on

So both variables should actually be:
| "${{ matrix.OS }}", "${{ matrix.ARCH }}"

> > >       name: >
> > >         LuaJIT ${{ matrix.FLAVOR }}
> > >         (Linux/${{ matrix.ARCH }})
> > > diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua
> > > index a287d675..7aec1555 100644
> > > --- a/src/jit/bcsave.lua
> > > +++ b/src/jit/bcsave.lua
> > > @@ -446,18 +446,18 @@ typedef struct {
> > >     uint32_t value;
> > >   } mach_nlist;
> > >   typedef struct {
> > > -  uint32_t strx;
> > > +  int32_t strx;
> > >     uint8_t type, sect;
> > >     uint16_t desc;
> > >     uint64_t value;
> > >   } mach_nlist_64;
> > >   typedef struct
> > >   {
> > > -  uint32_t magic, nfat_arch;
> > > +  int32_t magic, nfat_arch;
> > >   } mach_fat_header;
> > >   typedef struct
> > >   {
> > > -  uint32_t cputype, cpusubtype, offset, size, align;
> > > +  int32_t cputype, cpusubtype, offset, size, align;
> > >   } mach_fat_arch;
> > >   typedef struct {
> > >     struct {
> > > diff --git a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> > > index 8a97a441..0bb92da6 100644
> > > --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> > > +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> > Let's move this update to a separate patch alongside with the added
> > utility function. Currently, this change in unrelated test file is
> > a bit confusing.
>
>
> Moved to a separate patch:
>
>
> commit 138bcda850ed4aad4bbbae4d30315d9a0934cb26
> Author: Sergey Bronnikov <sergeyb@tarantool.org>
> Date:   Tue Mar 19 09:56:51 2024 +0300
>
>     test: introduce a helper read_file
>
>     The test `lj-366-strtab-correct-size.test.lua` has a test helper
>     `read_file` that read a file's content and returns it.
Typo: s/read/reads/
>     This helper will be useful for a test upcoming in the next commit,
>     so it is moved to a test tools.
Typo: s/to a/to/
>
> diff --git a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> index 8a97a441..0bb92da6 100644
> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> @@ -138,14 +138,6 @@ local function create_obj_file(name)
>    return elf_filename
>  end
>
> --- Reads a file located in a specified path and returns its content.
> -local function read_file(path)
> -  local file = assert(io.open(path), 'cannot open an object file')
> -  local content = file:read('*a')
> -  file:close()
> -  return content
> -end
> -
>  -- Parses a buffer in an ELF format and returns an offset and a size of
> strtab
>  -- and symtab sections.
>  local function read_elf(elf_content)
> @@ -172,7 +164,7 @@ end
>  test:plan(3)
>
>  local elf_filename = create_obj_file(MODULE_NAME)
> -local elf_content = read_file(elf_filename)
> +local elf_content = require('utils').tools.read_file(elf_filename)
>  assert(#elf_content ~= 0, 'cannot read an object file')
>
>  local strtab, symtab = read_elf(elf_content)
> diff --git a/test/tarantool-tests/utils/tools.lua
> b/test/tarantool-tests/utils/tools.lua
> index f35c6922..26b8c08d 100644
> --- a/test/tarantool-tests/utils/tools.lua
> +++ b/test/tarantool-tests/utils/tools.lua
> @@ -12,4 +12,12 @@ function M.profilename(name)
>    return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern))
>  end
>
> +-- Reads a file located in a specified path and returns its content.
> +function M.read_file(path)
> +  local file = assert(io.open(path), 'cannot open an object file')
> +  local content = file:read('*a')
> +  file:close()
> +  return content
> +end
> +
>  return M
>
>
> > > @@ -138,14 +138,6 @@ local function create_obj_file(name)
> > >     return elf_filename
> > >   end
> > >
> > > --- Reads a file located in a specified path and returns its content.
> > > -local function read_file(path)
> > > -  local file = assert(io.open(path), 'cannot open an object file')
> > > -  local content = file:read('*a')
> > > -  file:close()
> > > -  return content
> > > -end
> > > -
> > >   -- Parses a buffer in an ELF format and returns an offset and a size of strtab
> > >   -- and symtab sections.
> > >   local function read_elf(elf_content)
> > > @@ -172,7 +164,7 @@ end
> > >   test:plan(3)
> > >
> > >   local elf_filename = create_obj_file(MODULE_NAME)
> > > -local elf_content = read_file(elf_filename)
> > > +local elf_content = require('utils').tools.read_file(elf_filename)
> > >   assert(#elf_content ~= 0, 'cannot read an object file')
> > >
> > >   local strtab, symtab = read_elf(elf_content)

The newly added patch LGTM.

> > > diff --git a/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
> > > new file mode 100644
> > > index 00000000..0519e134
> > > --- /dev/null
> > > +++ b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
> > > @@ -0,0 +1,271 @@
<snipped>
> > Side note: I feel like the comments for the sections are not elaborate
> > enough for unprepared reader. I think you should briefly desribe the
> > basic structure of a FAT object (FAT header, then array of per-segment
> > headers, then object files)
> Added a short description and an ASCII scheme.
The scheme is very nice and informative, thanks!

<snipped>

Regards,
Maxim Kokryashkin