Hi, Sergey

On 12.04.2024 14:19, Sergey Kaplun via Tarantool-patches wrote:
> 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.

Fixed.


>> +  # 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?

Moved to 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/
Fixed.
>
>> +-- 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,/
Fixed.
>
>> +-- `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?
Moved to another place.
>> +
>> +-- LuaJIT can generate so called Universal Binary with Lua
> <snipped>
>
>> +--
>> +-- There are a good visual representation of Universal Binary
> Typo: s/are/is/
Fixed.
>
>> +-- 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/
Fixed.
>
>> +  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.
Refilled it.
>
>> +-- 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.
Added a comment
>> +  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.
ignored
>> +
>> +test:done(true)
>> -- 
>> 2.34.1
>>