<!DOCTYPE html>
<html data-lt-installed="true">
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body style="padding-bottom: 1px;">
    <p>Hi, Sergey</p>
    <p>thanks for the comments and fixes!</p>
    <p>But please don't squash fixes next time.</p>
    <p><br>
    </p>
    <p>Sergey<br>
    </p>
    <div class="moz-cite-prefix">On 11.07.2024 10:29, Sergey Kaplun
      wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:Zo-J514Zc8I7Peg1@root">
      <pre class="moz-quote-pre" wrap="">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:
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">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:
</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">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?
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
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.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
OK, let's leave it as is.</pre>
    </blockquote>
    Thanks!<br>
    <blockquote type="cite" cite="mid:Zo-J514Zc8I7Peg1@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">
</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
On 05.07.24, Sergey Bronnikov wrote:
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">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.
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">I suppose we should mention that no x86/x86_64 objects are generated
now.
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">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
$
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
My bad, thanks for the clarification.

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">
Anyway, commit message has been updated.

</pre>
        <blockquote type="cite">
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">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
</pre>
          </blockquote>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
<snipped>

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">+  subtest:is(mach_o.header.ncmds, 2,
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">Why there are 2 commands for Mach-O format?
Please use the named constant.
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">is constant name 'ncmds' self-explained?
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
No, since I don't understand why it is 2 and not 12. Please, add a
comment to make it obvious for any reader.
</pre>
    </blockquote>
    <p>Why it should be 12?</p>
    <p>I just used a constant from bcsave.lua [1].</p>
    <p>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</p>
    <p>listed in a table is used in bcsave.lua for generation Mach-O
      object file.</p>
    <p><br>
    </p>
    <p>1.
<a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/blob/04dca7911ea255f37be799c18d74c305b921c1a6/src/jit/bcsave.lua#L489">https://github.com/LuaJIT/LuaJIT/blob/04dca7911ea255f37be799c18d74c305b921c1a6/src/jit/bcsave.lua#L489</a></p>
    <p>2.
<a class="moz-txt-link-freetext" href="https://github.com/aidansteele/osx-abi-macho-file-format-reference?tab=readme-ov-file#table-4-mach-o-load-commands">https://github.com/aidansteele/osx-abi-macho-file-format-reference?tab=readme-ov-file#table-4-mach-o-load-commands</a><br>
    </p>
    <blockquote type="cite" cite="mid:Zo-J514Zc8I7Peg1@root">
      <pre class="moz-quote-pre" wrap="">
</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">+             'ncmds is correct in Mach-O')
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">Looks like this line may be joined with the previous one.
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">joined
</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">  end
</pre>
          </blockquote>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
<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.</pre>
    </blockquote>
    <p>Added two more fixes:</p>
    <p>* moved test description to the top of the test. It would be
      better to place problem description</p>
    <p>at the beginning and Mach-O format description before constants.</p>
    <p>* sorted constants alphabetically</p>
    <p>* and added a comment:</p>
    <p>---
      a/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua<br>
      +++
      b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua<br>
      @@ -64,6 +64,7 @@ test:plan(1)<br>
       <br>
       local CPU_SUBTYPE_ARM64 = 0x0<br>
       local CPU_TYPE_ARM64 = '0x100000c'<br>
      +-- MH_MAGIC_64: mach-o, big-endian, x64.<br>
       local MH_MAGIC_64 = '0xfeedfacf'<br>
       -- Relocatable object file.<br>
       local MH_OBJECT = 0x1<br>
    </p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:Zo-J514Zc8I7Peg1@root">
      <pre class="moz-quote-pre" wrap="">

===================================================================
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. <a class="moz-txt-link-freetext" href="https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html">https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html</a>
---- 2. <a class="moz-txt-link-freetext" href="https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512">https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512</a>
----
---- 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. <a class="moz-txt-link-freetext" href="https://github.com/aidansteele/osx-abi-macho-file-format-reference">https://github.com/aidansteele/osx-abi-macho-file-format-reference</a>
---- 2. <a class="moz-txt-link-freetext" href="https://reverseengineering.stackexchange.com/a/6357/46029">https://reverseengineering.stackexchange.com/a/6357/46029</a>
---- 3. <a class="moz-txt-link-freetext" href="http://formats.kaitai.io/mach_o/index.html">http://formats.kaitai.io/mach_o/index.html</a>
+-- 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. <a class="moz-txt-link-freetext" href="https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html">https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html</a>
+-- 2. <a class="moz-txt-link-freetext" href="https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512">https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512</a>
+--
+-- 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. <a class="moz-txt-link-freetext" href="https://github.com/aidansteele/osx-abi-macho-file-format-reference">https://github.com/aidansteele/osx-abi-macho-file-format-reference</a>
+-- 2. <a class="moz-txt-link-freetext" href="https://reverseengineering.stackexchange.com/a/6357/46029">https://reverseengineering.stackexchange.com/a/6357/46029</a>
+-- 3. <a class="moz-txt-link-freetext" href="http://formats.kaitai.io/mach_o/index.html">http://formats.kaitai.io/mach_o/index.html</a>
 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.

</pre>
    </blockquote>
  </body>
  <lt-container></lt-container>
</html>