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

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

Hi, Sergey!
Thanks for the fixes!
LGTM after fixing a few minor nits below.

On 11.04.24, Sergey Bronnikov wrote:
> From: Mike Pall <mike>
> 

<snipped>

> ---
>  src/jit/bcsave.lua                            |   6 +-
>  test/LuaJIT-tests/CMakeLists.txt              |   9 +
>  ...-865-cross-generation-mach-o-file.test.lua | 300 ++++++++++++++++++
>  3 files changed, 312 insertions(+), 3 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
> 
> diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua
> index a287d675..7aec1555 100644
> --- a/src/jit/bcsave.lua
> +++ b/src/jit/bcsave.lua

<snipped>

> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
> index b8e4dfc4..6d073700 100644
> --- a/test/LuaJIT-tests/CMakeLists.txt
> +++ b/test/LuaJIT-tests/CMakeLists.txt
> @@ -52,6 +52,15 @@ if(LUAJIT_NO_UNWIND)
>    set(LUAJIT_TEST_TAGS_EXTRA +internal_unwinder)
>  endif()
>  
> +if(CMAKE_C_FLAGS MATCHES "-march=skylake-avx512")
> +  # FIXME: Test <bit64.lua> verifies bitwise operations on numbers.

Nit: comment line width is more than 66 symbols.

> +  # There is a known issue - bitop doesn't work in LuaJIT built
> +  # with the enabled AVX512 instruction set, see
> +  # https://github.com/tarantool/tarantool/issues/6787.
> +  # Hence, skip this when "skylake-avx512" is passed.
> +  set(LUAJIT_TEST_TAGS_EXTRA +avx512)
> +endif()
> +

Should this be a part of the first commit?

>  set(TEST_SUITE_NAME "LuaJIT-tests")
>  
>  # XXX: The call produces both test and target <LuaJIT-tests-deps>
> 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
> new file mode 100644
> index 00000000..04fb5495
> --- /dev/null
> +++ b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
> @@ -0,0 +1,300 @@
> +local tap = require('tap')
> +local test = tap.test('lj-865-cross-generation-mach-o-file')
> +local utils = require('utils')
> +
> +test:plan(1)
> +
> +-- 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 that built with

Typo: s/LuaJIT that built/LuaJIT, which is built/

> +-- 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].
> +-- To detect the CPU codename execute:

Typo: s/codename/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)
> +
> +local ffi = require('ffi')

Nit: Why do we require the ffi here alongside from others requires?

> +
> +-- LuaJIT can generate so called Universal Binary with Lua

<snipped>

> +--
> +-- There are a good visual representation of Universal Binary

Typo: s/are/is/

> +-- in "Mac OS X Internals" book (pages 67-68) [5] and in the [6].
> +-- Below is the schematic structure of Universal Binary, which
> +-- includes two executables for PowerPC and Intel i386 (omitted):
> +--
> +--   0x0000000 ---------------------------------------

<snipped>

> +local function create_obj_file(name, arch)
> +  local mach_o_path = os.tmpname() .. '.o'
> +  local lua_path = os.getenv('LUA_PATH')
> +  local lua_bin = utils.exec.luacmd(arg):match('%S+')
> +  local cmd_fmt = 'LUA_PATH="%s" %s -b -n "%s" -o osx -a %s -e "print()" %s'
> +  local cmd = (cmd_fmt):format(lua_path, lua_bin, name, arch, mach_o_path)

Nit: Typo: s/(cmd_fmt)/cmd_fmt/

> +  local ret = os.execute(cmd)
> +  assert(ret == 0, 'cannot create an object file')
> +  return mach_o_path
> +end
> +
> +-- Parses a buffer in the Mach-O format and returns

Nit: The comment line looks underfilled.

> +-- the FAT magic number and `nfat_arch`.
> +local function read_mach_o(buf)

<snipped>

> +local SUM_CPUTYPE = {

Minor: It will be nice to add the comment:
|  -- x86 + arm.

> +  arm = 7 + 12,
> +}
> +local SUM_CPUSUBTYPE = {

Minor: It will be nice to add the comment:
|  -- x86 + arm.

> +  arm = 3 + 9,
> +}
> +

<snipped>

> +local function build_and_check_mach_o(subtest, hw_arch)
> +  assert(hw_arch == 'arm')
> +
> +  subtest:plan(4)

<snipped>

> +test:test('arm', build_and_check_mach_o, 'arm')

Minor: we can use `subtest.name` as the definition of the `hw_arch` in the
`build_and_check_mach_o()`, so it helps to avoid duplication of arch
usage.

Matter of taste.
Feel free to ignore.

> +
> +test:done(true)
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun