Re: [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS: Always generate 64 bit non-FAT Mach-O object files.

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

[-- Attachment #1: Type: text/plain, Size: 12715 bytes --]

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


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

[-- Attachment #2: Type: text/html, Size: 16716 bytes --]