Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.

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

Thanks for the fixes!
Please consider my answers below.

May you please resend the patch series after adding the AVX512 checker?

On 09.04.24, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> thanks for review! Please see my comment below.
> 
> 
> On 08.04.2024 18:01, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > I'll proceed with the version from the branch.
> >
> > Please consider my comments below.
> >
> > Please rebase your branch to the tarantool/master and solve the
> > conflicts.
> >
> > On 26.03.24, Sergey Bronnikov wrote:
> >> Subject: [PATCH 2/3] OSX/iOS/ARM64: Fix generation of Mach-O object files.
> >>
> >> Thanks to Carlo Cabrera.
> >>
> >> (cherry picked from commit 3065c910ad6027031aabe2dfd3c26a3d0f014b4f)
> >>

<snipped>

> > Is there any guarantee that the given runner has avx512 support?
> > If not, our tests will fail due to illegal instruction error.
> 
> I'll add a check for AVX support and condition that will skip job if 
> there is no AVX support.

Looking forward for it.


> 

<snipped>

> --- a/.github/workflows/exotic-builds-testing.yml
> +++ b/.github/workflows/exotic-builds-testing.yml

<snipped>

> >> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
> >> index a0fb5440..29146113 100644
> >> --- a/test/LuaJIT-tests/CMakeLists.txt
> >> +++ b/test/LuaJIT-tests/CMakeLists.txt

<snipped>

> >> diff --git a/test/LuaJIT-tests/lib/ffi/index b/test/LuaJIT-tests/lib/ffi/index
> >> index e3a34e30..e7c168c2 100644
> >> --- a/test/LuaJIT-tests/lib/ffi/index
> >> +++ b/test/LuaJIT-tests/lib/ffi/index
> >> @@ -1,4 +1,4 @@
> >> -bit64.lua +luajit>=2.1
> >> +bit64.lua +luajit>=2.1 -avx512
> > Do we need to skip all tests instead of only failed one?

What about this?

> >
> >>   cdata_var.lua
> >>   copy_fill.lua
> >>   err.lua
> >> 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

<snipped>

> >> +-- Please consult for the available model architecture on
> >> +-- GCC Online Documentation [1] for available CPU codenames.
> >> +-- and Wikipedia for CPUs with AVX-512 support [2].
> > I suggest the following rewording:
> >
> > | Please consult the GCC Online Documentation [1] for available CPU
> > | codenames. Also, see the Wikipedia for CPUs with AVX-512 support [2].
> >
> > Or the follwing:
> >
> > | 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].
> >
> > Feel free to reword it like you want.
> 
> What's wrong with provided text?
> 
> Why should I change it?

Quillbot[1] suggests dropping the "for" preposition since "it is not
needed to show relationships between nouns and other words in the
sentence." Also, I found that "consult for" lacks the semantics of the
sentence you want to tell [2][3]. So I suggest rephrasing this part as
"consult the" or "consult with" or "take a look", whatever you prefer.

Regarding the part:
| CPU codenames. and Wikipedia
This dot in the middle looks clumsy, so I suggest joining sentences
with the parenthesis "Also".

> 

<snipped>

> >> +--
> >> +-- Manual steps for reproducing are the following:
> > I suppose you mean "from the original issue". Or we can use
> > | -DCMAKE_C_FLAGS="-march=skylake-avx512"
> These steps works in our fork too.

But this is unsupported by us build, we prefer to declare CMake flags
instead, don't we?

> >> +--

<snipped>

> >> +typedef struct {
> >> +  mach_fat_header fat;
> >> +  mach_fat_arch fat_arch[2];
> >> +  struct {
> >> +    mach_header_64 hdr;
> >> +    mach_segment_command_64 seg;
> >> +    mach_section_64 sec;
> >> +    mach_symtab_command sym;
> >> +  } arch[2];
> >> +  mach_nlist_64 sym_entry;
> >> +  uint8_t space[4096];
> >> +} mach_fat_obj_64;
> > Should this part be defined in the next patch, since there is no such
> > cdef at the moment?
> >
> > OTOH, it makes test more arch-agnostic. So, the adding of the other case
> > is more simple.
> Moved to the third patch.

Since this part has been moved, should we move all arm64-related
declarations and their usage to the third patch to be consistent? (*)

> >> +]]

<snipped>

> >> +
> >> +  local is64 = hw_arch == 'arm64'
> >> +
> >> +  -- Mach-O FAT object.
> >> +  local mach_fat_obj_type = ffi.typeof(is64 and
> >> +                                       'mach_fat_obj_64 *' or 'mach_fat_obj *')

(*) Like here.

> >> +  local obj = ffi.cast(mach_fat_obj_type, buf)
> >> +
> >> +  -- Mach-O FAT object header.
> >> +  local mach_fat_header_type = ffi.typeof('mach_fat_header *')
> >> +  local mach_fat_header = ffi.cast(mach_fat_header_type, obj.fat)
> > Looks like this cast is excess:
> > | src/luajit -e '
> > | local ffi = require"ffi"
> > | ffi.cdef[[
> > | struct test {int a; int b;};
> > | struct test_outer { struct test t; int c;};
> > | ]]
> > | local s = ffi.new("struct test_outer", {{0}, 0})
> > | print(s.t.a)
> > | '
> > | 0
> 
> Code in the test casts cdata to ffi type, you snippet demonstrate how to 
> create cdata using ffi.new.
> 
> It is similar cases?

Yes, since the resulting cdata objects iherit their type as declared in
the parent object.

The following patch works fine for me (**):

===================================================================
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 89ef9f33..6d0fef95 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
@@ -231,17 +231,15 @@ local function read_mach_o(buf, hw_arch)
   local obj = ffi.cast(mach_fat_obj_type, buf)
 
   -- Mach-O FAT object header.
-  local mach_fat_header_type = ffi.typeof('mach_fat_header *')
-  local mach_fat_header = ffi.cast(mach_fat_header_type, obj.fat)
+  local mach_fat_header = obj.fat
   -- Mach-O FAT is BE, target arch is LE.
   local be32 = bit.bswap
   res.header.magic = be32(mach_fat_header.magic)
   res.header.nfat_arch = be32(mach_fat_header.nfat_arch)
 
   -- Mach-O FAT object arches.
-  local mach_fat_arch_type = ffi.typeof('mach_fat_arch *')
   for i = 0, res.header.nfat_arch - 1 do
-    local fat_arch = ffi.cast(mach_fat_arch_type, obj.fat_arch[i])
+    local fat_arch = obj.fat_arch[i]
     local arch = {
       cputype = be32(fat_arch.cputype),
       cpusubtype = be32(fat_arch.cpusubtype),
===================================================================

| $ ctest -R lj-865
| Test project /home/burii/reviews/luajit/mach-o-fix
|     Start 190: test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
| 1/1 Test #190: test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua ...   Passed    0.04 sec
|
| 100% tests passed, 0 tests failed out of 1
|
| Label Time Summary:
| tarantool-tests    =   0.04 sec*proc (1 test)
|
| Total Test time (real) =   0.15 sec

> 

<snipped>

> >> +  local mach_fat_arch_type = ffi.typeof('mach_fat_arch *')
> >> +  for i = 0, res.header.nfat_arch - 1 do
> >> +    local fat_arch = ffi.cast(mach_fat_arch_type, obj.fat_arch[i])
> > Looks like this cast is excess.

See (**).

> >

<snipped>

> >
> >> +-- Numbers below are defined in <src/jit/bcsave.lua:bcsave_machobj>.
> > Typo: s/Numbers/The numbers/
> 
> There is no space for "<src/jit/bcsave.lua:bcsave_machob>" on the line
> 
> after adding "The"as you requested. And line become underfileld if
> 
> "<src/jit/bcsave.lua:bcsave_machob>" moved to the next line. How do it 
> better:
> 
> leave a free space on the line or left "Number" without article?

I suppose something like this will satisfy both of us :)

| -- Universal Binary can contain executables for more than one
| -- CPU architecture. For simplicity, the test compares the sum of
| -- CPU types and CPU subtypes.
| --
| -- <src/jit/bcsave.lua:bcsave_machobj> has the definitions of the
| -- numbers below. The original XNU source code may be found in
| -- <osfmk/mach/machine.h> [1].
| --
| -- 1. https://opensource.apple.com/source/xnu/xnu-4570.41.2/osfmk/mach/machine.h.auto.html

> 
> >
> >> +-- Original source is XNU source code, <osfmk/mach/machine.h> [1].
> > Typo: s/Original/The original/
> > Typo: s/XNU/the XNU/
> the same as above, with added articles line looks underfilled and 
> therefore it looks ugly.
> >

> >> +--
> >> +-- 1.https://opensource.apple.com/source/xnu/xnu-4570.41.2/osfmk/mach/machine.h.auto.html
> >> +local sum_cputype = {
> >> +  x86 = 7,
> >> +  x64 = 0x01000007,
> >> +  arm = 7 + 12,
> >> +  arm64 = 0x01000007 + 0x0100000c,
> >> +}
> >> +local sum_cpusubtype = {
> >> +  x86 = 3,
> >> +  x64 = 3,
> >> +  arm = 3 + 9,
> >> +  arm64 = 3 + 0,
> >> +}
> > I suggest the following for the clarification:
> >
> > | local cputype = {
> > |   x86 = 7,
> > |   x64 = 0x01000007,
> > |   arm = 7,
> > |   arm64 = 0x0100000c,
> > | }
> > | local cpusubtype = {
> > |   x86 = 3,
> > |   x64 = 3,
> > |   arm = 9,
> > |   arm64 = 0,
> > | }
> >
> > And declare next:
> >
> > | local sum_cputype = {
> > |   arm = cputype.arm + cputype.x86,
> > |   arm64 = cputype.arm64 + cputype.x64,
> > | }
> > | local sum_cpusubtype = {
> > |   arm = cpusubtype.arm + cpusubtype.x86,
> > |   arm64 = cpusubtype.arm64 + cpusubtype.x64,
> > | }
> 
> You proposed changes breaks the test.

My bad that was an error at copipasting here

The following patch works OK for me:
===================================================================
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 89ef9f33..c1fbe635 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
@@ -260,18 +258,28 @@ end
 -- <osfmk/mach/machine.h> [1].
 --
 -- 1. https://opensource.apple.com/source/xnu/xnu-4570.41.2/osfmk/mach/machine.h.auto.html
---
-local sum_cputype = {
+
+local cputype = {
   x86 = 7,
   x64 = 0x01000007,
-  arm = 7 + 12,
-  arm64 = 0x01000007 + 0x0100000c,
+  arm = 12,
+  arm64 = 0x0100000c,
 }
-local sum_cpusubtype = {
+
+local cpusubtype = {
   x86 = 3,
   x64 = 3,
-  arm = 3 + 9,
-  arm64 = 3 + 0,
+  arm = 9,
+  arm64 = 0,
+}
+
+local sum_cputype = {
+  arm = cputype.arm + cputype.x86,
+  arm64 = cputype.arm64 + cputype.x64,
+}
+local sum_cpusubtype = {
+  arm = cpusubtype.arm + cpusubtype.x86,
+  arm64 = cpusubtype.arm64 + cpusubtype.x64,
 }
 
===================================================================

> 
> See original dictionaries
> 
> https://github.com/tarantool/luajit/blob/ecc45358ddab52c2d0100c74da8efddef2aaf365/src/jit/bcsave.lua#L512-L513
> 
>    local cputype = ({ x86={7}, x64={0x01000007}, arm={7,12}, 
> arm64={0x01000007,0x0100000c} })[ctx.arch]
> 
> 
> converted to sum_cputype = { x86 = 7, x64 = 0x01000007, arm = 7 + 12, 
> arm64 = 0x01000007 + 0x0100000c}
> 
> 
>    local cpusubtype = ({ x86={3}, x64={3}, arm={3,9}, arm64={3,0} 
> })[ctx.arch]
> 
> 
> converted to sum_cpusubtype = { x86 = 3, x64 = 3, arm = 3 + 9, arm64 = 3 
> + 0 }
> 
> 
> I don't know how to make this place in the test more clear. See comment 
> that explains these arrays:

Yes, I understand what these arrays are and how they are represented
here. I don't like the idea of copy-pasting the value of x86/x64
cputype/cpusubtype in the summ array.
Since it is only a suggestion (due to a matter of taste), feel free to
ignore.

> 
> 
>    -- Universal Binary can contain executables for more than one
>    -- CPU architecture. For simplicity, the test compares the sum of
>    -- CPU types and CPU subtypes.
>    -- Numbers below are defined in <src/jit/bcsave.lua:bcsave_machobj>.
>    -- The original source is the XNU source code,
>    -- <osfmk/mach/machine.h> [1].
>    --
> 
> 
> Ignored.
> 
> >
> > Also, it is preferable to use upper case since values are constants.
> 
> 
> Made everything uppercase.

Side note: What part do you change? I mean SUM_CPUTYPE, but I see no changes about
them on the branch.

> 
> >
> >> +
> >> +-- The function builds Mach-O FAT object file and retrieves
> >> +-- its header fields (magic and nfat_arch)
> >> +-- and fields of the each arch (cputype, cpusubtype).
> > Typo: s/the each/each/
> Fixed. Also line above was underfilled, fixed that.

Nice, thanks.

> >
> >> +--
> >> +-- Mach-O FAT object header can be retrieved with `otool` on macOS:
> > Typo: s/Mach-O FAT object/A Mach-O FAT object/
> Fixed to "The Mach-O FAT object".
> > Typo: s/header/headers/
> single header, why "s"?

My bad. Just ignore this comment.

> >> +--

<snipped>

> >> +local function build_and_check_mach_o(hw_arch)
> > Maybe it is better to use a subtest here:
> > The number of tests is fixed for this function, and it becomes more
> > clear when we add the additional arch for testing.

What do you think about this?

> >
> >> +  assert(hw_arch == 'arm' or hw_arch == 'arm64')

(*) And like here.

> >> +
> >> +  -- FAT_MAGIC is an integer containing the value 0xCAFEBABE in
> >> +  -- big-endian byte order format. On a big-endian host CPU,
> >> +  -- this can be validated using the constant FAT_MAGIC;
> >> +  -- on a little-endian host CPU, it can be validated using
> >> +  -- the constant FAT_CIGAM.
> >> +  --
> >> +  -- FAT_NARCH is an integer specifying the number of fat_arch
> >> +  -- data structures that follow. This is the number of
> >> +  -- architectures contained in this binary.
> >> +  --
> >> +  -- See aforementioned "OS X ABI Mach-O File Format Reference".
> > Typo: s/aforementioned/the aforementioned/
> 
> With added article I need to move name of the reference to a next line,
> 
> and line looks under-filled and ugly.

For me the following formatting looks OK:

| -- FAT_NARCH is an integer specifying the number of fat_arch
| -- data structures that follow. This is the number of
| -- architectures contained in this binary.
| --
| -- See the aforementioned "OS X ABI Mach-O File Format
| -- Reference".
| local FAT_MAGIC = '0xffffffffcafebabe'
| local FAT_NARCH = 2

> 
> >> +  --
> > Excess empty line.
> 
> It is a visual splitter.

So we may not use a comment here?

> 
> 
> >
> >> +  local FAT_MAGIC = '0xffffffffcafebabe'
> >> +  local FAT_NARCH = 2
> >> +
> >> +  local MODULE_NAME = 'lango_team'
> >> +
> >> +  local mach_o_obj_path = create_obj_file(MODULE_NAME, hw_arch)
> >> +  local mach_o_buf = require('utils').tools.read_file(mach_o_obj_path)
> > Please move `require` of the necessary tool to the start of the file to
> > be consistent with the code style in tests.
> 
> moved.
> 
> 
> I found that 20 tests in our repository are not consistent too:

I found only 8 (at tarantool/master branch):

| grep -P "require\('utils'\).*\(" -R test/tarantool-tests/
| test/tarantool-tests/lj-366-strtab-correct-size.test.lua:  local lua_bin = require('utils').exec.luacmd(arg):match('%S+')
| test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua:local script = require('utils').exec.makecmd(arg)
| test/tarantool-tests/lj-946-print-errors-from-gc-fin-default.test.lua:local script = require('utils').exec.makecmd(arg, { redirect = '2>&1' })
| test/tarantool-tests/lj-962-stack-overflow-report.test.lua:local LUABIN = require('utils').exec.luabin(arg)
| test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua:local script = require('utils').exec.makecmd(arg, {
| test/tarantool-tests/lj-351-print-tostring-number.test.lua:local script = require('utils').exec.makecmd(arg)
| test/tarantool-tests/lj-flush-on-trace.test.lua:local script = require('utils').exec.makecmd(arg, {
| test/tarantool-tests/gh-4427-ffi-sandwich.test.lua:local script = require('utils').exec.makecmd(arg, {

Your listing also includes local definitions (such as those I proposed).
AFAICS, only `makecmd` and `luacmd` are used in a different way, and it
may definitely be refactored later if it is crucial for us.

<snipped>

> >> 2.44.0

[1]: https://quillbot.com/grammar-check
[2]: https://www.multitran.com/m.exe?l1=2&l2=1&s=consult%20for
[3]: https://translate.google.com/?sl=en&tl=ru&text=consult%20for%0A&op=translate

-- 
Best regards,
Sergey Kaplun