Hi, Sergey thanks for the comments and fixes! But please don't squash fixes next time. Sergey On 11.07.2024 10:29, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the fixes! > I've applied some minor fixes to your branch. > LGTM, after adding the comment about ncmds value. > > On 10.07.24, Sergey Bronnikov wrote: >> Sergey, >> >> thanks for review. Fixes applied and force-pushed to the same branch. >> >> >> Sergey >> >> On 09.07.2024 16:03, Sergey Kaplun via Tarantool-patches wrote: >>> Hi, Sergey! >>> Thanks for the patch! >>> Please consider my comments below. >>> >>> May we add the test [1] to verify that there will be no regression in the >>> future? >> This "test" was for a FAT binary generated by LuaJIT. >> >> With backported patches, FAT (aka Universal Binary) support is gone. >> >> mach-O header is checked by tests in proposed patch. > OK, let's leave it as is. Thanks! > >>> On 05.07.24, Sergey Bronnikov wrote: >>>> Reported by Sergey Bronnikov. >>>> >>>> (cherry picked from commit 7110b935672489afd6ba3eef3e5139d2f3bd05b6) >>>> >>>> Previously, LuaJIT generated Mach-O FAT object files for ARM and >>>> ARM64 on macOS. The patch removes support of 32-bit ARM and >>>> FAT object files and now LuaJIT generate Mach-O object files for >>>> ARM64. >>> I suppose we should mention that no x86/x86_64 objects are generated >>> now. >> x86_64 is still there: >> >> $ ./build/gc64/src/luajit -b -o osx -a arm64 empty.lua empty.o >> $ file empty.o >> empty.o: Mach-O 64-bit arm64 object >> $ ./build/gc64/src/luajit -b -o osx -a x64 empty.lua empty.o >> $ file empty.o >> empty.o: Mach-O 64-bit x86_64 object >> $ > My bad, thanks for the clarification. > >> Anyway, commit message has been updated. >> >>>> Sergey Bronnikov: >>>> * added the description and the trimmed the test for the problem >>>> >>>> Part of tarantool/tarantool#10199 >>>> --- >>>> src/jit/bcsave.lua | 155 ++------- >>>> ...-865-cross-generation-mach-o-file.test.lua | 294 +++--------------- >>>> 2 files changed, 70 insertions(+), 379 deletions(-) >>>> >>>> diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua >>>> index 26ec29c6..61953c2d 100644 >>>> --- a/src/jit/bcsave.lua >>>> +++ b/src/jit/bcsave.lua > > >>>> + subtest:is(mach_o.header.ncmds, 2, >>> Why there are 2 commands for Mach-O format? >>> Please use the named constant. >> is constant name 'ncmds' self-explained? > No, since I don't understand why it is 2 and not 12. Please, add a > comment to make it obvious for any reader. Why it should be 12? I just used a constant from bcsave.lua [1]. I don't get an initial idea behind this number. As I got it right from Mach-O spec `ncmd` is a number of load commands following the header. The possible load commands listed in a table [2], but no one load command listed in a table is used in bcsave.lua for generation Mach-O object file. 1. https://github.com/LuaJIT/LuaJIT/blob/04dca7911ea255f37be799c18d74c305b921c1a6/src/jit/bcsave.lua#L489 2. https://github.com/aidansteele/osx-abi-macho-file-format-reference?tab=readme-ov-file#table-4-mach-o-load-commands >>>> + 'ncmds is correct in Mach-O') >>> Looks like this line may be joined with the previous one. >> joined >>>> end > > > I've added some fixes to your branch: > * Fixed the comment from --- to --. > * Fixed some typos. > * Moved constant declarations to the beginning of the file. > * Join some lines since there is no need to add a new line. Added two more fixes: * moved test description to the top of the test. It would be better to place problem description at the beginning and Mach-O format description before constants. * sorted constants alphabetically * and added a comment: --- a/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua +++ b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua @@ -64,6 +64,7 @@ test:plan(1)  local CPU_SUBTYPE_ARM64 = 0x0  local CPU_TYPE_ARM64 = '0x100000c' +-- MH_MAGIC_64: mach-o, big-endian, x64.  local MH_MAGIC_64 = '0xfeedfacf'  -- Relocatable object file.  local MH_OBJECT = 0x1 > > =================================================================== > diff --git a/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua > index d30198c0..9963bf88 100644 > --- a/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua > +++ b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua > @@ -3,6 +3,12 @@ local test = tap.test('lj-865-cross-generation-mach-o-file') > local utils = require('utils') > local ffi = require('ffi') > > +local CPU_TYPE_ARM64 = '0x100000c' > +local CPU_SUBTYPE_ARM64 = 0x0 > +local MH_MAGIC_64 = '0xfeedfacf' > +-- Relocatable object file. > +local MH_OBJECT = 0x1 > + > test:plan(1) > > -- The test creates an object file in Mach-O format with LuaJIT > @@ -66,64 +72,64 @@ local function create_obj_file(name, arch) > return mach_o_path > end > > --- Parses a buffer in the Mach-O format and returns its fields > --- in a table. > +-- Parses a buffer in the Mach-O format and returns its fields in > +-- a table. > -- The test creates an object file in Mach-O format with LuaJIT > -- bytecode and checks the validity of the object file fields. > ---- > ---- The original problem is reproduced with LuaJIT, which is built > ---- with enabled AVX512F instructions. The support for AVX512F > ---- could be checked in `/proc/cpuinfo` on Linux and > ---- `sysctl hw.optional.avx512f` on Mac. AVX512F must be > ---- implicitly enabled in a C compiler by passing a CPU codename. > ---- Please take a look at the GCC Online Documentation [1] for > ---- available CPU codenames. Also, see the Wikipedia for CPUs with > ---- AVX-512 support [2]. > ---- Execute command below to detect the CPU codename: > ---- `gcc -march=native -Q --help=target | grep march`. > ---- > ---- 1.https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html > ---- 2.https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512 > ---- > ---- Manual steps for reproducing are the following: > ---- > ---- $ CC=gcc TARGET_CFLAGS='skylake-avx512' cmake -S . -B build > ---- $ cmake --build build --parallel > ---- $ echo > test.lua > ---- $ LUA_PATH="src/?.lua;;" luajit -b -o osx -a arm test.lua test.o > ---- $ file test.o > ---- empty.o: DOS executable (block device driver) > - > ---- The format of the Mach-O is described in the document > ---- "OS X ABI Mach-O File Format Reference", published by Apple > ---- company. The copy of the (now removed) official documentation > ---- can be found here [1]. There is a good visual representation > ---- of Mach-O format in "Mac OS X Internals" book (pages 67-68) > ---- [2] and in the [3]. > -- > ---- 0x0000000 --------------------------------------- > ---- | 0xfeedface MH_MAGIC magic > ---- | ------------------------------------ > ---- | 0x00000012 CPU_TYPE_POWERPC cputype > ---- | ------------------------------------ > ---- struct | 0x00000000 CPU_SUBTYPE_POWERPC_ALL cpusubtype > ---- mach_header | ------------------------------------ > ---- | 0x00000002 MH_EXECUTE filetype > ---- | ------------------------------------ > ---- | 0x0000000b 10 load commands ncmds > ---- | ------------------------------------ > ---- | 0x00000574 1396 bytes sizeofcmds > ---- | ------------------------------------ > ---- | 0x00000085 DYLDLINK TWOLEVEL flags > ---- -------------------------------------- > ---- Load commands > ---- --------------------------------------- > ---- Data > ---- --------------------------------------- > ---- > ---- 1.https://github.com/aidansteele/osx-abi-macho-file-format-reference > ---- 2.https://reverseengineering.stackexchange.com/a/6357/46029 > ---- 3.http://formats.kaitai.io/mach_o/index.html > +-- The original problem is reproduced with LuaJIT, which is built > +-- with enabled AVX512F instructions. The support for AVX512F > +-- could be checked in `/proc/cpuinfo` on Linux and > +-- `sysctl hw.optional.avx512f` on Mac. AVX512F must be implicitly > +-- enabled in a C compiler by passing a CPU codename. > +-- Please take a look at the GCC Online Documentation [1] for > +-- available CPU codenames. Also, see the Wikipedia for CPUs with > +-- AVX-512 support [2]. > +-- Execute the command below to detect the CPU codename: > +-- `gcc -march=native -Q --help=target | grep march`. > +-- > +-- 1.https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html > +-- 2.https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512 > +-- > +-- Manual steps for reproducing are the following: > +-- > +-- $ CC=gcc TARGET_CFLAGS='skylake-avx512' cmake -S . -B build > +-- $ cmake --build build --parallel > +-- $ echo > test.lua > +-- $ ./luajit -b -o osx -a arm64 test.lua test.o > +-- $ file test.o > +-- test.o: Mach-O 64-bit arm64 object > + > +-- The format of the Mach-O is described in the document > +-- "OS X ABI Mach-O File Format Reference", published by Apple > +-- company. The copy of the (now removed) official documentation > +-- can be found here [1]. There is a good visual representation > +-- of Mach-O format in "Mac OS X Internals" book (pages 67-68) > +-- [2] and in the [3]. > +-- > +-- 0x0000000 --------------------------------------- > +-- | 0xfeedface MH_MAGIC magic > +-- | ------------------------------------ > +-- | 0x00000012 CPU_TYPE_POWERPC cputype > +-- | ------------------------------------ > +-- struct | 0x00000000 CPU_SUBTYPE_POWERPC_ALL cpusubtype > +-- mach_header | ------------------------------------ > +-- | 0x00000002 MH_EXECUTE filetype > +-- | ------------------------------------ > +-- | 0x0000000b 10 load commands ncmds > +-- | ------------------------------------ > +-- | 0x00000574 1396 bytes sizeofcmds > +-- | ------------------------------------ > +-- | 0x00000085 DYLDLINK TWOLEVEL flags > +-- -------------------------------------- > +-- Load commands > +-- --------------------------------------- > +-- Data > +-- --------------------------------------- > +-- > +-- 1.https://github.com/aidansteele/osx-abi-macho-file-format-reference > +-- 2.https://reverseengineering.stackexchange.com/a/6357/46029 > +-- 3.http://formats.kaitai.io/mach_o/index.html > local function read_mach_o_hdr(buf, hw_arch) > -- LuaJIT always generate 64-bit non-FAT Mach-O object files. > assert(hw_arch == 'arm64') > @@ -138,11 +144,11 @@ local function read_mach_o_hdr(buf, hw_arch) > return mach_header > end > > --- The function builds Mach-O object file and retrieves > --- its header fields. > +-- The function builds a Mach-O object file and retrieves its > +-- header fields. > local function build_and_check_mach_o(subtest) > local hw_arch = subtest.name > - -- LuaJIT always generate 64-bit non-FAT Mach-O object files. > + -- LuaJIT always generates 64-bit, non-FAT Mach-O object files. > assert(hw_arch == 'arm64') > > subtest:plan(5) > @@ -159,19 +165,12 @@ local function build_and_check_mach_o(subtest) > assert(os.remove(mach_o_obj_path), 'remove an object file') > > local magic_str = string.format('%#x', mach_o.magic) > - local MH_MAGIC_64 = '0xfeedfacf' > - subtest:is(magic_str, MH_MAGIC_64, > - 'magic is correct in Mach-O') > + subtest:is(magic_str, MH_MAGIC_64, 'magic is correct in Mach-O') > local cputype_str = string.format('%#x', mach_o.cputype) > - local CPU_TYPE_ARM64 = '0x100000c' > - subtest:is(cputype_str, CPU_TYPE_ARM64, > - 'cputype is correct in Mach-O') > - local CPU_SUBTYPE_ARM64 = 0 > + subtest:is(cputype_str, CPU_TYPE_ARM64, 'cputype is correct in Mach-O') > subtest:is(mach_o.cpusubtype, CPU_SUBTYPE_ARM64, > 'cpusubtype is correct in Mach-O') > - local MH_OBJECT = 1 > - subtest:is(mach_o.filetype, MH_OBJECT, > - 'filetype is correct in Mach-O') > + subtest:is(mach_o.filetype, MH_OBJECT, 'filetype is correct in Mach-O') > subtest:is(mach_o.ncmds, 2, 'ncmds is correct in Mach-O') > end > > =================================================================== > > Please let me know if you have any concerns. >