Hi, Sergey
thanks for the comments and fixes!
But please don't squash fixes next time.
Sergey
Thanks!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.
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<snipped>+ 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.
+ 'ncmds is correct in Mach-O')Looks like this line may be joined with the previous one.joinedend<snipped> 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.