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

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

Max,

changes force-pushed.

On 3/19/24 19:28, Maxim Kokryashkin wrote:
> 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 }}"

Fixed. Patch with fix is below:


--- a/.github/workflows/exotic-builds-testing.yml
+++ b/.github/workflows/exotic-builds-testing.yml
@@ -35,7 +35,7 @@ jobs:
          OS: [Linux, macOS]
          ARCH: [ARM64, x86_64]
          GC64: [ON, OFF]
-        FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
+        FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind, avx512]
          include:
            - BUILDTYPE: Debug
              CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON 
-DLUA_USE_APICHECK=ON


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