[Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.
Sergey Bronnikov
sergeyb at tarantool.org
Tue Mar 26 16:53:38 MSK 2024
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 at 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
More information about the Tarantool-patches
mailing list