From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Maxim Kokryashkin <m.kokryashkin@tarantool.org> Cc: Sergey Bronnikov <estetus@gmail.com>, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files. Date: Tue, 26 Mar 2024 16:53:38 +0300 [thread overview] Message-ID: <ffb905cc-176c-45a2-81ef-d7e46db4ac65@tarantool.org> (raw) In-Reply-To: <jn6o2mfvecya5v4yt3c3qhbg4c2vqbhgktf2ycdcya2om24jqi@4o5q4q3dflg3> 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
next prev parent reply other threads:[~2024-03-26 13:53 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-03-14 11:39 [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes Sergey Bronnikov via Tarantool-patches 2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files Sergey Bronnikov via Tarantool-patches 2024-03-18 12:53 ` Maxim Kokryashkin via Tarantool-patches 2024-03-19 8:19 ` Sergey Bronnikov via Tarantool-patches 2024-03-19 16:28 ` Maxim Kokryashkin via Tarantool-patches 2024-03-26 13:53 ` Sergey Bronnikov via Tarantool-patches [this message] 2024-03-26 15:44 ` Maxim Kokryashkin via Tarantool-patches 2024-04-08 15:01 ` Sergey Kaplun via Tarantool-patches 2024-04-09 11:07 ` Sergey Bronnikov via Tarantool-patches 2024-04-09 12:47 ` Sergey Kaplun via Tarantool-patches 2024-03-18 12:55 ` Maxim Kokryashkin via Tarantool-patches 2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file Sergey Bronnikov via Tarantool-patches 2024-03-18 13:44 ` Maxim Kokryashkin via Tarantool-patches 2024-03-19 8:22 ` Sergey Bronnikov via Tarantool-patches 2024-03-19 16:15 ` Maxim Kokryashkin via Tarantool-patches 2024-03-26 14:01 ` Sergey Bronnikov via Tarantool-patches 2024-03-26 15:45 ` Maxim Kokryashkin via Tarantool-patches 2024-04-08 15:16 ` Sergey Kaplun via Tarantool-patches 2024-04-11 7:56 ` Sergey Bronnikov via Tarantool-patches 2024-04-08 7:47 ` [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes Sergey Kaplun via Tarantool-patches 2024-04-08 13:06 ` Sergey Kaplun via Tarantool-patches 2024-04-11 8:08 ` Sergey Bronnikov via Tarantool-patches 2024-04-11 8:27 ` Sergey Kaplun via Tarantool-patches 2024-04-11 12:39 ` Sergey Bronnikov via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=ffb905cc-176c-45a2-81ef-d7e46db4ac65@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=estetus@gmail.com \ --cc=m.kokryashkin@tarantool.org \ --cc=sergeyb@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox