[Tarantool-patches] [PATCH luajit 0/4][v2] Mach-O generation fixes

[Tarantool-patches] [PATCH luajit 0/4][v2] Mach-O generation fixes

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Changes v2:

- addressed comments from Sergey Kaplun
- added a separate patch with AVX512 workflow

Issues:
- https://github.com/LuaJIT/LuaJIT/issues/649
- https://github.com/LuaJIT/LuaJIT/pull/863
- https://github.com/LuaJIT/LuaJIT/issues/865
- Epic: https://github.com/tarantool/tarantool/issues/9595
PR: https://github.com/tarantool/tarantool/pull/9814
Branch: https://github.com/tarantool/luajit/tree/ligurio/lj-865-fix_generation_of_mach-o_object_files

Mike Pall (2):
  OSX/iOS/ARM64: Fix generation of Mach-O object files.
  OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.

Sergey Bronnikov (2):
  ci: add a workflow for testing with AVX512
  test: introduce a helper read_file

 .github/actions/setup-linux/action.yml        |  12 +
 .github/workflows/avx512-build-testing.yml    |  54 +++
 src/jit/bcsave.lua                            |  20 +-
 test/LuaJIT-tests/CMakeLists.txt              |   9 +
 test/LuaJIT-tests/lib/ffi/bit64.lua           |   2 +-
 .../lj-366-strtab-correct-size.test.lua       |  13 +-
 ...-865-cross-generation-mach-o-file.test.lua | 345 ++++++++++++++++++
 test/tarantool-tests/utils/tools.lua          |   8 +
 8 files changed, 448 insertions(+), 15 deletions(-)
 create mode 100644 .github/workflows/avx512-build-testing.yml
 create mode 100644 test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua

-- 
2.34.1

[Tarantool-patches] [PATCH luajit 1/4][v2] ci: add a workflow for testing with AVX512

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

This commit adds a workflow for building and testing with enabled
AVX512.

Needed for tarantool/tarantool#9595
Related to tarantool/tarantool#6787
---
 .github/actions/setup-linux/action.yml     | 12 +++++
 .github/workflows/avx512-build-testing.yml | 54 ++++++++++++++++++++++
 test/LuaJIT-tests/lib/ffi/bit64.lua        |  2 +-
 3 files changed, 67 insertions(+), 1 deletion(-)
 create mode 100644 .github/workflows/avx512-build-testing.yml

diff --git a/.github/actions/setup-linux/action.yml b/.github/actions/setup-linux/action.yml
index f0171b83..19bdcfa2 100644
--- a/.github/actions/setup-linux/action.yml
+++ b/.github/actions/setup-linux/action.yml
@@ -17,3 +17,15 @@ runs:
         apt -y update
         apt -y install cmake gcc make ninja-build perl
       shell: bash
+    - name: Detect a presence of AVX512
+      id: avx512_script
+      run: |
+        # Set avx512_support environment variable to 'true' when AVX512
+        # is supported and 'false' otherwise.
+        #
+        # Normally `grep` has the exit status is 0 if a line is
+        # selected, 1 if no lines were selected, and 2 if an error
+        # occurred.
+        avx512_support=$(grep avx /proc/cpuinfo 2>&1 > /dev/null;  [[ $? == 0 ]] && echo 0 || echo 1)
+        echo "avx512_support=$avx512_support" >> $GITHUB_OUTPUT
+      shell: bash
diff --git a/.github/workflows/avx512-build-testing.yml b/.github/workflows/avx512-build-testing.yml
new file mode 100644
index 00000000..d70fa661
--- /dev/null
+++ b/.github/workflows/avx512-build-testing.yml
@@ -0,0 +1,54 @@
+name: "AVX512 build testing"
+
+on:
+  push:
+    branches-ignore:
+      - '**-notest'
+      - 'upstream-**'
+    tags-ignore:
+      - '**'
+
+concurrency:
+  # An update of a developer branch cancels the previously
+  # scheduled workflow run for this branch. However, the default
+  # branch, and long-term branch (tarantool/release/2.11,
+  # tarantool/release/2.10, etc) workflow runs are never canceled.
+  #
+  # We use a trick here: define the concurrency group as 'workflow
+  # run ID' + # 'workflow run attempt' because it is a unique
+  # combination for any run. So it effectively discards grouping.
+  #
+  # XXX: we cannot use `github.sha` as a unique identifier because
+  # pushing a tag may cancel a run that works on a branch push
+  # event.
+  group: ${{ startsWith(github.ref, 'refs/heads/tarantool/')
+    && format('{0}-{1}', github.run_id, github.run_attempt)
+    || format('{0}-{1}', github.workflow, github.ref) }}
+  cancel-in-progress: true
+
+jobs:
+  test-avx512:
+    strategy:
+      fail-fast: false
+    runs-on: [self-hosted, regular, x86_64, Linux]
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+          submodules: recursive
+      - name: setup Linux
+        uses: ./.github/actions/setup-linux
+      - name: configure
+        if: needs.avx512_script.outputs.avx512_support == 0
+        run: >
+          cmake -S . -B ${{ env.BUILDDIR }}
+          -G Ninja
+          -DCMAKE_BUILD_TYPE=RelWithDebInfo
+          -DCMAKE_C_FLAGS=-march=skylake-avx512
+          -DCMAKE_C_COMPILER=gcc
+      - name: build
+        if: needs.avx512_script.outputs.avx512_support == 0
+        run: cmake --build ${{ env.BUILDDIR }} --parallel
+      - name: run regression tests
+        if: needs.avx512_script.outputs.avx512_support == 0
+        run: cmake --build ${{ env.BUILDDIR }} --parallel --target LuaJIT-test
diff --git a/test/LuaJIT-tests/lib/ffi/bit64.lua b/test/LuaJIT-tests/lib/ffi/bit64.lua
index d1b47bef..cf3a96eb 100644
--- a/test/LuaJIT-tests/lib/ffi/bit64.lua
+++ b/test/LuaJIT-tests/lib/ffi/bit64.lua
@@ -41,7 +41,7 @@ do --- tobit/band assorted C types
   end
 end
 
-do --- tobit/band negative unsigned enum
+do --- tobit/band negative unsigned enum -avx512
   local x = ffi.new("uenum_t", -10)
   local y = tobit(x)
   local z = band(x)
-- 
2.34.1

[Tarantool-patches] [PATCH luajit 2/4][v2] test: introduce a helper read_file

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

The test `lj-366-strtab-correct-size.test.lua` has a test helper
`read_file` that reads a file's content and returns it.
This helper will be useful for a test upcoming in the next commit,
so it is moved to test tools.

Needed for tarantool/tarantool#9595
---
 .../lj-366-strtab-correct-size.test.lua             | 13 +++----------
 test/tarantool-tests/utils/tools.lua                |  8 ++++++++
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
index 8a97a441..580fce09 100644
--- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
+++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
@@ -9,6 +9,7 @@ local test = tap.test('lj-366-strtab-correct-size'):skipcond({
 })
 
 local ffi = require 'ffi'
+local utils = require('utils')
 
 -- Command below exports bytecode as an object file in ELF format:
 -- $ luajit -b -n 'lango_team' -e 'print()' xxx.obj
@@ -130,7 +131,7 @@ local is64 = is64_arch[jit.arch]
 local function create_obj_file(name)
   local elf_filename = os.tmpname() .. '.obj'
   local lua_path = os.getenv('LUA_PATH')
-  local lua_bin = require('utils').exec.luacmd(arg):match('%S+')
+  local lua_bin = utils.exec.luacmd(arg):match('%S+')
   local cmd_fmt = 'LUA_PATH="%s" %s -b -n "%s" -e "print()" %s'
   local cmd = (cmd_fmt):format(lua_path, lua_bin, name, elf_filename)
   local ret = os.execute(cmd)
@@ -138,14 +139,6 @@ local function create_obj_file(name)
   return elf_filename
 end
 
--- Reads a file located in a specified path and returns its content.
-local function read_file(path)
-  local file = assert(io.open(path), 'cannot open an object file')
-  local content = file:read('*a')
-  file:close()
-  return content
-end
-
 -- Parses a buffer in an ELF format and returns an offset and a size of strtab
 -- and symtab sections.
 local function read_elf(elf_content)
@@ -172,7 +165,7 @@ end
 test:plan(3)
 
 local elf_filename = create_obj_file(MODULE_NAME)
-local elf_content = read_file(elf_filename)
+local elf_content = require('utils').tools.read_file(elf_filename)
 assert(#elf_content ~= 0, 'cannot read an object file')
 
 local strtab, symtab = read_elf(elf_content)
diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua
index f35c6922..a2556e32 100644
--- a/test/tarantool-tests/utils/tools.lua
+++ b/test/tarantool-tests/utils/tools.lua
@@ -12,4 +12,12 @@ function M.profilename(name)
   return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern))
 end
 
+-- Reads a file located at a specified path and returns its content.
+function M.read_file(path)
+  local file = assert(io.open(path), 'cannot open an object file')
+  local content = file:read('*a')
+  file:close()
+  return content
+end
+
 return M
-- 
2.34.1

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

[Sergey Bronnikov via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Carlo Cabrera.

(cherry picked from commit 3065c910ad6027031aabe2dfd3c26a3d0f014b4f)

The Mach-O FAT object constructed by LuaJIT had an incorrect format.
The problem is reproduced when the target hardware platform has
AVX512F and LuaJIT is compiled with enabled AVX512F instructions.

The problem arises because LuaJIT FFI code for Mach-O file
generation in `bcsave.lua` relies on undefined behavior for
conversions to `uint32_t`. AVX512F has the `vcvttsd2usi`
instruction, which converts `double`/`float` to `uint32_t/uint64_t`.
Earlier architectures (SSE2, AVX2) are sorely lacking such
an instruction, as they only support signed conversions. Unsigned
conversions are done with a signed convert and range shifting -
the exact algorithm depends on the compiler. A side-effect of these
workarounds is that negative `double`/`float` often inadvertently
convert 'as expected', even though this is invoking undefined
behavior. Whereas `vcvttsd2usi` always returns 0x80000000 or
0x8000000000000000 for out-of-range inputs.

The patch fixes the problem, however, the real issue remains unfixed.

Sergey Bronnikov:
* added the description, the test for the problem and
  flavor with AVX512 to `exotic-builds-testing` workflow

Part of tarantool/tarantool#9595
---
 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
@@ -446,18 +446,18 @@ typedef struct {
   uint32_t value;
 } mach_nlist;
 typedef struct {
-  uint32_t strx;
+  int32_t strx;
   uint8_t type, sect;
   uint16_t desc;
   uint64_t value;
 } mach_nlist_64;
 typedef struct
 {
-  uint32_t magic, nfat_arch;
+  int32_t magic, nfat_arch;
 } mach_fat_header;
 typedef struct
 {
-  uint32_t cputype, cpusubtype, offset, size, align;
+  int32_t cputype, cpusubtype, offset, size, align;
 } mach_fat_arch;
 typedef struct {
   struct {
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.
+  # 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()
+
 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
+-- 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:
+-- `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')
+
+-- LuaJIT can generate so called Universal Binary with Lua
+-- bytecode. The Universal Binary format is a format for
+-- executable files that run natively on hardware platforms with
+-- different hardware architectures. This concept is more
+-- generally known as a fat binary.
+--
+-- 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]. Yet another source of truth is
+-- XNU headers, see the definition of C-structures in:
+-- [2] (`nlist_64`), [3] (`fat_arch` and `fat_header`).
+--
+-- There are a good visual representation of Universal Binary
+-- 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 ---------------------------------------
+--             |
+-- struct      | 0xcafebabe  FAT_MAGIC                 magic
+-- fat_header  | -------------------------------------
+--             | 0x00000003                            nfat_arch
+--             ---------------------------------------
+--             | 0x00000012  CPU_TYPE_POWERPC          cputype
+--             | -------------------------------------
+--             | 0x00000000  CPU_SUBTYPE_POWERPC_ALL   cpusubtype
+-- struct      | -------------------------------------
+-- fat_arch    | 0x00001000  4096 bytes                offset
+--             | -------------------------------------
+--             | 0x00004224  16932 bytes               size
+--             | -------------------------------------
+--             | 0x0000000c  2^12 = 4096 bytes         align
+--             ---------------------------------------
+--             ---------------------------------------
+--             | 0x00000007  CPU_TYPE_I386             cputype
+--             | -------------------------------------
+--             | 0x00000003  CPU_SUBTYPE_I386_ALL      cpusubtype
+-- struct      | -------------------------------------
+-- fat_arch    | 0x00006000  24576 bytes               offset
+--             | -------------------------------------
+--             | 0x0000292c  10540 bytes               size
+--             | -------------------------------------
+--             | 0x0000000c  2^12 = 4096 bytes         align
+--             ---------------------------------------
+--               Unused
+-- 0x00001000  ---------------------------------------
+--             | 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
+--             ---------------------------------------
+--
+--               < x86 executable >
+--
+-- 1. https://github.com/aidansteele/osx-abi-macho-file-format-reference
+-- 2. https://github.com/apple-oss-distributions/xnu/blob/xnu-10002.1.13/EXTERNAL_HEADERS/mach-o/nlist.h
+-- 3. https://github.com/apple-oss-distributions/xnu/blob/xnu-10002.1.13/EXTERNAL_HEADERS/mach-o/fat.h
+-- 4. https://developer.apple.com/documentation/apple-silicon/addressing-architectural-differences-in-your-macos-code
+-- 5. https://reverseengineering.stackexchange.com/a/6357/46029
+-- 6. http://formats.kaitai.io/mach_o/index.html
+--
+-- Using the same declarations as defined in <src/jit/bcsave.lua>.
+ffi.cdef[[
+typedef struct
+{
+  uint32_t magic, cputype, cpusubtype, filetype, ncmds, sizeofcmds, flags;
+} mach_header;
+
+typedef struct {
+  uint32_t cmd, cmdsize;
+  char segname[16];
+  uint32_t vmaddr, vmsize, fileoff, filesize;
+  uint32_t maxprot, initprot, nsects, flags;
+} mach_segment_command;
+
+typedef struct {
+  char sectname[16], segname[16];
+  uint32_t addr, size;
+  uint32_t offset, align, reloff, nreloc, flags;
+  uint32_t reserved1, reserved2;
+} mach_section;
+
+typedef struct {
+  uint32_t cmd, cmdsize, symoff, nsyms, stroff, strsize;
+} mach_symtab_command;
+
+typedef struct {
+  int32_t strx;
+  uint8_t type, sect;
+  int16_t desc;
+  uint32_t value;
+} mach_nlist;
+
+typedef struct
+{
+  int32_t magic, nfat_arch;
+} mach_fat_header;
+
+typedef struct
+{
+  int32_t cputype, cpusubtype, offset, size, align;
+} mach_fat_arch;
+
+typedef struct {
+  mach_fat_header fat;
+  mach_fat_arch fat_arch[2];
+  struct {
+    mach_header hdr;
+    mach_segment_command seg;
+    mach_section sec;
+    mach_symtab_command sym;
+  } arch[2];
+  mach_nlist sym_entry;
+  uint8_t space[4096];
+} mach_fat_obj;
+]]
+
+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)
+  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
+-- the FAT magic number and `nfat_arch`.
+local function read_mach_o(buf)
+  local res = {
+    header = {
+      magic = 0,
+      nfat_arch = 0,
+    },
+    fat_arch = {},
+  }
+
+  -- Mach-O FAT object.
+  local mach_fat_obj_type = ffi.typeof('mach_fat_obj *')
+  local obj = ffi.cast(mach_fat_obj_type, buf)
+
+  -- Mach-O FAT object header.
+  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.
+  for i = 0, res.header.nfat_arch - 1 do
+    local fat_arch = obj.fat_arch[i]
+    local arch = {
+      cputype = be32(fat_arch.cputype),
+      cpusubtype = be32(fat_arch.cpusubtype),
+    }
+    table.insert(res.fat_arch, arch)
+  end
+
+  return res
+end
+
+-- 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
+--
+local SUM_CPUTYPE = {
+  arm = 7 + 12,
+}
+local SUM_CPUSUBTYPE = {
+  arm = 3 + 9,
+}
+
+-- The function builds Mach-O FAT object file and retrieves
+-- its header fields (magic and nfat_arch) and fields of each arch
+-- (cputype, cpusubtype).
+--
+-- The Mach-O FAT object header can be retrieved with `otool` on
+-- macOS:
+--
+-- $ otool -f empty.o
+-- Fat headers
+-- fat_magic 0xcafebabe
+-- nfat_arch 2
+-- <snipped>
+--
+-- CPU type and subtype can be retrieved with `lipo` on macOS:
+--
+-- $ luajit -b -o osx -a arm empty.lua empty.o
+-- $ lipo -archs empty.o
+-- i386 armv7
+-- $ luajit -b -o osx -a arm64 empty.lua empty.o
+-- $ lipo -archs empty.o
+-- x86_64 arm64
+local function build_and_check_mach_o(subtest, hw_arch)
+  assert(hw_arch == 'arm')
+
+  subtest:plan(4)
+  -- 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 the aforementioned "OS X ABI Mach-O File Format
+  -- Reference".
+  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 = utils.tools.read_file(mach_o_obj_path)
+  assert(mach_o_buf ~= nil and #mach_o_buf ~= 0, 'cannot read an object file')
+
+  local mach_o = read_mach_o(mach_o_buf)
+
+  -- Teardown.
+  assert(os.remove(mach_o_obj_path), 'remove an object file')
+
+  local magic_str = string.format('%#x', mach_o.header.magic)
+  subtest:is(magic_str, FAT_MAGIC,
+             'fat_magic is correct in Mach-O')
+  subtest:is(mach_o.header.nfat_arch, FAT_NARCH,
+             'nfat_arch is correct in Mach-O')
+
+  local total_cputype = 0
+  local total_cpusubtype = 0
+  for i = 1, FAT_NARCH do
+    total_cputype = total_cputype + mach_o.fat_arch[i].cputype
+    total_cpusubtype = total_cpusubtype + mach_o.fat_arch[i].cpusubtype
+  end
+  subtest:is(total_cputype, SUM_CPUTYPE[hw_arch],
+             'cputype is correct in Mach-O')
+  subtest:is(total_cpusubtype, SUM_CPUSUBTYPE[hw_arch],
+             'cpusubtype is correct in Mach-O')
+end
+
+test:test('arm', build_and_check_mach_o, 'arm')
+
+test:done(true)
-- 
2.34.1

[Tarantool-patches] [PATCH luajit 4/4][v2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.

[Sergey Bronnikov via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Carlo Cabrera.

(cherry picked from commit b98b37231bd2dcb79e10b0f974cefd91eb0d7b3a)

Mach-O FAT object files generated by LuaJIT for ARM64 Had
an incorrect format due to the usage of the 32-bit version of
FFI structure. This patch adds the 64-bit structure definition
and uses it for ARM64.

Sergey Bronnikov:
* added the description and the test for the problem

Part of tarantool/tarantool#9595
---
 src/jit/bcsave.lua                            | 14 ++++-
 ...-865-cross-generation-mach-o-file.test.lua | 55 +++++++++++++++++--
 2 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua
index 7aec1555..069cf1a3 100644
--- a/src/jit/bcsave.lua
+++ b/src/jit/bcsave.lua
@@ -491,6 +491,18 @@ typedef struct {
   mach_nlist sym_entry;
   uint8_t space[4096];
 } mach_fat_obj;
+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;
 ]]
   local symname = '_'..LJBC_PREFIX..ctx.modname
   local isfat, is64, align, mobj = false, false, 4, "mach_obj"
@@ -499,7 +511,7 @@ typedef struct {
   elseif ctx.arch == "arm" then
     isfat, mobj = true, "mach_fat_obj"
   elseif ctx.arch == "arm64" then
-    is64, align, isfat, mobj = true, 8, true, "mach_fat_obj"
+    is64, align, isfat, mobj = true, 8, true, "mach_fat_obj_64"
   else
     check(ctx.arch == "x86", "unsupported architecture for OSX")
   end
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 04fb5495..07988948 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
@@ -2,7 +2,7 @@ local tap = require('tap')
 local test = tap.test('lj-865-cross-generation-mach-o-file')
 local utils = require('utils')
 
-test:plan(1)
+test:plan(2)
 
 -- The test creates an object file in Mach-O format with LuaJIT
 -- bytecode and checks the validity of the object file fields.
@@ -114,6 +114,11 @@ typedef struct
   uint32_t magic, cputype, cpusubtype, filetype, ncmds, sizeofcmds, flags;
 } mach_header;
 
+typedef struct
+{
+  mach_header; uint32_t reserved;
+} mach_header_64;
+
 typedef struct {
   uint32_t cmd, cmdsize;
   char segname[16];
@@ -121,6 +126,13 @@ typedef struct {
   uint32_t maxprot, initprot, nsects, flags;
 } mach_segment_command;
 
+typedef struct {
+  uint32_t cmd, cmdsize;
+  char segname[16];
+  uint64_t vmaddr, vmsize, fileoff, filesize;
+  uint32_t maxprot, initprot, nsects, flags;
+} mach_segment_command_64;
+
 typedef struct {
   char sectname[16], segname[16];
   uint32_t addr, size;
@@ -128,6 +140,13 @@ typedef struct {
   uint32_t reserved1, reserved2;
 } mach_section;
 
+typedef struct {
+  char sectname[16], segname[16];
+  uint64_t addr, size;
+  uint32_t offset, align, reloff, nreloc, flags;
+  uint32_t reserved1, reserved2, reserved3;
+} mach_section_64;
+
 typedef struct {
   uint32_t cmd, cmdsize, symoff, nsyms, stroff, strsize;
 } mach_symtab_command;
@@ -139,6 +158,13 @@ typedef struct {
   uint32_t value;
 } mach_nlist;
 
+typedef struct {
+  int32_t strx;
+  uint8_t type, sect;
+  uint16_t desc;
+  uint64_t value;
+} mach_nlist_64;
+
 typedef struct
 {
   int32_t magic, nfat_arch;
@@ -161,6 +187,19 @@ typedef struct {
   mach_nlist sym_entry;
   uint8_t space[4096];
 } mach_fat_obj;
+
+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;
 ]]
 
 local function create_obj_file(name, arch)
@@ -176,7 +215,7 @@ end
 
 -- Parses a buffer in the Mach-O format and returns
 -- the FAT magic number and `nfat_arch`.
-local function read_mach_o(buf)
+local function read_mach_o(buf, hw_arch)
   local res = {
     header = {
       magic = 0,
@@ -185,8 +224,11 @@ local function read_mach_o(buf)
     fat_arch = {},
   }
 
+  local is64 = hw_arch == 'arm64'
+
   -- Mach-O FAT object.
-  local mach_fat_obj_type = ffi.typeof('mach_fat_obj *')
+  local mach_fat_obj_type = ffi.typeof(is64 and
+                                       'mach_fat_obj_64 *' or 'mach_fat_obj *')
   local obj = ffi.cast(mach_fat_obj_type, buf)
 
   -- Mach-O FAT object header.
@@ -221,9 +263,11 @@ end
 --
 local SUM_CPUTYPE = {
   arm = 7 + 12,
+  arm64 = 0x01000007 + 0x0100000c,
 }
 local SUM_CPUSUBTYPE = {
   arm = 3 + 9,
+  arm64 = 3 + 0,
 }
 
 -- The function builds Mach-O FAT object file and retrieves
@@ -248,7 +292,7 @@ local SUM_CPUSUBTYPE = {
 -- $ lipo -archs empty.o
 -- x86_64 arm64
 local function build_and_check_mach_o(subtest, hw_arch)
-  assert(hw_arch == 'arm')
+  assert(hw_arch == 'arm' or hw_arch == 'arm64')
 
   subtest:plan(4)
   -- FAT_MAGIC is an integer containing the value 0xCAFEBABE in
@@ -272,7 +316,7 @@ local function build_and_check_mach_o(subtest, hw_arch)
   local mach_o_buf = utils.tools.read_file(mach_o_obj_path)
   assert(mach_o_buf ~= nil and #mach_o_buf ~= 0, 'cannot read an object file')
 
-  local mach_o = read_mach_o(mach_o_buf)
+  local mach_o = read_mach_o(mach_o_buf, hw_arch)
 
   -- Teardown.
   assert(os.remove(mach_o_obj_path), 'remove an object file')
@@ -296,5 +340,6 @@ local function build_and_check_mach_o(subtest, hw_arch)
 end
 
 test:test('arm', build_and_check_mach_o, 'arm')
+test:test('arm64', build_and_check_mach_o, 'arm64')
 
 test:done(true)
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit 1/4][v2] ci: add a workflow for testing with AVX512

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On 11.04.24, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> This commit adds a workflow for building and testing with enabled
> AVX512.

Typo: s/with enabled AVX512/with AVX512 enabled/

> 
> Needed for tarantool/tarantool#9595
> Related to tarantool/tarantool#6787

Typo: s/Related/Relates/

| git log | grep Relates | wc -l
| 29
| git log | grep Related | wc -l
| 7

> ---
>  .github/actions/setup-linux/action.yml     | 12 +++++
>  .github/workflows/avx512-build-testing.yml | 54 ++++++++++++++++++++++
>  test/LuaJIT-tests/lib/ffi/bit64.lua        |  2 +-
>  3 files changed, 67 insertions(+), 1 deletion(-)
>  create mode 100644 .github/workflows/avx512-build-testing.yml
> 
> diff --git a/.github/actions/setup-linux/action.yml b/.github/actions/setup-linux/action.yml
> index f0171b83..19bdcfa2 100644
> --- a/.github/actions/setup-linux/action.yml
> +++ b/.github/actions/setup-linux/action.yml
> @@ -17,3 +17,15 @@ runs:
>          apt -y update
>          apt -y install cmake gcc make ninja-build perl
>        shell: bash
> +    - name: Detect a presence of AVX512

I suppose it shouldn't be a part of setup-linux action, but just a one
step in testing-avx512.yml, see [1] for details of steps content usage.

Is it possible or did I miss some GH-action API trick here?
But anyway it should be done not in the same action.

> +      id: avx512_script
> +      run: |
> +        # Set avx512_support environment variable to 'true' when AVX512

Typo: s/avx512_support/the avx512_support/

> +        # is supported and 'false' otherwise.

The comment is a little bit misleading since the resulting values are
"0" or "1". Changing it to 'true' or 'false' looks like a good idea and
simplifies reading later.

> +        #
> +        # Normally `grep` has the exit status is 0 if a line is

Typo: s/Normally/Normally,/
Also, "has the exit status is" has two verbs, so I suggest to
reformulate this like the following:

| Normally, the exit status of `grep` is 0 if a line is selected, 1 if
| no lines were selected, and 2 if an error occurred.

> +        # selected, 1 if no lines were selected, and 2 if an error
> +        # occurred.
> +        avx512_support=$(grep avx /proc/cpuinfo 2>&1 > /dev/null;  [[ $? == 0 ]] && echo 0 || echo 1)

Why not just echo the "$?"?

On my laptop without avx512 support this command returns true.
| $ grep avx /proc/cpuinfo 2>&1 > /dev/null;  [[ $? == 0 ]] && echo 0 || echo 1
| 0

I have no avx512 support, but avx|avx2 support. Hence, there is matching
content:

| $ grep -o -P '.{4}avx.{4}' /proc/cpuinfo
| ave avx f16
| mi1 avx2 sm
| ave avx f16
| mi1 avx2 sm
| ave avx f16
| mi1 avx2 sm
| ave avx f16
| mi1 avx2 sm

I suppose we should check for the "AVX512F" CPUID feature flag: its
supports allows to emit vcvttsd2usi [2] instruction that causes troubles.

> +        echo "avx512_support=$avx512_support" >> $GITHUB_OUTPUT

Minor: It will be nice to echo some message about the value of this
variable. Hence, we can check this from the GitHub logs.

Side note: Do I understand correctly that GH-actions output is always a
string that evaluates to `true` regardless of its content?

> +      shell: bash
> diff --git a/.github/workflows/avx512-build-testing.yml b/.github/workflows/avx512-build-testing.yml
> new file mode 100644
> index 00000000..d70fa661
> --- /dev/null
> +++ b/.github/workflows/avx512-build-testing.yml

<snipped>

> +
> +jobs:
> +  test-avx512:

Minor: I suggest adding a name for the job to be consistent with other
jobs. Something like "LuaJIT avx512""

> +    strategy:
> +      fail-fast: false
> +    runs-on: [self-hosted, regular, x86_64, Linux]
> +    steps:
> +      - uses: actions/checkout@v4
> +        with:
> +          fetch-depth: 0
> +          submodules: recursive
> +      - name: setup Linux
> +        uses: ./.github/actions/setup-linux
> +      - name: configure
> +        if: needs.avx512_script.outputs.avx512_support == 0
> +        run: >
> +          cmake -S . -B ${{ env.BUILDDIR }}
> +          -G Ninja
> +          -DCMAKE_BUILD_TYPE=RelWithDebInfo

Please add a comment. Why do we check the non-debug build here?
If there is no reason for it, maybe it is better to check both: the
debug and release builds within a simple matrix.

> +          -DCMAKE_C_FLAGS=-march=skylake-avx512
> +          -DCMAKE_C_COMPILER=gcc

Do we need to specify compiler manually?

> +      - name: build
> +        if: needs.avx512_script.outputs.avx512_support == 0
> +        run: cmake --build ${{ env.BUILDDIR }} --parallel
> +      - name: run regression tests
> +        if: needs.avx512_script.outputs.avx512_support == 0
> +        run: cmake --build ${{ env.BUILDDIR }} --parallel --target LuaJIT-test
> diff --git a/test/LuaJIT-tests/lib/ffi/bit64.lua b/test/LuaJIT-tests/lib/ffi/bit64.lua
> index d1b47bef..cf3a96eb 100644
> --- a/test/LuaJIT-tests/lib/ffi/bit64.lua
> +++ b/test/LuaJIT-tests/lib/ffi/bit64.lua
> @@ -41,7 +41,7 @@ do --- tobit/band assorted C types
>    end
>  end
>  
> -do --- tobit/band negative unsigned enum
> +do --- tobit/band negative unsigned enum -avx512

Should we also to set such variable in the CMakeLists.txt when we met
the corresponding condition?

>    local x = ffi.new("uenum_t", -10)
>    local y = tobit(x)
>    local z = band(x)
> -- 
> 2.34.1

[1]: https://docs.github.com/en/actions/learn-github-actions/contexts#example-usage-of-the-steps-context
[2]: https://www.laruence.com/x86/VCVTTSD2USI.html

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/4][v2] test: introduce a helper read_file

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, after fixing two nits below.

On 11.04.24, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 

<snipped>

> diff --git a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> index 8a97a441..580fce09 100644
> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> @@ -9,6 +9,7 @@ local test = tap.test('lj-366-strtab-correct-size'):skipcond({
>  })
>  
>  local ffi = require 'ffi'
> +local utils = require('utils')

<snipped>

>  -- Parses a buffer in an ELF format and returns an offset and a size of strtab
>  -- and symtab sections.
>  local function read_elf(elf_content)
> @@ -172,7 +165,7 @@ end
>  test:plan(3)
>  
>  local elf_filename = create_obj_file(MODULE_NAME)
> -local elf_content = read_file(elf_filename)
> +local elf_content = require('utils').tools.read_file(elf_filename)

Nit: s/require('utils')/utils/
Rationale: `utils` are already required above.

>  assert(#elf_content ~= 0, 'cannot read an object file')
>  
>  local strtab, symtab = read_elf(elf_content)
> diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua
> index f35c6922..a2556e32 100644
> --- a/test/tarantool-tests/utils/tools.lua
> +++ b/test/tarantool-tests/utils/tools.lua
> @@ -12,4 +12,12 @@ function M.profilename(name)
>    return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern))
>  end
>  
> +-- Reads a file located at a specified path and returns its content.

Nit: Comment line width is more than 66 symbols.

> +function M.read_file(path)
> +  local file = assert(io.open(path), 'cannot open an object file')
> +  local content = file:read('*a')
> +  file:close()
> +  return content
> +end
> +
>  return M
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

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

Re: [Tarantool-patches] [PATCH luajit 4/4][v2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, with a several minor nits below.

On 11.04.24, Sergey Bronnikov wrote:
> From: Mike Pall <mike>
> 
> Thanks to Carlo Cabrera.
> 
> (cherry picked from commit b98b37231bd2dcb79e10b0f974cefd91eb0d7b3a)
> 
> Mach-O FAT object files generated by LuaJIT for ARM64 Had

Typo: s/Had/had/

> an incorrect format due to the usage of the 32-bit version of
> FFI structure. This patch adds the 64-bit structure definition

Typo: s/FFI structure/the FFI structure/

> and uses it for ARM64.
> 
> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#9595
> ---
>  src/jit/bcsave.lua                            | 14 ++++-
>  ...-865-cross-generation-mach-o-file.test.lua | 55 +++++++++++++++++--
>  2 files changed, 63 insertions(+), 6 deletions(-)
> 

<snipped>

>  
>    -- Mach-O FAT object header.
> @@ -221,9 +263,11 @@ end
>  --
>  local SUM_CPUTYPE = {
>    arm = 7 + 12,
> +  arm64 = 0x01000007 + 0x0100000c,

Minor: The comment will be appreciated:
| x64 + arm64.

>  }
>  local SUM_CPUSUBTYPE = {
>    arm = 3 + 9,
> +  arm64 = 3 + 0,

Minor: The comment will be appreciated:
| x64 + arm64.

>  }
>  
>  -- The function builds Mach-O FAT object file and retrieves

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/4][v2] ci: add a workflow for testing with AVX512

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey

On 12.04.2024 13:27, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
> On 11.04.24, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov<sergeyb@tarantool.org>
>>
>> This commit adds a workflow for building and testing with enabled
>> AVX512.
> Typo: s/with enabled AVX512/with AVX512 enabled/
Fixed.
>
>> Needed for tarantool/tarantool#9595
>> Related to tarantool/tarantool#6787
> Typo: s/Related/Relates/
Fixed.
>
> | git log | grep Relates | wc -l
> | 29
> | git log | grep Related | wc -l
> | 7

>> ---
>>   .github/actions/setup-linux/action.yml     | 12 +++++
>>   .github/workflows/avx512-build-testing.yml | 54 ++++++++++++++++++++++
>>   test/LuaJIT-tests/lib/ffi/bit64.lua        |  2 +-
>>   3 files changed, 67 insertions(+), 1 deletion(-)
>>   create mode 100644 .github/workflows/avx512-build-testing.yml
>>
>> diff --git a/.github/actions/setup-linux/action.yml b/.github/actions/setup-linux/action.yml
>> index f0171b83..19bdcfa2 100644
>> --- a/.github/actions/setup-linux/action.yml
>> +++ b/.github/actions/setup-linux/action.yml
>> @@ -17,3 +17,15 @@ runs:
>>           apt -y update
>>           apt -y install cmake gcc make ninja-build perl
>>         shell: bash
>> +    - name: Detect a presence of AVX512
> I suppose it shouldn't be a part of setup-linux action,

It was requested by Max.


> but just a one
> step in testing-avx512.yml, see [1] for details of steps content usage.
>
> Is it possible or did I miss some GH-action API trick here?
> But anyway it should be done not in the same action.
>
>> +      id: avx512_script
>> +      run: |
>> +        # Set avx512_support environment variable to 'true' when AVX512
> Typo: s/avx512_support/the avx512_support/
Fixed.
>> +        # is supported and 'false' otherwise.
> The comment is a little bit misleading since the resulting values are
> "0" or "1". Changing it to 'true' or 'false' looks like a good idea and
> simplifies reading later.
Fixed.
>
>> +        #
>> +        # Normally `grep` has the exit status is 0 if a line is
> Typo: s/Normally/Normally,/
> Also, "has the exit status is" has two verbs, so I suggest to
> reformulate this like the following:
>
> | Normally, the exit status of `grep` is 0 if a line is selected, 1 if
> | no lines were selected, and 2 if an error occurred.
>
>> +        # selected, 1 if no lines were selected, and 2 if an error
>> +        # occurred.
>> +        avx512_support=$(grep avx /proc/cpuinfo 2>&1 > /dev/null;  [[ $? == 0 ]] && echo 0 || echo 1)
> Why not just echo the "$?"?
because comment above. `grep` has three possible values.
>
> On my laptop without avx512 support this command returns true.
> | $ grep avx /proc/cpuinfo 2>&1 > /dev/null;  [[ $? == 0 ]] && echo 0 || echo 1
> | 0
>
> I have no avx512 support, but avx|avx2 support. Hence, there is matching
> content:
>
> | $ grep -o -P '.{4}avx.{4}' /proc/cpuinfo
> | ave avx f16
> | mi1 avx2 sm
> | ave avx f16
> | mi1 avx2 sm
> | ave avx f16
> | mi1 avx2 sm
> | ave avx f16
> | mi1 avx2 sm
>
> I suppose we should check for the "AVX512F" CPUID feature flag: its
> supports allows to emit vcvttsd2usi [2] instruction that causes troubles.
Changed to AVX512F.
>
>> +        echo "avx512_support=$avx512_support" >> $GITHUB_OUTPUT
> Minor: It will be nice to echo some message about the value of this
> variable. Hence, we can check this from the GitHub logs.

What for? Job will be skipped if there is no AVX512 support.

What is the value of the message?


>
> Side note: Do I understand correctly that GH-actions output is always a
> string that evaluates to `true` regardless of its content?
0/1
>
>> +      shell: bash
>> diff --git a/.github/workflows/avx512-build-testing.yml b/.github/workflows/avx512-build-testing.yml
>> new file mode 100644
>> index 00000000..d70fa661
>> --- /dev/null
>> +++ b/.github/workflows/avx512-build-testing.yml
> <snipped>
>
>> +
>> +jobs:
>> +  test-avx512:
> Minor: I suggest adding a name for the job to be consistent with other
> jobs. Something like "LuaJIT avx512""
Added.
>
>> +    strategy:
>> +      fail-fast: false
>> +    runs-on: [self-hosted, regular, x86_64, Linux]
>> +    steps:
>> +      - uses: actions/checkout@v4
>> +        with:
>> +          fetch-depth: 0
>> +          submodules: recursive
>> +      - name: setup Linux
>> +        uses: ./.github/actions/setup-linux
>> +      - name: configure
>> +        if: needs.avx512_script.outputs.avx512_support == 0
>> +        run: >
>> +          cmake -S . -B ${{ env.BUILDDIR }}
>> +          -G Ninja
>> +          -DCMAKE_BUILD_TYPE=RelWithDebInfo
> Please add a comment. Why do we check the non-debug build here?
> If there is no reason for it, maybe it is better to check both: the
> debug and release builds within a simple matrix.
There is no reason for this.
>
>> +          -DCMAKE_C_FLAGS=-march=skylake-avx512
>> +          -DCMAKE_C_COMPILER=gcc
> Do we need to specify compiler manually?
Yes, CFLAG is gcc-specific.
>
>> +      - name: build
>> +        if: needs.avx512_script.outputs.avx512_support == 0
>> +        run: cmake --build ${{ env.BUILDDIR }} --parallel
>> +      - name: run regression tests
>> +        if: needs.avx512_script.outputs.avx512_support == 0
>> +        run: cmake --build ${{ env.BUILDDIR }} --parallel --target LuaJIT-test
>> diff --git a/test/LuaJIT-tests/lib/ffi/bit64.lua b/test/LuaJIT-tests/lib/ffi/bit64.lua
>> index d1b47bef..cf3a96eb 100644
>> --- a/test/LuaJIT-tests/lib/ffi/bit64.lua
>> +++ b/test/LuaJIT-tests/lib/ffi/bit64.lua
>> @@ -41,7 +41,7 @@ do --- tobit/band assorted C types
>>     end
>>   end
>>   
>> -do --- tobit/band negative unsigned enum
>> +do --- tobit/band negative unsigned enum -avx512
> Should we also to set such variable in the CMakeLists.txt when we met
> the corresponding condition?
Yes, double check.
>
>>     local x = ffi.new("uenum_t", -10)
>>     local y = tobit(x)
>>     local z = band(x)
>> -- 
>> 2.34.1
> [1]:https://docs.github.com/en/actions/learn-github-actions/contexts#example-usage-of-the-steps-context
> [2]:https://www.laruence.com/x86/VCVTTSD2USI.html
>

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

Re: [Tarantool-patches] [PATCH luajit 2/4][v2] test: introduce a helper read_file

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey

On 12.04.2024 13:47, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, after fixing two nits below.
>
> On 11.04.24, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov<sergeyb@tarantool.org>
>>
> <snipped>
>
>> diff --git a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> index 8a97a441..580fce09 100644
>> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> @@ -9,6 +9,7 @@ local test = tap.test('lj-366-strtab-correct-size'):skipcond({
>>   })
>>   
>>   local ffi = require 'ffi'
>> +local utils = require('utils')
> <snipped>
>
>>   -- Parses a buffer in an ELF format and returns an offset and a size of strtab
>>   -- and symtab sections.
>>   local function read_elf(elf_content)
>> @@ -172,7 +165,7 @@ end
>>   test:plan(3)
>>   
>>   local elf_filename = create_obj_file(MODULE_NAME)
>> -local elf_content = read_file(elf_filename)
>> +local elf_content = require('utils').tools.read_file(elf_filename)
> Nit: s/require('utils')/utils/
> Rationale: `utils` are already required above.
Fixed.
>
>>   assert(#elf_content ~= 0, 'cannot read an object file')
>>   
>>   local strtab, symtab = read_elf(elf_content)
>> diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua
>> index f35c6922..a2556e32 100644
>> --- a/test/tarantool-tests/utils/tools.lua
>> +++ b/test/tarantool-tests/utils/tools.lua
>> @@ -12,4 +12,12 @@ function M.profilename(name)
>>     return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern))
>>   end
>>   
>> +-- Reads a file located at a specified path and returns its content.
> Nit: Comment line width is more than 66 symbols.
Fixed.
>
>> +function M.read_file(path)
>> +  local file = assert(io.open(path), 'cannot open an object file')
>> +  local content =file:read('*a')
>> +file:close()
>> +  return content
>> +end
>> +
>>   return M
>> -- 
>> 2.34.1
>>

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

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

[Sergey Bronnikov via Tarantool-patches]

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

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

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

Re: [Tarantool-patches] [PATCH luajit 1/4][v2] ci: add a workflow for testing with AVX512

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey

On 16.04.24, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> On 12.04.2024 13:27, Sergey Kaplun wrote:

<snipped>

> >> diff --git a/.github/actions/setup-linux/action.yml b/.github/actions/setup-linux/action.yml
> >> index f0171b83..19bdcfa2 100644
> >> --- a/.github/actions/setup-linux/action.yml
> >> +++ b/.github/actions/setup-linux/action.yml
> >> @@ -17,3 +17,15 @@ runs:
> >>           apt -y update
> >>           apt -y install cmake gcc make ninja-build perl
> >>         shell: bash
> >> +    - name: Detect a presence of AVX512
> > I suppose it shouldn't be a part of setup-linux action,
> 
> It was requested by Max.

And what is the rationale for it?
Why don't use this detection during steps in our workflow?

Hence, we avoid this unnecesary step for other jobs.

> 
> 
> > but just a one
> > step in testing-avx512.yml, see [1] for details of steps content usage.
> >
> > Is it possible or did I miss some GH-action API trick here?
> > But anyway it should be done not in the same action.
> >

<snipped>

> >
> >> +        #
> >> +        # Normally `grep` has the exit status is 0 if a line is
> > Typo: s/Normally/Normally,/
> > Also, "has the exit status is" has two verbs, so I suggest to
> > reformulate this like the following:
> >
> > | Normally, the exit status of `grep` is 0 if a line is selected, 1 if
> > | no lines were selected, and 2 if an error occurred.

Ping.

> >
> >> +        # selected, 1 if no lines were selected, and 2 if an error
> >> +        # occurred.
> >> +        avx512_support=$(grep avx /proc/cpuinfo 2>&1 > /dev/null;  [[ $? == 0 ]] && echo 0 || echo 1)
> > Why not just echo the "$?"?
> because comment above. `grep` has three possible values.

Yes, but why do we need such tricks if we are still interested in the 0
value only? Also, with this and the logging of the `avx512_support`, we
may see the reason why the workflow was skipped: an incorrect `grep`
command (for some reason) or a not-founded CPU feature flag.

Hence, if the command fails with exit code 2, that means that our CI
settings are incorrect and need to be adjusted.

> >

<snipped>

> >
> >> +        echo "avx512_support=$avx512_support" >> $GITHUB_OUTPUT
> > Minor: It will be nice to echo some message about the value of this
> > variable. Hence, we can check this from the GitHub logs.
> 
> What for? Job will be skipped if there is no AVX512 support.
> 
> What is the value of the message?

See the rationale above.

> 
> 
> >

<snipped>

> >> +        if: needs.avx512_script.outputs.avx512_support == 0
> >> +        run: >
> >> +          cmake -S . -B ${{ env.BUILDDIR }}
> >> +          -G Ninja
> >> +          -DCMAKE_BUILD_TYPE=RelWithDebInfo
> > Please add a comment. Why do we check the non-debug build here?
> > If there is no reason for it, maybe it is better to check both: the
> > debug and release builds within a simple matrix.
> There is no reason for this.

So, let's check the Debug build only. And with enabled assertions.
They are useful for testing.

> >
> >> +          -DCMAKE_C_FLAGS=-march=skylake-avx512
> >> +          -DCMAKE_C_COMPILER=gcc
> > Do we need to specify compiler manually?
> Yes, CFLAG is gcc-specific.

Emm, is it?

| $ clang -march=skylake-avx512 /tmp/t.c -o /tmp/t.exe
| $  clang --version
| clang version 17.0.6
| Target: x86_64-pc-linux-gnu
| Thread model: posix
| InstalledDir: /usr/lib/llvm/17/bin
| Configuration file: /etc/clang/x86_64-pc-linux-gnu-clang.cfg

Maybe it is unsupported for clang version we used in CI?

> >
> >> +      - name: build
> >> +        if: needs.avx512_script.outputs.avx512_support == 0
> >> +        run: cmake --build ${{ env.BUILDDIR }} --parallel
> >> +      - name: run regression tests
> >> +        if: needs.avx512_script.outputs.avx512_support == 0
> >> +        run: cmake --build ${{ env.BUILDDIR }} --parallel --target LuaJIT-test
> >> diff --git a/test/LuaJIT-tests/lib/ffi/bit64.lua b/test/LuaJIT-tests/lib/ffi/bit64.lua
> >> index d1b47bef..cf3a96eb 100644
> >> --- a/test/LuaJIT-tests/lib/ffi/bit64.lua
> >> +++ b/test/LuaJIT-tests/lib/ffi/bit64.lua
> >> @@ -41,7 +41,7 @@ do --- tobit/band assorted C types
> >>     end
> >>   end
> >>   
> >> -do --- tobit/band negative unsigned enum
> >> +do --- tobit/band negative unsigned enum -avx512
> > Should we also to set such variable in the CMakeLists.txt when we met
> > the corresponding condition?
> Yes, double check.

I meant that part of the third commit should be done within this patch,
as you've already fixed.

> >
> >>     local x = ffi.new("uenum_t", -10)
> >>     local y = tobit(x)
> >>     local z = band(x)
> >> -- 
> >> 2.34.1
> > [1]:https://docs.github.com/en/actions/learn-github-actions/contexts#example-usage-of-the-steps-context
> > [2]:https://www.laruence.com/x86/VCVTTSD2USI.html
> >

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/4][v2] ci: add a workflow for testing with AVX512

[Maxim Kokryashkin via Tarantool-patches]

Hi!
On Thu, Apr 18, 2024 at 11:24:42AM UTC, Sergey Kaplun wrote:
> Hi, Sergey
>
> On 16.04.24, Sergey Bronnikov wrote:
> > Hi, Sergey
> >
> > On 12.04.2024 13:27, Sergey Kaplun wrote:
>
> <snipped>
>
> > >> diff --git a/.github/actions/setup-linux/action.yml b/.github/actions/setup-linux/action.yml
> > >> index f0171b83..19bdcfa2 100644
> > >> --- a/.github/actions/setup-linux/action.yml
> > >> +++ b/.github/actions/setup-linux/action.yml
> > >> @@ -17,3 +17,15 @@ runs:
> > >>           apt -y update
> > >>           apt -y install cmake gcc make ninja-build perl
> > >>         shell: bash
> > >> +    - name: Detect a presence of AVX512
> > > I suppose it shouldn't be a part of setup-linux action,
> >
> > It was requested by Max.
>
> And what is the rationale for it?
> Why don't use this detection during steps in our workflow?
>
> Hence, we avoid this unnecesary step for other jobs.
I've suggested to move that to a separate action, not to the
setup-linux.

<snipped>

Re: [Tarantool-patches] [PATCH luajit 4/4][v2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey


fixes applied and force-pushed

On 12.04.2024 14:27, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, with a several minor nits below.
>
> On 11.04.24, Sergey Bronnikov wrote:
>> From: Mike Pall <mike>
>>
>> Thanks to Carlo Cabrera.
>>
>> (cherry picked from commit b98b37231bd2dcb79e10b0f974cefd91eb0d7b3a)
>>
>> Mach-O FAT object files generated by LuaJIT for ARM64 Had
> Typo: s/Had/had/
Fixed, thanks!
>
>> an incorrect format due to the usage of the 32-bit version of
>> FFI structure. This patch adds the 64-bit structure definition
> Typo: s/FFI structure/the FFI structure/
Fixed, thanks!
>
>> and uses it for ARM64.
>>
>> Sergey Bronnikov:
>> * added the description and the test for the problem
>>
>> Part of tarantool/tarantool#9595
>> ---
>>   src/jit/bcsave.lua                            | 14 ++++-
>>   ...-865-cross-generation-mach-o-file.test.lua | 55 +++++++++++++++++--
>>   2 files changed, 63 insertions(+), 6 deletions(-)
>>
> <snipped>
>
>>   
>>     -- Mach-O FAT object header.
>> @@ -221,9 +263,11 @@ end
>>   --
>>   local SUM_CPUTYPE = {
>>     arm = 7 + 12,
>> +  arm64 = 0x01000007 + 0x0100000c,
> Minor: The comment will be appreciated:
> | x64 + arm64.
Added, thanks!
>>   }
>>   local SUM_CPUSUBTYPE = {
>>     arm = 3 + 9,
>> +  arm64 = 3 + 0,
> Minor: The comment will be appreciated:
> | x64 + arm64.

Added, thanks!


Iterative patch below


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
index 661e148e..9c27e12d 100644
--- 
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
@@ -173,7 +173,7 @@ local function read_mach_o(buf, is64)
    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])
-    arch = {
+    local arch = {
        cputype = be32(fat_arch.cputype),
        cpusubtype = be32(fat_arch.cpusubtype),
      }
@@ -184,16 +184,18 @@ local function read_mach_o(buf, is64)
  end

  -- Defined in <src/jit/bcsave.lua:bcsave_machobj>.
-local sum_cputype = {
+local SUM_CPUTYPE = {
    x86 = 7,
    x64 = 0x01000007,
    arm = 7 + 12,
+  -- x64 + arm64.
    arm64 = 0x01000007 + 0x0100000c,
  }
-local sum_cpusubtype = {
+local SUM_CPUSUBTYPE = {
    x86 = 3,
    x64 = 3,
    arm = 3 + 9,
+  -- x64 + arm64.
    arm64 = 3 + 0,
  }

@@ -257,8 +259,8 @@ local function build_and_check_mach_o(is64)
      total_cputype = total_cputype + mach_o.fat_arch[i].cputype
      total_cpusubtype = total_cpusubtype + mach_o.fat_arch[i].cpusubtype
    end
-  test:is(total_cputype, sum_cputype[arch], 'cputype is correct in 
Mach-O, ' .. arch)
-  test:is(total_cpusubtype, sum_cpusubtype[arch], 'cpusubtype is 
correct in Mach-O, ' .. arch)
+  test:is(total_cputype, SUM_CPUTYPE[arch], 'cputype is correct in 
Mach-O, ' .. arch)
+  test:is(total_cpusubtype, SUM_CPUSUBTYPE[arch], 'cpusubtype is 
correct in Mach-O, ' .. arch)
  end

  -- ARM

>
>>   }
>>   
>>   -- The function builds Mach-O FAT object file and retrieves
> <snipped>
>
>> -- 
>> 2.34.1
>>

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

Re: [Tarantool-patches] [PATCH luajit 4/4][v2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.

[Sergey Bronnikov via Tarantool-patches]

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


On 13.06.2024 18:40, Sergey Bronnikov via Tarantool-patches wrote:
>
> Hi, Sergey
>
>
> fixes applied and force-pushed
>
> On 12.04.2024 14:27, Sergey Kaplun via Tarantool-patches wrote:
>> Hi, Sergey!
>> Thanks for the fixes!
>> LGTM, with a several minor nits below.
>>
>> On 11.04.24, Sergey Bronnikov wrote:
>>> From: Mike Pall <mike>
>>>
>>> Thanks to Carlo Cabrera.
>>>
>>> (cherry picked from commit b98b37231bd2dcb79e10b0f974cefd91eb0d7b3a)
>>>
>>> Mach-O FAT object files generated by LuaJIT for ARM64 Had
>> Typo: s/Had/had/
> Fixed, thanks!
>>> an incorrect format due to the usage of the 32-bit version of
>>> FFI structure. This patch adds the 64-bit structure definition
>> Typo: s/FFI structure/the FFI structure/
> Fixed, thanks!
>>> and uses it for ARM64.
>>>
>>> Sergey Bronnikov:
>>> * added the description and the test for the problem
>>>
>>> Part of tarantool/tarantool#9595
>>> ---
>>>   src/jit/bcsave.lua                            | 14 ++++-
>>>   ...-865-cross-generation-mach-o-file.test.lua | 55 +++++++++++++++++--
>>>   2 files changed, 63 insertions(+), 6 deletions(-)
>>>
>> <snipped>
>>
>>>   
>>>     -- Mach-O FAT object header.
>>> @@ -221,9 +263,11 @@ end
>>>   --
>>>   local SUM_CPUTYPE = {
>>>     arm = 7 + 12,
>>> +  arm64 = 0x01000007 + 0x0100000c,
>> Minor: The comment will be appreciated:
>> | x64 + arm64.
> Added, thanks!
>>>   }
>>>   local SUM_CPUSUBTYPE = {
>>>     arm = 3 + 9,
>>> +  arm64 = 3 + 0,
>> Minor: The comment will be appreciated:
>> | x64 + arm64.
>
> Added, thanks!
>
>
> Iterative patch below
>
>
> 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
> index 661e148e..9c27e12d 100644
> --- 
> 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
> @@ -173,7 +173,7 @@ local function read_mach_o(buf, is64)
>    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])
> -    arch = {
> +    local arch = {
>        cputype = be32(fat_arch.cputype),
>        cpusubtype = be32(fat_arch.cpusubtype),
>      }
> @@ -184,16 +184,18 @@ local function read_mach_o(buf, is64)
>  end
>
>  -- Defined in <src/jit/bcsave.lua:bcsave_machobj>.
> -local sum_cputype = {
> +local SUM_CPUTYPE = {
>    x86 = 7,
>    x64 = 0x01000007,
>    arm = 7 + 12,
> +  -- x64 + arm64.
>    arm64 = 0x01000007 + 0x0100000c,
>  }
> -local sum_cpusubtype = {
> +local SUM_CPUSUBTYPE = {
>    x86 = 3,
>    x64 = 3,
>    arm = 3 + 9,
> +  -- x64 + arm64.
>    arm64 = 3 + 0,
>  }
>
> @@ -257,8 +259,8 @@ local function build_and_check_mach_o(is64)
>      total_cputype = total_cputype + mach_o.fat_arch[i].cputype
>      total_cpusubtype = total_cpusubtype + mach_o.fat_arch[i].cpusubtype
>    end
> -  test:is(total_cputype, sum_cputype[arch], 'cputype is correct in 
> Mach-O, ' .. arch)
> -  test:is(total_cpusubtype, sum_cpusubtype[arch], 'cpusubtype is 
> correct in Mach-O, ' .. arch)
> +  test:is(total_cputype, SUM_CPUTYPE[arch], 'cputype is correct in 
> Mach-O, ' .. arch)
> +  test:is(total_cpusubtype, SUM_CPUSUBTYPE[arch], 'cpusubtype is 
> correct in Mach-O, ' .. arch)
>  end
>
>  -- ARM
>
This is a correct iterative patch:


--- 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
@@ -262,13 +262,15 @@ end
  -- 1. 
https://opensource.apple.com/source/xnu/xnu-4570.41.2/osfmk/mach/machine.h.auto.html
  --
  local SUM_CPUTYPE = {
-  -- x86 + arm
+  -- x86 + arm.
    arm = 7 + 12,
+  -- x64 + arm64.
    arm64 = 0x01000007 + 0x0100000c,
  }
  local SUM_CPUSUBTYPE = {
-  -- x86 + arm
+  -- x86 + arm.
    arm = 3 + 9,
+  -- x64 + arm64.
    arm64 = 3 + 0,
  }

>
>>>   }
>>>   
>>>   -- The function builds Mach-O FAT object file and retrieves
>> <snipped>
>>
>>> -- 
>>> 2.34.1
>>>

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

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

[Sergey Bronnikov via Tarantool-patches]

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

Sergey,

On 16.04.2024 18:29, Sergey Bronnikov via Tarantool-patches wrote:
<snipped>
>> 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

removed additional argument with `hw_arch` and replaced with subtest.name:


@@ -293,7 +295,8 @@ local SUM_CPUSUBTYPE = {
  -- $ luajit -b -o osx -a arm64 empty.lua empty.o
  -- $ lipo -archs empty.o
  -- x86_64 arm64
-local function build_and_check_mach_o(subtest, hw_arch)
+local function build_and_check_mach_o(subtest)
+  local hw_arch = subtest.name
    assert(hw_arch == 'arm' or hw_arch == 'arm64')

    subtest:plan(4)
@@ -341,7 +344,7 @@ local function build_and_check_mach_o(subtest, hw_arch)
               'cpusubtype is correct in Mach-O')
  end

-test:test('arm', build_and_check_mach_o, 'arm')
-test:test('arm64', build_and_check_mach_o, 'arm64')
+test:test('arm', build_and_check_mach_o)
+test:test('arm64', build_and_check_mach_o)

  test:done(true)

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

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

Re: [Tarantool-patches] [PATCH luajit 1/4][v2] ci: add a workflow for testing with AVX512

[Sergey Bronnikov via Tarantool-patches]

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


On 05.05.2024 15:29, Maxim Kokryashkin wrote:
> Hi!
> On Thu, Apr 18, 2024 at 11:24:42AM UTC, Sergey Kaplun wrote:
>> Hi, Sergey
>>
>> On 16.04.24, Sergey Bronnikov wrote:
>>> Hi, Sergey
>>>
>>> On 12.04.2024 13:27, Sergey Kaplun wrote:
>> <snipped>
>>
>>>>> diff --git a/.github/actions/setup-linux/action.yml b/.github/actions/setup-linux/action.yml
>>>>> index f0171b83..19bdcfa2 100644
>>>>> --- a/.github/actions/setup-linux/action.yml
>>>>> +++ b/.github/actions/setup-linux/action.yml
>>>>> @@ -17,3 +17,15 @@ runs:
>>>>>            apt -y update
>>>>>            apt -y install cmake gcc make ninja-build perl
>>>>>          shell: bash
>>>>> +    - name: Detect a presence of AVX512
>>>> I suppose it shouldn't be a part of setup-linux action,
>>> It was requested by Max.
>> And what is the rationale for it?
>> Why don't use this detection during steps in our workflow?
>>
>> Hence, we avoid this unnecesary step for other jobs.
> I've suggested to move that to a separate action, not to the
> setup-linux.
Do you have objections to move this steps to workflow?
>
> <snipped>

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

Re: [Tarantool-patches] [PATCH luajit 1/4][v2] ci: add a workflow for testing with AVX512

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey


please see my comments. Fixes applied and force-pushed to the branch.

Sergey

On 18.04.2024 11:24, Sergey Kaplun wrote:
> Hi, Sergey
>
> On 16.04.24, Sergey Bronnikov wrote:
>> Hi, Sergey
>>
>> On 12.04.2024 13:27, Sergey Kaplun wrote:
> <snipped>
>
>>>> diff --git a/.github/actions/setup-linux/action.yml b/.github/actions/setup-linux/action.yml
>>>> index f0171b83..19bdcfa2 100644
>>>> --- a/.github/actions/setup-linux/action.yml
>>>> +++ b/.github/actions/setup-linux/action.yml
>>>> @@ -17,3 +17,15 @@ runs:
>>>>            apt -y update
>>>>            apt -y install cmake gcc make ninja-build perl
>>>>          shell: bash
>>>> +    - name: Detect a presence of AVX512
>>> I suppose it shouldn't be a part of setup-linux action,
>> It was requested by Max.
> And what is the rationale for it?
> Why don't use this detection during steps in our workflow?
>
> Hence, we avoid this unnecesary step for other jobs.
Detection steps moved to workflow.


>
>>
>>> but just a one
>>> step in testing-avx512.yml, see [1] for details of steps content usage.
>>>
>>> Is it possible or did I miss some GH-action API trick here?
>>> But anyway it should be done not in the same action.
>>>
> <snipped>
>
>>>> +        #
>>>> +        # Normally `grep` has the exit status is 0 if a line is
>>> Typo: s/Normally/Normally,/
>>> Also, "has the exit status is" has two verbs, so I suggest to
>>> reformulate this like the following:
>>>
>>> | Normally, the exit status of `grep` is 0 if a line is selected, 1 if
>>> | no lines were selected, and 2 if an error occurred.

Updated:

--- a/.github/workflows/avx512-build-testing.yml
+++ b/.github/workflows/avx512-build-testing.yml
@@ -40,13 +40,11 @@ jobs:
        - name: Detect a presence of AVX512
          id: avx512_script
          run: |
-          # Set the avx512_support environment variable to '0'
-          # when AVX512 is supported and '1' otherwise.
-          #
-          # Normally, `grep` has the exit status is 0 if a line is
-          # selected, 1 if no lines were selected, and 2 if an error
-          # occurred.
-          avx512_support=$(grep avx512f /proc/cpuinfo 2>&1 > 
/dev/null;  [[ $? == 0 ]] && echo 0 || echo 1)
+          # Set the avx512_support environment variable to an exit
+          # code returned by `grep`. The exit status of `grep` is
+          # 0 if lines were found, 1 if no lines were found, and 2
+          # if an error occurred.
+          avx512_support=$(grep avx512f /proc/cpuinfo 2>&1 > /dev/null; 
echo $?)
            echo "avx512_support=$avx512_support" >> $GITHUB_OUTPUT
          shell: bash
        - name: setup Linux
> Ping.
>
>>>> +        # selected, 1 if no lines were selected, and 2 if an error
>>>> +        # occurred.
>>>> +        avx512_support=$(grep avx /proc/cpuinfo 2>&1 > /dev/null;  [[ $? == 0 ]] && echo 0 || echo 1)
>>> Why not just echo the "$?"?
removed all "tricks" and left "echo $?" as requested
>> because comment above. `grep` has three possible values.
> Yes, but why do we need such tricks if we are still interested in the 0
> value only? Also, with this and the logging of the `avx512_support`, we
> may see the reason why the workflow was skipped: an incorrect `grep`
> command (for some reason) or a not-founded CPU feature flag.
>
> Hence, if the command fails with exit code 2, that means that our CI
> settings are incorrect and need to be adjusted.
>
> <snipped>
>
>>>> +        echo "avx512_support=$avx512_support" >> $GITHUB_OUTPUT
>>> Minor: It will be nice to echo some message about the value of this
>>> variable. Hence, we can check this from the GitHub logs.
>> What for? Job will be skipped if there is no AVX512 support.
>>
>> What is the value of the message?
> See the rationale above.
>
>>
> <snipped>
>
>>>> +        if: needs.avx512_script.outputs.avx512_support == 0
>>>> +        run: >
>>>> +          cmake -S . -B ${{ env.BUILDDIR }}
>>>> +          -G Ninja
>>>> +          -DCMAKE_BUILD_TYPE=RelWithDebInfo
>>> Please add a comment. Why do we check the non-debug build here?
>>> If there is no reason for it, maybe it is better to check both: the
>>> debug and release builds within a simple matrix.
>> There is no reason for this.
> So, let's check the Debug build only. And with enabled assertions.
> They are useful for testing.

replaced with Debug


>
>>>> +          -DCMAKE_C_FLAGS=-march=skylake-avx512
>>>> +          -DCMAKE_C_COMPILER=gcc
>>> Do we need to specify compiler manually?
removed CMAKE_C_COMPILER at all
>> Yes, CFLAG is gcc-specific.
> Emm, is it?
>
> | $ clang -march=skylake-avx512 /tmp/t.c -o /tmp/t.exe
> | $  clang --version
> | clang version 17.0.6
> | Target: x86_64-pc-linux-gnu
> | Thread model: posix
> | InstalledDir: /usr/lib/llvm/17/bin
> | Configuration file: /etc/clang/x86_64-pc-linux-gnu-clang.cfg
>
> Maybe it is unsupported for clang version we used in CI?
>
>>>> +      - name: build
>>>> +        if: needs.avx512_script.outputs.avx512_support == 0
>>>> +        run: cmake --build ${{ env.BUILDDIR }} --parallel
>>>> +      - name: run regression tests
>>>> +        if: needs.avx512_script.outputs.avx512_support == 0
>>>> +        run: cmake --build ${{ env.BUILDDIR }} --parallel --target LuaJIT-test
>>>> diff --git a/test/LuaJIT-tests/lib/ffi/bit64.lua b/test/LuaJIT-tests/lib/ffi/bit64.lua
>>>> index d1b47bef..cf3a96eb 100644
>>>> --- a/test/LuaJIT-tests/lib/ffi/bit64.lua
>>>> +++ b/test/LuaJIT-tests/lib/ffi/bit64.lua
>>>> @@ -41,7 +41,7 @@ do --- tobit/band assorted C types
>>>>      end
>>>>    end
>>>>    
>>>> -do --- tobit/band negative unsigned enum
>>>> +do --- tobit/band negative unsigned enum -avx512
>>> Should we also to set such variable in the CMakeLists.txt when we met
>>> the corresponding condition?
>> Yes, double check.
> I meant that part of the third commit should be done within this patch,
> as you've already fixed.
Moved.
>
>>>>      local x = ffi.new("uenum_t", -10)
>>>>      local y = tobit(x)
>>>>      local z = band(x)
>>>> -- 
>>>> 2.34.1
>>> [1]:https://docs.github.com/en/actions/learn-github-actions/contexts#example-usage-of-the-steps-context
>>> [2]:https://www.laruence.com/x86/VCVTTSD2USI.html
>>>

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

Re: [Tarantool-patches] [PATCH luajit 0/4][v2] Mach-O generation fixes

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/4][v2] test: introduce a helper read_file

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey

On 12.04.2024 13:47, Sergey Kaplun wrote:


<snipped>

>> +function M.read_file(path)
>> +  local file = assert(io.open(path), 'cannot open an object file')

Fixed and force-pushed a message in assert:


--- a/test/tarantool-tests/utils/tools.lua
+++ b/test/tarantool-tests/utils/tools.lua
@@ -15,7 +15,7 @@ end
  -- Reads a file located at a specified path and returns its
  -- content.
  function M.read_file(path)
-  local file = assert(io.open(path), 'cannot open an object file')
+  local file = assert(io.open(path), 'cannot open a file')
    local content = file:read('*a')
file:close()
    return content


>> +  local content =file:read('*a')
>> +file:close()
>> +  return content
>> +end
>> +
>>   return M
>> -- 
>> 2.34.1
>>

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

Re: [Tarantool-patches] [PATCH luajit 0/4][v2] Mach-O generation fixes

[Sergey Kaplun via Tarantool-patches]

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master [1], release/3.1 [2]
and release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/10200
[2]: https://github.com/tarantool/tarantool/pull/10201
[3]: https://github.com/tarantool/tarantool/pull/10202

-- 
Best regards,
Sergey Kaplun