* [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes
@ 2024-03-14 11:39 Sergey Bronnikov via Tarantool-patches
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files Sergey Bronnikov via Tarantool-patches
` (2 more replies)
0 siblings, 3 replies; 24+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-03-14 11:39 UTC (permalink / raw)
To: tarantool-patches, Sergey Kaplun, Maxim Kokryashkin
From: Sergey Bronnikov <sergeyb@tarantool.org>
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.
.github/workflows/exotic-builds-testing.yml | 5 +-
src/jit/bcsave.lua | 20 +-
.../lj-366-strtab-correct-size.test.lua | 10 +-
...generation_of_mach-o_object_files.test.lua | 273 ++++++++++++++++++
test/tarantool-tests/utils/tools.lua | 8 +
5 files changed, 302 insertions(+), 14 deletions(-)
create mode 100644 test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
--
2.34.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.
2024-03-14 11:39 [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes Sergey Bronnikov via Tarantool-patches
@ 2024-03-14 11:39 ` Sergey Bronnikov via Tarantool-patches
2024-03-18 12:53 ` Maxim Kokryashkin via Tarantool-patches
2024-03-18 12:55 ` Maxim Kokryashkin via Tarantool-patches
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file Sergey Bronnikov via Tarantool-patches
2024-04-08 7:47 ` [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes Sergey Kaplun via Tarantool-patches
2 siblings, 2 replies; 24+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-03-14 11:39 UTC (permalink / raw)
To: tarantool-patches, Sergey Kaplun, Maxim Kokryashkin
From: sergeyb@tarantool.org
Thanks to Carlo Cabrera.
(cherry picked from commit 3065c910ad6027031aabe2dfd3c26a3d0f014b4f)
Mach-O FAT object constructed by LuaJIT had an incorrect format. The
problem is reproduced when target hardware platform has AVX512F and
LuaJIT is compiled with enabled AVX512F instructions.
The problem is arise because LuaJIT FFI code for Mach-O file generation
in `bcsave.lua` relied 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 and a test for the problem
Part of tarantool/tarantool#9595
---
.github/workflows/exotic-builds-testing.yml | 5 +-
src/jit/bcsave.lua | 6 +-
.../lj-366-strtab-correct-size.test.lua | 10 +-
...generation_of_mach-o_object_files.test.lua | 271 ++++++++++++++++++
test/tarantool-tests/utils/tools.lua | 8 +
5 files changed, 287 insertions(+), 13 deletions(-)
create mode 100644 test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
index a9ba5fd5..df4bc2e9 100644
--- a/.github/workflows/exotic-builds-testing.yml
+++ b/.github/workflows/exotic-builds-testing.yml
@@ -32,6 +32,7 @@ jobs:
fail-fast: false
matrix:
BUILDTYPE: [Debug, Release]
+ OS: [Linux, macOS]
ARCH: [ARM64, x86_64]
GC64: [ON, OFF]
FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
@@ -50,13 +51,15 @@ jobs:
FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
- FLAVOR: nounwind
FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
+ - FLAVOR: avx512
+ CMAKEFLAGS: -DCMAKE_C_FLAGS=skylake-avx512 -DCMAKE_C_COMPILER=gcc
exclude:
- ARCH: ARM64
GC64: OFF
# DUALNUM is default for ARM64, no need for additional testing.
- FLAVOR: dualnum
ARCH: ARM64
- runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
+ runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}', '${ matrix.OS }']
name: >
LuaJIT ${{ matrix.FLAVOR }}
(Linux/${{ matrix.ARCH }})
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/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
index 8a97a441..0bb92da6 100644
--- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
+++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
@@ -138,14 +138,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 +164,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/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
new file mode 100644
index 00000000..0519e134
--- /dev/null
+++ b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
@@ -0,0 +1,271 @@
+local tap = require('tap')
+local test = tap.test('lj-865-fix_generation_of_mach-o_object_files'):skipcond({
+ -- XXX: Tarantool doesn't use default LuaJIT loaders, and Lua
+ -- bytecode can't be loaded from the shared library. For more
+ -- info: https://github.com/tarantool/tarantool/issues/9671.
+ -- luacheck: no global
+ ['Test uses exotic type of loaders (see #9671)'] = _TARANTOOL,
+})
+
+test:plan(4)
+
+-- Test creates an object file in Mach-O format with LuaJIT bytecode
+-- and checks validness of the object file fields.
+--
+-- The original problem is reproduced with LuaJIT that built with
+-- enabled AVX512F instructions. The support of 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 CPU codename.
+-- Please consult for available model architecture on GCC Online
+-- Documentation [1] for available CPU codenames. To detect
+-- CPU codename execute `gcc -march=native -Q --help=target | grep march`.
+--
+-- 1. https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
+--
+-- Manual steps for reproducing are the following:
+--
+-- $ make CC=gcc TARGET_CFLAGS='skylake-avx512' -f Makefile.original
+-- $ 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')
+
+-- Format of the Mach-O is described in a document
+-- "OS X ABI Mach-O File Format Reference", published by Apple company.
+-- Copy of the (now removed) official documentation in [1].
+-- Yet another source of thruth is a XNU headers, see the definition
+-- of C-structures in: [2] (`nlist_64`), [3] (`fat_arch` and `fat_header`).
+
+-- 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
+--
+-- 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
+{
+ mach_header; uint32_t reserved;
+} mach_header_64;
+
+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 {
+ 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;
+ uint32_t offset, align, reloff, nreloc, flags;
+ 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;
+
+typedef struct {
+ int32_t strx;
+ uint8_t type, sect;
+ int16_t desc;
+ uint32_t value;
+} mach_nlist;
+
+typedef struct {
+ uint32_t strx;
+ uint8_t type, sect;
+ uint16_t desc;
+ uint64_t value;
+} mach_nlist_64;
+
+typedef struct
+{
+ uint32_t magic, nfat_arch;
+} mach_fat_header;
+
+typedef struct
+{
+ uint32_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;
+
+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)
+ local mach_o_path = os.tmpname() .. '.o'
+ local lua_path = os.getenv('LUA_PATH')
+ local lua_bin = require('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 an Mach-O format and returns
+-- an fat magic number and nfat_arch.
+local function read_mach_o(buf, is64)
+ local res = {
+ header = {
+ magic = 0,
+ nfat_arch = 0,
+ },
+ fat_arch = {
+ },
+ }
+
+ -- Mach-O FAT object.
+ 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.
+ local mach_fat_header_type = ffi.typeof('mach_fat_header *')
+ local mach_fat_header = ffi.cast(mach_fat_header_type, obj.fat)
+ local be32 = bit.bswap -- Mach-O FAT is BE, target arch is LE.
+ res.header.magic = be32(mach_fat_header.magic)
+ res.header.nfat_arch = be32(mach_fat_header.nfat_arch)
+
+ -- Mach-O FAT object archs.
+ 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 = {
+ cputype = be32(fat_arch.cputype),
+ cpusubtype = be32(fat_arch.cpusubtype),
+ }
+ table.insert(res.fat_arch, arch)
+ end
+
+ return res
+end
+
+-- Defined in <src/jit/bcsave.lua:bcsave_machobj>.
+local sum_cputype = {
+ x86 = 7,
+ x64 = 0x01000007,
+ arm = 7 + 12,
+ arm64 = 0x01000007 + 0x0100000c,
+}
+local sum_cpusubtype = {
+ x86 = 3,
+ x64 = 3,
+ arm = 3 + 9,
+ arm64 = 3 + 0,
+}
+
+-- The function builds Mach-O FAT object file and retrieves
+-- its header fields (magic and nfat_arch)
+-- and fields of the each arch (cputype, cpusubtype).
+--
+-- Mach-O FAT object header could be retrieved with `otool` on macOS:
+--
+-- $ otool -f empty.o
+-- Fat headers
+-- fat_magic 0xcafebabe
+-- nfat_arch 2
+-- <snipped>
+--
+-- CPU type and subtype could 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(is64)
+ local arch = is64 and 'arm64' or 'arm'
+
+ -- FAT_MAGIC is an integer containing the value 0xCAFEBABE in
+ -- big-endian byte order format. On a big-endian host CPU,
+ -- this can be validated using the constant FAT_MAGIC;
+ -- on a little-endian host CPU, it can be validated using
+ -- the constant FAT_CIGAM.
+ --
+ -- FAT_NARCH is an integer specifying the number of fat_arch
+ -- data structures that follow. This is the number of
+ -- architectures contained in this binary.
+ --
+ -- See aforementioned "OS X ABI Mach-O File Format Reference".
+ --
+ local FAT_MAGIC = '0xffffffffcafebabe'
+ local FAT_NARCH = 2
+
+ local MODULE_NAME = 'lango_team'
+
+ local mach_o_obj_path = create_obj_file(MODULE_NAME, arch)
+ local mach_o_buf = require('utils').tools.read_file(mach_o_obj_path)
+ assert(mach_o_buf == nil or #mach_o_buf ~= 0, 'cannot read an object file')
+
+ local mach_o = read_mach_o(mach_o_buf, is64)
+
+ -- Teardown.
+ local retcode = os.remove(mach_o_obj_path)
+ assert(retcode == true, 'remove an object file')
+
+ local magic_str = string.format('0x%02x', mach_o.header.magic)
+ test:is(magic_str, FAT_MAGIC, 'fat_magic is correct in Mach-O, ' .. arch)
+ test:is(mach_o.header.nfat_arch, FAT_NARCH, 'nfat_arch is correct in Mach-O, ' .. arch)
+
+ local total_cputype = 0
+ local total_cpusubtype = 0
+ for i = 1, mach_o.header.nfat_arch do
+ 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)
+end
+
+-- ARM
+build_and_check_mach_o(false)
+
+test:done(true)
diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua
index f35c6922..26b8c08d 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 in 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.
2024-03-14 11:39 [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes Sergey Bronnikov via Tarantool-patches
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files Sergey Bronnikov via Tarantool-patches
@ 2024-03-14 11:39 ` Sergey Bronnikov via Tarantool-patches
2024-03-18 13:44 ` Maxim Kokryashkin via Tarantool-patches
2024-04-08 15:16 ` Sergey Kaplun via Tarantool-patches
2024-04-08 7:47 ` [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes Sergey Kaplun via Tarantool-patches
2 siblings, 2 replies; 24+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-03-14 11:39 UTC (permalink / raw)
To: tarantool-patches, Sergey Kaplun, Maxim Kokryashkin
From: sergeyb@tarantool.org
Thanks to Carlo Cabrera.
(cherry picked from commit b98b37231bd2dcb79e10b0f974cefd91eb0d7b3a)
Mach-O FAR object files generated by LuaJIT for arm64 had an incorrect
format because FFI structure used for generation was wrong:
`mach_fat_obj` instead of `mach_fat_obj_64`.
Sergey Bronnikov:
* added the description and a test for the problem
Part of tarantool/tarantool#9595
---
src/jit/bcsave.lua | 14 +++++++++++++-
...-fix_generation_of_mach-o_object_files.test.lua | 4 +++-
2 files changed, 16 insertions(+), 2 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-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 0519e134..0a11f163 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
@@ -7,7 +7,7 @@ local test = tap.test('lj-865-fix_generation_of_mach-o_object_files'):skipcond({
['Test uses exotic type of loaders (see #9671)'] = _TARANTOOL,
})
-test:plan(4)
+test:plan(8)
-- Test creates an object file in Mach-O format with LuaJIT bytecode
-- and checks validness of the object file fields.
@@ -267,5 +267,7 @@ end
-- ARM
build_and_check_mach_o(false)
+-- ARM64
+build_and_check_mach_o(true)
test:done(true)
--
2.34.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files Sergey Bronnikov via Tarantool-patches
@ 2024-03-18 12:53 ` Maxim Kokryashkin via Tarantool-patches
2024-03-19 8:19 ` Sergey Bronnikov via Tarantool-patches
2024-03-18 12:55 ` Maxim Kokryashkin via Tarantool-patches
1 sibling, 1 reply; 24+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-03-18 12:53 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Hi, Sergey, thanks for the patch!
Please consider my comments below.
On Thu, Mar 14, 2024 at 02:39:49PM +0300, Sergey Bronnikov wrote:
> From: sergeyb@tarantool.org
>
> Thanks to Carlo Cabrera.
>
> (cherry picked from commit 3065c910ad6027031aabe2dfd3c26a3d0f014b4f)
>
> Mach-O FAT object constructed by LuaJIT had an incorrect format. The
Typo: s/Mach-O FAT/The Mach-O FAT/
> problem is reproduced when target hardware platform has AVX512F and
Typo:s/target/the target/
> LuaJIT is compiled with enabled AVX512F instructions.
>
> The problem is arise because LuaJIT FFI code for Mach-O file generation
Typo: s/is arise/arises/
> in `bcsave.lua` relied on undefined behavior for conversions to
Typo: s/relied/relies/
> `uint32_t`. AVX512F has the `vcvttsd2usi` instruction which converts
Typo: s/instruction/instruction,/
> `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 and a test for the problem
Typo: s/a test/the test/
>
> Part of tarantool/tarantool#9595
> ---
> .github/workflows/exotic-builds-testing.yml | 5 +-
> src/jit/bcsave.lua | 6 +-
> .../lj-366-strtab-correct-size.test.lua | 10 +-
> ...generation_of_mach-o_object_files.test.lua | 271 ++++++++++++++++++
> test/tarantool-tests/utils/tools.lua | 8 +
> 5 files changed, 287 insertions(+), 13 deletions(-)
> create mode 100644 test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
>
> diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
> index a9ba5fd5..df4bc2e9 100644
> --- a/.github/workflows/exotic-builds-testing.yml
> +++ b/.github/workflows/exotic-builds-testing.yml
> @@ -32,6 +32,7 @@ jobs:
> fail-fast: false
> matrix:
> BUILDTYPE: [Debug, Release]
> + OS: [Linux, macOS]
> ARCH: [ARM64, x86_64]
> GC64: [ON, OFF]
> FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
> @@ -50,13 +51,15 @@ jobs:
> FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
https://github.com/tarantool/luajit/actions/runs/8279362128
> - FLAVOR: nounwind
> FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
> + - FLAVOR: avx512
> + CMAKEFLAGS: -DCMAKE_C_FLAGS=skylake-avx512 -DCMAKE_C_COMPILER=gcc
> exclude:
> - ARCH: ARM64
> GC64: OFF
> # DUALNUM is default for ARM64, no need for additional testing.
> - FLAVOR: dualnum
> ARCH: ARM64
> - runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
> + runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}', '${ matrix.OS }']
The matrix.OS variable should be wrapped with double curly braces,
instead of singular ones. So, it should be like this:
| '${{ matrix.OS }}'
Currently, exotic build testing fails to start because of this mistake.
https://github.com/tarantool/luajit/actions/runs/8279362128
> name: >
> LuaJIT ${{ matrix.FLAVOR }}
> (Linux/${{ matrix.ARCH }})
> 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/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> index 8a97a441..0bb92da6 100644
> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
Let's move this update to a separate patch alongside with the added
utility function. Currently, this change in unrelated test file is
a bit confusing.
> @@ -138,14 +138,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 +164,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/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
> new file mode 100644
> index 00000000..0519e134
> --- /dev/null
> +++ b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
> @@ -0,0 +1,271 @@
> +local tap = require('tap')
> +local test = tap.test('lj-865-fix_generation_of_mach-o_object_files'):skipcond({
> + -- XXX: Tarantool doesn't use default LuaJIT loaders, and Lua
> + -- bytecode can't be loaded from the shared library. For more
Typo: s/the shared/a shared/
> + -- info: https://github.com/tarantool/tarantool/issues/9671.
> + -- luacheck: no global
> + ['Test uses exotic type of loaders (see #9671)'] = _TARANTOOL,
Typo: s/uses exotic/uses an exotic/
> +})
> +
> +test:plan(4)
> +
> +-- Test creates an object file in Mach-O format with LuaJIT bytecode
Typo: s/Test/The test/
> +-- and checks validness of the object file fields.
Typo: s/validness/the validity/
> +--
> +-- The original problem is reproduced with LuaJIT that built with
> +-- enabled AVX512F instructions. The support of AVX512F could be
Typo: s/support of/support for/
> +-- checked in `/proc/cpuinfo` on Linux and
> +-- `sysctl hw.optional.avx512f` on Mac. AVX512F must be
> +-- implicitly enabled in a C compiler by passing CPU codename.
> +-- Please consult for available model architecture on GCC Online
Typo: s/for/the/
> +-- Documentation [1] for available CPU codenames. To detect
> +-- CPU codename execute `gcc -march=native -Q --help=target | grep march`.
> +--
> +-- 1. https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
> +--
> +-- Manual steps for reproducing are the following:
> +--
> +-- $ make CC=gcc TARGET_CFLAGS='skylake-avx512' -f Makefile.original
> +-- $ 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')
> +
> +-- Format of the Mach-O is described in a document
Typo: s/a document/the document/
> +-- "OS X ABI Mach-O File Format Reference", published by Apple company.
> +-- Copy of the (now removed) official documentation in [1].
Let's replace "in [1]" with "can be found here [1].".
> +-- Yet another source of thruth is a XNU headers, see the definition
Typo: s/is a XNU/are XNU/
Typo: s/the definition/definitions/
> +-- of C-structures in: [2] (`nlist_64`), [3] (`fat_arch` and `fat_header`).
> +
> +-- 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
> +--
> +-- 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
> +{
> + mach_header; uint32_t reserved;
> +} mach_header_64;
> +
> +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 {
> + 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;
> + uint32_t offset, align, reloff, nreloc, flags;
> + 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;
> +
> +typedef struct {
> + int32_t strx;
> + uint8_t type, sect;
> + int16_t desc;
> + uint32_t value;
> +} mach_nlist;
> +
> +typedef struct {
> + uint32_t strx;
> + uint8_t type, sect;
> + uint16_t desc;
> + uint64_t value;
> +} mach_nlist_64;
> +
> +typedef struct
> +{
> + uint32_t magic, nfat_arch;
> +} mach_fat_header;
> +
> +typedef struct
> +{
> + uint32_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;
> +
> +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)
> + local mach_o_path = os.tmpname() .. '.o'
> + local lua_path = os.getenv('LUA_PATH')
> + local lua_bin = require('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 an Mach-O format and returns
Typo: s/in an/in the/
> +-- an fat magic number and nfat_arch.
Typo: s/an fat/the FAT/
Typo: s/nfat_arch/`nfat_arch`
> +local function read_mach_o(buf, is64)
> + local res = {
> + header = {
> + magic = 0,
> + nfat_arch = 0,
> + },
> + fat_arch = {
> + },
I guess, that the formatting below is a bit better here:
| fat_arch = {},
> + }
> +
> + -- Mach-O FAT object.
> + local mach_fat_obj_type = ffi.typeof(is64 and 'mach_fat_obj_64 *' or 'mach_fat_obj *')
The line is longer than 80 symbols.
> + local obj = ffi.cast(mach_fat_obj_type, buf)
> +
> + -- Mach-O FAT object header.
> + local mach_fat_header_type = ffi.typeof('mach_fat_header *')
> + local mach_fat_header = ffi.cast(mach_fat_header_type, obj.fat)
> + local be32 = bit.bswap -- Mach-O FAT is BE, target arch is LE.
> + res.header.magic = be32(mach_fat_header.magic)
> + res.header.nfat_arch = be32(mach_fat_header.nfat_arch)
> +
> + -- Mach-O FAT object archs.
Typo: s/archs/arches/
Side note: I feel like the comments for the sections are not elaborate
enough for unprepared reader. I think you should briefly desribe the
basic structure of a FAT object (FAT header, then array of per-segment
headers, then object files)
> + 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 = {
> + cputype = be32(fat_arch.cputype),
> + cpusubtype = be32(fat_arch.cpusubtype),
> + }
> + table.insert(res.fat_arch, arch)
> + end
> +
> + return res
> +end
> +
> +-- Defined in <src/jit/bcsave.lua:bcsave_machobj>.
> +local sum_cputype = {
> + x86 = 7,
> + x64 = 0x01000007,
> + arm = 7 + 12,
> + arm64 = 0x01000007 + 0x0100000c,
> +}
> +local sum_cpusubtype = {
> + x86 = 3,
> + x64 = 3,
> + arm = 3 + 9,
> + arm64 = 3 + 0,
> +}
It would be nice to have an explanation for these magic numbers.
> +
> +-- The function builds Mach-O FAT object file and retrieves
> +-- its header fields (magic and nfat_arch)
> +-- and fields of the each arch (cputype, cpusubtype).
> +--
> +-- Mach-O FAT object header could be retrieved with `otool` on macOS:
Typo: s/could be/can be/
> +--
> +-- $ otool -f empty.o
> +-- Fat headers
> +-- fat_magic 0xcafebabe
> +-- nfat_arch 2
> +-- <snipped>
> +--
> +-- CPU type and subtype could be retrieved with `lipo` on macOS:
Typo: s/could be/can be/
> +--
> +-- $ 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(is64)
> + local arch = is64 and 'arm64' or 'arm'
> +
> + -- FAT_MAGIC is an integer containing the value 0xCAFEBABE in
> + -- big-endian byte order format. On a big-endian host CPU,
> + -- this can be validated using the constant FAT_MAGIC;
> + -- on a little-endian host CPU, it can be validated using
> + -- the constant FAT_CIGAM.
> + --
> + -- FAT_NARCH is an integer specifying the number of fat_arch
> + -- data structures that follow. This is the number of
> + -- architectures contained in this binary.
> + --
> + -- See aforementioned "OS X ABI Mach-O File Format Reference".
> + --
> + local FAT_MAGIC = '0xffffffffcafebabe'
> + local FAT_NARCH = 2
> +
> + local MODULE_NAME = 'lango_team'
> +
> + local mach_o_obj_path = create_obj_file(MODULE_NAME, arch)
> + local mach_o_buf = require('utils').tools.read_file(mach_o_obj_path)
> + assert(mach_o_buf == nil or #mach_o_buf ~= 0, 'cannot read an object file')
> +
> + local mach_o = read_mach_o(mach_o_buf, is64)
> +
> + -- Teardown.
> + local retcode = os.remove(mach_o_obj_path)
> + assert(retcode == true, 'remove an object file')
> +
> + local magic_str = string.format('0x%02x', mach_o.header.magic)
> + test:is(magic_str, FAT_MAGIC, 'fat_magic is correct in Mach-O, ' .. arch)
> + test:is(mach_o.header.nfat_arch, FAT_NARCH, 'nfat_arch is correct in Mach-O, ' .. arch)
> +
> + local total_cputype = 0
> + local total_cpusubtype = 0
> + for i = 1, mach_o.header.nfat_arch do
> + 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)
> +end
> +
> +-- ARM
> +build_and_check_mach_o(false)
> +
> +test:done(true)
Please mention that alongside with test and fix for the issue
you've added this tool. IMO, it would be even better to do that
in a separate commit, to avoid confusion because of the updates in
test files unrelated to the patch.
> diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua
> index f35c6922..26b8c08d 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 in 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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files Sergey Bronnikov via Tarantool-patches
2024-03-18 12:53 ` Maxim Kokryashkin via Tarantool-patches
@ 2024-03-18 12:55 ` Maxim Kokryashkin via Tarantool-patches
1 sibling, 0 replies; 24+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-03-18 12:55 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Sorry folks, I forgot to mention one more thing, that concerns me:
we have no way to make sure that we test this case on an AVX512f
platform. We don't have out own runners with AVX512f support, and
there is no way to specify them in GitHub Actions. I believe that
we should mention that in the commit message, or in a comment in
the workflow file.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file Sergey Bronnikov via Tarantool-patches
@ 2024-03-18 13:44 ` Maxim Kokryashkin via Tarantool-patches
2024-03-19 8:22 ` Sergey Bronnikov via Tarantool-patches
2024-04-08 15:16 ` Sergey Kaplun via Tarantool-patches
1 sibling, 1 reply; 24+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-03-18 13:44 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On Thu, Mar 14, 2024 at 02:39:51PM +0300, Sergey Bronnikov wrote:
> From: sergeyb@tarantool.org
>
> Thanks to Carlo Cabrera.
>
> (cherry picked from commit b98b37231bd2dcb79e10b0f974cefd91eb0d7b3a)
>
> Mach-O FAR object files generated by LuaJIT for arm64 had an incorrect
Typo: s/FAR/FAT/
Typo: s/arm64/ARM64/
> format because FFI structure used for generation was wrong:
Typo: s/FFI/The FFI/
> `mach_fat_obj` instead of `mach_fat_obj_64`.
>
> Sergey Bronnikov:
> * added the description and a test for the problem
Typo: s/a test/the test/
>
> Part of tarantool/tarantool#9595
> ---
> src/jit/bcsave.lua | 14 +++++++++++++-
> ...-fix_generation_of_mach-o_object_files.test.lua | 4 +++-
> 2 files changed, 16 insertions(+), 2 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-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 0519e134..0a11f163 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
> @@ -7,7 +7,7 @@ local test = tap.test('lj-865-fix_generation_of_mach-o_object_files'):skipcond({
> ['Test uses exotic type of loaders (see #9671)'] = _TARANTOOL,
> })
>
> -test:plan(4)
> +test:plan(8)
>
> -- Test creates an object file in Mach-O format with LuaJIT bytecode
> -- and checks validness of the object file fields.
> @@ -267,5 +267,7 @@ end
>
> -- ARM
> build_and_check_mach_o(false)
> +-- ARM64
> +build_and_check_mach_o(true)
These `true/false` should be explained as platform toggle.
An even better solution would be to pass the platform name
explicitly.
>
> test:done(true)
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.
2024-03-18 12:53 ` Maxim Kokryashkin via Tarantool-patches
@ 2024-03-19 8:19 ` Sergey Bronnikov via Tarantool-patches
2024-03-19 16:28 ` Maxim Kokryashkin via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-03-19 8:19 UTC (permalink / raw)
To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
Max,
thanks for review! see comments below
On 3/18/24 15:53, Maxim Kokryashkin wrote:
> Hi, Sergey, thanks for the patch!
> Please consider my comments below.
> On Thu, Mar 14, 2024 at 02:39:49PM +0300, Sergey Bronnikov wrote:
>> From: sergeyb@tarantool.org
>>
>> Thanks to Carlo Cabrera.
>>
>> (cherry picked from commit 3065c910ad6027031aabe2dfd3c26a3d0f014b4f)
>>
>> Mach-O FAT object constructed by LuaJIT had an incorrect format. The
> Typo: s/Mach-O FAT/The Mach-O FAT/
Fixed.
>> problem is reproduced when target hardware platform has AVX512F and
> Typo:s/target/the target/
Fixed.
>> LuaJIT is compiled with enabled AVX512F instructions.
>>
>> The problem is arise because LuaJIT FFI code for Mach-O file generation
> Typo: s/is arise/arises/
Fixed.
>> in `bcsave.lua` relied on undefined behavior for conversions to
> Typo: s/relied/relies/
Fixed.
>> `uint32_t`. AVX512F has the `vcvttsd2usi` instruction which converts
> Typo: s/instruction/instruction,/
Fixed.
>> `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 and a test for the problem
> Typo: s/a test/the test/
Fixed.
>> Part of tarantool/tarantool#9595
>> ---
>> .github/workflows/exotic-builds-testing.yml | 5 +-
>> src/jit/bcsave.lua | 6 +-
>> .../lj-366-strtab-correct-size.test.lua | 10 +-
>> ...generation_of_mach-o_object_files.test.lua | 271 ++++++++++++++++++
>> test/tarantool-tests/utils/tools.lua | 8 +
>> 5 files changed, 287 insertions(+), 13 deletions(-)
>> create mode 100644 test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
>>
>> diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
>> index a9ba5fd5..df4bc2e9 100644
>> --- a/.github/workflows/exotic-builds-testing.yml
>> +++ b/.github/workflows/exotic-builds-testing.yml
>> @@ -32,6 +32,7 @@ jobs:
>> fail-fast: false
>> matrix:
>> BUILDTYPE: [Debug, Release]
>> + OS: [Linux, macOS]
>> ARCH: [ARM64, x86_64]
>> GC64: [ON, OFF]
>> FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
>> @@ -50,13 +51,15 @@ jobs:
>> FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
> https://github.com/tarantool/luajit/actions/runs/8279362128
>> - FLAVOR: nounwind
>> FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
>> + - FLAVOR: avx512
>> + CMAKEFLAGS: -DCMAKE_C_FLAGS=skylake-avx512 -DCMAKE_C_COMPILER=gcc
>> exclude:
>> - ARCH: ARM64
>> GC64: OFF
>> # DUALNUM is default for ARM64, no need for additional testing.
>> - FLAVOR: dualnum
>> ARCH: ARM64
>> - runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
>> + runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}', '${ matrix.OS }']
> The matrix.OS variable should be wrapped with double curly braces,
> instead of singular ones. So, it should be like this:
> | '${{ matrix.OS }}'
Fixed, thanks!
> Currently, exotic build testing fails to start because of this mistake.
> https://github.com/tarantool/luajit/actions/runs/8279362128
>> name: >
>> LuaJIT ${{ matrix.FLAVOR }}
>> (Linux/${{ matrix.ARCH }})
>> 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/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> index 8a97a441..0bb92da6 100644
>> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> Let's move this update to a separate patch alongside with the added
> utility function. Currently, this change in unrelated test file is
> a bit confusing.
Moved to a separate patch:
commit 138bcda850ed4aad4bbbae4d30315d9a0934cb26
Author: Sergey Bronnikov <sergeyb@tarantool.org>
Date: Tue Mar 19 09:56:51 2024 +0300
test: introduce a helper read_file
The test `lj-366-strtab-correct-size.test.lua` has a test helper
`read_file` that read 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 a test tools.
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..0bb92da6 100644
--- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
+++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
@@ -138,14 +138,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 +164,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..26b8c08d 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 in 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
>> @@ -138,14 +138,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 +164,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/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
>> new file mode 100644
>> index 00000000..0519e134
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
>> @@ -0,0 +1,271 @@
>> +local tap = require('tap')
>> +local test = tap.test('lj-865-fix_generation_of_mach-o_object_files'):skipcond({
>> + -- XXX: Tarantool doesn't use default LuaJIT loaders, and Lua
>> + -- bytecode can't be loaded from the shared library. For more
> Typo: s/the shared/a shared/
Fixed.
>> + -- info: https://github.com/tarantool/tarantool/issues/9671.
>> + -- luacheck: no global
>> + ['Test uses exotic type of loaders (see #9671)'] = _TARANTOOL,
> Typo: s/uses exotic/uses an exotic/
Fixed.
>> +})
>> +
>> +test:plan(4)
>> +
>> +-- Test creates an object file in Mach-O format with LuaJIT bytecode
> Typo: s/Test/The test/
Fixed.
>> +-- and checks validness of the object file fields.
> Typo: s/validness/the validity/
Fixed.
>> +--
>> +-- The original problem is reproduced with LuaJIT that built with
>> +-- enabled AVX512F instructions. The support of AVX512F could be
> Typo: s/support of/support for/
Fixed.
>> +-- checked in `/proc/cpuinfo` on Linux and
>> +-- `sysctl hw.optional.avx512f` on Mac. AVX512F must be
>> +-- implicitly enabled in a C compiler by passing CPU codename.
>> +-- Please consult for available model architecture on GCC Online
> Typo: s/for/the/
Fixed.
>> +-- Documentation [1] for available CPU codenames. To detect
>> +-- CPU codename execute `gcc -march=native -Q --help=target | grep march`.
>> +--
>> +-- 1. https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
>> +--
>> +-- Manual steps for reproducing are the following:
>> +--
>> +-- $ make CC=gcc TARGET_CFLAGS='skylake-avx512' -f Makefile.original
>> +-- $ 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')
>> +
>> +-- Format of the Mach-O is described in a document
> Typo: s/a document/the document/
Fixed.
>> +-- "OS X ABI Mach-O File Format Reference", published by Apple company.
>> +-- Copy of the (now removed) official documentation in [1].
> Let's replace "in [1]" with "can be found here [1].".
Fixed.
>> +-- Yet another source of thruth is a XNU headers, see the definition
> Typo: s/is a XNU/are XNU/
> Typo: s/the definition/definitions/
Fixed.
>> +-- of C-structures in: [2] (`nlist_64`), [3] (`fat_arch` and `fat_header`).
>> +
>> +-- 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
>> +--
>> +-- 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
>> +{
>> + mach_header; uint32_t reserved;
>> +} mach_header_64;
>> +
>> +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 {
>> + 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;
>> + uint32_t offset, align, reloff, nreloc, flags;
>> + 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;
>> +
>> +typedef struct {
>> + int32_t strx;
>> + uint8_t type, sect;
>> + int16_t desc;
>> + uint32_t value;
>> +} mach_nlist;
>> +
>> +typedef struct {
>> + uint32_t strx;
>> + uint8_t type, sect;
>> + uint16_t desc;
>> + uint64_t value;
>> +} mach_nlist_64;
>> +
>> +typedef struct
>> +{
>> + uint32_t magic, nfat_arch;
>> +} mach_fat_header;
>> +
>> +typedef struct
>> +{
>> + uint32_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;
>> +
>> +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)
>> + local mach_o_path = os.tmpname() .. '.o'
>> + local lua_path = os.getenv('LUA_PATH')
>> + local lua_bin = require('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 an Mach-O format and returns
> Typo: s/in an/in the/
Fixed.
>> +-- an fat magic number and nfat_arch.
> Typo: s/an fat/the FAT/
> Typo: s/nfat_arch/`nfat_arch`
Fixed.
>> +local function read_mach_o(buf, is64)
>> + local res = {
>> + header = {
>> + magic = 0,
>> + nfat_arch = 0,
>> + },
>> + fat_arch = {
>> + },
> I guess, that the formatting below is a bit better here:
> | fat_arch = {},
Fixed.
>> + }
>> +
>> + -- Mach-O FAT object.
>> + local mach_fat_obj_type = ffi.typeof(is64 and 'mach_fat_obj_64 *' or 'mach_fat_obj *')
> The line is longer than 80 symbols.
Fixed.
>> + local obj = ffi.cast(mach_fat_obj_type, buf)
>> +
>> + -- Mach-O FAT object header.
>> + local mach_fat_header_type = ffi.typeof('mach_fat_header *')
>> + local mach_fat_header = ffi.cast(mach_fat_header_type, obj.fat)
>> + local be32 = bit.bswap -- Mach-O FAT is BE, target arch is LE.
>> + res.header.magic = be32(mach_fat_header.magic)
>> + res.header.nfat_arch = be32(mach_fat_header.nfat_arch)
>> +
>> + -- Mach-O FAT object archs.
> Typo: s/archs/arches/
Fixed.
> Side note: I feel like the comments for the sections are not elaborate
> enough for unprepared reader. I think you should briefly desribe the
> basic structure of a FAT object (FAT header, then array of per-segment
> headers, then object files)
Added a short description and an ASCII scheme.
>> + 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 = {
>> + cputype = be32(fat_arch.cputype),
>> + cpusubtype = be32(fat_arch.cpusubtype),
>> + }
>> + table.insert(res.fat_arch, arch)
>> + end
>> +
>> + return res
>> +end
>> +
>> +-- Defined in <src/jit/bcsave.lua:bcsave_machobj>.
>> +local sum_cputype = {
>> + x86 = 7,
>> + x64 = 0x01000007,
>> + arm = 7 + 12,
>> + arm64 = 0x01000007 + 0x0100000c,
>> +}
>> +local sum_cpusubtype = {
>> + x86 = 3,
>> + x64 = 3,
>> + arm = 3 + 9,
>> + arm64 = 3 + 0,
>> +}
> It would be nice to have an explanation for these magic numbers.
Added:
@@ -183,7 +246,13 @@ local function read_mach_o(buf, is64)
return res
end
--- Defined in <src/jit/bcsave.lua:bcsave_machobj>.
+-- Universal Binary can contain executables for more than one
+-- CPU architecture. For simplicity the test compares
+-- sum of CPU types and CPU subtypes.
+-- Numbers below are defined in <src/jit/bcsave.lua:bcsave_machobj>.
+-- Original source is XNU source code, <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 = {
x86 = 7,
x64 = 0x01000007,
>> +
>> +-- The function builds Mach-O FAT object file and retrieves
>> +-- its header fields (magic and nfat_arch)
>> +-- and fields of the each arch (cputype, cpusubtype).
>> +--
>> +-- Mach-O FAT object header could be retrieved with `otool` on macOS:
> Typo: s/could be/can be/
Fixed.
>> +--
>> +-- $ otool -f empty.o
>> +-- Fat headers
>> +-- fat_magic 0xcafebabe
>> +-- nfat_arch 2
>> +-- <snipped>
>> +--
>> +-- CPU type and subtype could be retrieved with `lipo` on macOS:
> Typo: s/could be/can be/
Fixed.
>> +--
>> +-- $ 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(is64)
>> + local arch = is64 and 'arm64' or 'arm'
>> +
>> + -- FAT_MAGIC is an integer containing the value 0xCAFEBABE in
>> + -- big-endian byte order format. On a big-endian host CPU,
>> + -- this can be validated using the constant FAT_MAGIC;
>> + -- on a little-endian host CPU, it can be validated using
>> + -- the constant FAT_CIGAM.
>> + --
>> + -- FAT_NARCH is an integer specifying the number of fat_arch
>> + -- data structures that follow. This is the number of
>> + -- architectures contained in this binary.
>> + --
>> + -- See aforementioned "OS X ABI Mach-O File Format Reference".
>> + --
>> + local FAT_MAGIC = '0xffffffffcafebabe'
>> + local FAT_NARCH = 2
>> +
>> + local MODULE_NAME = 'lango_team'
>> +
>> + local mach_o_obj_path = create_obj_file(MODULE_NAME, arch)
>> + local mach_o_buf = require('utils').tools.read_file(mach_o_obj_path)
>> + assert(mach_o_buf == nil or #mach_o_buf ~= 0, 'cannot read an object file')
>> +
>> + local mach_o = read_mach_o(mach_o_buf, is64)
>> +
>> + -- Teardown.
>> + local retcode = os.remove(mach_o_obj_path)
>> + assert(retcode == true, 'remove an object file')
>> +
>> + local magic_str = string.format('0x%02x', mach_o.header.magic)
>> + test:is(magic_str, FAT_MAGIC, 'fat_magic is correct in Mach-O, ' .. arch)
>> + test:is(mach_o.header.nfat_arch, FAT_NARCH, 'nfat_arch is correct in Mach-O, ' .. arch)
>> +
>> + local total_cputype = 0
>> + local total_cpusubtype = 0
>> + for i = 1, mach_o.header.nfat_arch do
>> + 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)
>> +end
>> +
>> +-- ARM
>> +build_and_check_mach_o(false)
>> +
>> +test:done(true)
> Please mention that alongside with test and fix for the issue
> you've added this tool. IMO, it would be even better to do that
> in a separate commit, to avoid confusion because of the updates in
> test files unrelated to the patch.
Helper is moved to a separate patch.
>> diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua
>> index f35c6922..26b8c08d 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 in 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
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.
2024-03-18 13:44 ` Maxim Kokryashkin via Tarantool-patches
@ 2024-03-19 8:22 ` Sergey Bronnikov via Tarantool-patches
2024-03-19 16:15 ` Maxim Kokryashkin via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-03-19 8:22 UTC (permalink / raw)
To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
Max,
thanks for review. See my comments below.
Changes force-pushed.
On 3/18/24 16:44, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> On Thu, Mar 14, 2024 at 02:39:51PM +0300, Sergey Bronnikov wrote:
>> From: sergeyb@tarantool.org
>>
>> Thanks to Carlo Cabrera.
>>
>> (cherry picked from commit b98b37231bd2dcb79e10b0f974cefd91eb0d7b3a)
>>
>> Mach-O FAR object files generated by LuaJIT for arm64 had an incorrect
> Typo: s/FAR/FAT/
> Typo: s/arm64/ARM64/
Fixed.
>> format because FFI structure used for generation was wrong:
> Typo: s/FFI/The FFI/
Fixed.
>> `mach_fat_obj` instead of `mach_fat_obj_64`.
>>
>> Sergey Bronnikov:
>> * added the description and a test for the problem
> Typo: s/a test/the test/
Fixed.
>> Part of tarantool/tarantool#9595
>> ---
>> src/jit/bcsave.lua | 14 +++++++++++++-
>> ...-fix_generation_of_mach-o_object_files.test.lua | 4 +++-
>> 2 files changed, 16 insertions(+), 2 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-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 0519e134..0a11f163 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
>> @@ -7,7 +7,7 @@ local test = tap.test('lj-865-fix_generation_of_mach-o_object_files'):skipcond({
>> ['Test uses exotic type of loaders (see #9671)'] = _TARANTOOL,
>> })
>>
>> -test:plan(4)
>> +test:plan(8)
>>
>> -- Test creates an object file in Mach-O format with LuaJIT bytecode
>> -- and checks validness of the object file fields.
>> @@ -267,5 +267,7 @@ end
>>
>> -- ARM
>> build_and_check_mach_o(false)
>> +-- ARM64
>> +build_and_check_mach_o(true)
> These `true/false` should be explained as platform toggle.
> An even better solution would be to pass the platform name
> explicitly.
Why comment above is not sufficient?
>> test:done(true)
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.
2024-03-19 8:22 ` Sergey Bronnikov via Tarantool-patches
@ 2024-03-19 16:15 ` Maxim Kokryashkin via Tarantool-patches
2024-03-26 14:01 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-03-19 16:15 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: Sergey Bronnikov, tarantool-patches
Hi, Sergey!
Thanks for the fixes!
See my answer for the last unresolved comment below.
On Tue, Mar 19, 2024 at 11:22:20AM +0300, Sergey Bronnikov wrote:
> Max,
>
> thanks for review. See my comments below.
>
> Changes force-pushed.
>
> On 3/18/24 16:44, Maxim Kokryashkin wrote:
<snipped>
> > > 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 0519e134..0a11f163 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
> > > @@ -7,7 +7,7 @@ local test = tap.test('lj-865-fix_generation_of_mach-o_object_files'):skipcond({
> > > ['Test uses exotic type of loaders (see #9671)'] = _TARANTOOL,
> > > })
> > >
> > > -test:plan(4)
> > > +test:plan(8)
> > >
> > > -- Test creates an object file in Mach-O format with LuaJIT bytecode
> > > -- and checks validness of the object file fields.
Typo: s/validness/the validity/
> > > @@ -267,5 +267,7 @@ end
> > >
> > > -- ARM
> > > build_and_check_mach_o(false)
> > > +-- ARM64
> > > +build_and_check_mach_o(true)
> > These `true/false` should be explained as platform toggle.
> > An even better solution would be to pass the platform name
> > explicitly.
> Why comment above is not sufficient?
Comment is sufficient, sure, but why should you opt for a comment, when
you can write self-explanatory code instead?
> > > test:done(true)
> > > --
> > > 2.34.1
> > >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.
2024-03-19 8:19 ` Sergey Bronnikov via Tarantool-patches
@ 2024-03-19 16:28 ` Maxim Kokryashkin via Tarantool-patches
2024-03-26 13:53 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-03-19 16:28 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: Sergey Bronnikov, tarantool-patches
Hi, Sergey!
Thanks for the fixes!
Please consider my answers below.
On Tue, Mar 19, 2024 at 11:19:19AM +0300, Sergey Bronnikov wrote:
> Max,
>
> thanks for review! see comments below
>
>
> On 3/18/24 15:53, Maxim Kokryashkin wrote:
> > Hi, Sergey, thanks for the patch!
> > Please consider my comments below.
> > On Thu, Mar 14, 2024 at 02:39:49PM +0300, Sergey Bronnikov wrote:
<snipped>
> > > .github/workflows/exotic-builds-testing.yml | 5 +-
> > > src/jit/bcsave.lua | 6 +-
> > > .../lj-366-strtab-correct-size.test.lua | 10 +-
> > > ...generation_of_mach-o_object_files.test.lua | 271 ++++++++++++++++++
> > > test/tarantool-tests/utils/tools.lua | 8 +
> > > 5 files changed, 287 insertions(+), 13 deletions(-)
> > > create mode 100644 test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
> > >
> > > diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
> > > index a9ba5fd5..df4bc2e9 100644
> > > --- a/.github/workflows/exotic-builds-testing.yml
> > > +++ b/.github/workflows/exotic-builds-testing.yml
> > > @@ -32,6 +32,7 @@ jobs:
> > > fail-fast: false
> > > matrix:
> > > BUILDTYPE: [Debug, Release]
> > > + OS: [Linux, macOS]
> > > ARCH: [ARM64, x86_64]
> > > GC64: [ON, OFF]
> > > FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
> > > @@ -50,13 +51,15 @@ jobs:
> > > FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
> > https://github.com/tarantool/luajit/actions/runs/8279362128
> > > - FLAVOR: nounwind
> > > FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
> > > + - FLAVOR: avx512
> > > + CMAKEFLAGS: -DCMAKE_C_FLAGS=skylake-avx512 -DCMAKE_C_COMPILER=gcc
> > > exclude:
> > > - ARCH: ARM64
> > > GC64: OFF
> > > # DUALNUM is default for ARM64, no need for additional testing.
> > > - FLAVOR: dualnum
> > > ARCH: ARM64
> > > - runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
> > > + runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}', '${ matrix.OS }']
> > The matrix.OS variable should be wrapped with double curly braces,
> > instead of singular ones. So, it should be like this:
> > | '${{ matrix.OS }}'
>
> Fixed, thanks!
>
>
> > Currently, exotic build testing fails to start because of this mistake.
> > https://github.com/tarantool/luajit/actions/runs/8279362128
That's unfortunate, but the workflow run still fails to start. It seems
like double quotation marks are required around the computed properties.
At least, they are mentioned specifically in the documentation:
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idruns-on
So both variables should actually be:
| "${{ matrix.OS }}", "${{ matrix.ARCH }}"
> > > name: >
> > > LuaJIT ${{ matrix.FLAVOR }}
> > > (Linux/${{ matrix.ARCH }})
> > > 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/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> > > index 8a97a441..0bb92da6 100644
> > > --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> > > +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> > Let's move this update to a separate patch alongside with the added
> > utility function. Currently, this change in unrelated test file is
> > a bit confusing.
>
>
> Moved to a separate patch:
>
>
> commit 138bcda850ed4aad4bbbae4d30315d9a0934cb26
> Author: Sergey Bronnikov <sergeyb@tarantool.org>
> Date: Tue Mar 19 09:56:51 2024 +0300
>
> test: introduce a helper read_file
>
> The test `lj-366-strtab-correct-size.test.lua` has a test helper
> `read_file` that read a file's content and returns it.
Typo: s/read/reads/
> This helper will be useful for a test upcoming in the next commit,
> so it is moved to a test tools.
Typo: s/to a/to/
>
> 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..0bb92da6 100644
> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> @@ -138,14 +138,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 +164,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..26b8c08d 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 in 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
>
>
> > > @@ -138,14 +138,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 +164,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)
The newly added patch LGTM.
> > > 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
> > > new file mode 100644
> > > index 00000000..0519e134
> > > --- /dev/null
> > > +++ b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
> > > @@ -0,0 +1,271 @@
<snipped>
> > Side note: I feel like the comments for the sections are not elaborate
> > enough for unprepared reader. I think you should briefly desribe the
> > basic structure of a FAT object (FAT header, then array of per-segment
> > headers, then object files)
> Added a short description and an ASCII scheme.
The scheme is very nice and informative, thanks!
<snipped>
Regards,
Maxim Kokryashkin
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.
2024-03-19 16:28 ` Maxim Kokryashkin via Tarantool-patches
@ 2024-03-26 13:53 ` Sergey Bronnikov via Tarantool-patches
2024-03-26 15:44 ` Maxim Kokryashkin via Tarantool-patches
2024-04-08 15:01 ` Sergey Kaplun via Tarantool-patches
0 siblings, 2 replies; 24+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-03-26 13:53 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: Sergey Bronnikov, tarantool-patches
Max,
changes force-pushed.
On 3/19/24 19:28, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the fixes!
> Please consider my answers below.
>
> On Tue, Mar 19, 2024 at 11:19:19AM +0300, Sergey Bronnikov wrote:
>> Max,
>>
>> thanks for review! see comments below
>>
>>
>> On 3/18/24 15:53, Maxim Kokryashkin wrote:
>>> Hi, Sergey, thanks for the patch!
>>> Please consider my comments below.
>>> On Thu, Mar 14, 2024 at 02:39:49PM +0300, Sergey Bronnikov wrote:
> <snipped>
>>>> .github/workflows/exotic-builds-testing.yml | 5 +-
>>>> src/jit/bcsave.lua | 6 +-
>>>> .../lj-366-strtab-correct-size.test.lua | 10 +-
>>>> ...generation_of_mach-o_object_files.test.lua | 271 ++++++++++++++++++
>>>> test/tarantool-tests/utils/tools.lua | 8 +
>>>> 5 files changed, 287 insertions(+), 13 deletions(-)
>>>> create mode 100644 test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
>>>>
>>>> diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
>>>> index a9ba5fd5..df4bc2e9 100644
>>>> --- a/.github/workflows/exotic-builds-testing.yml
>>>> +++ b/.github/workflows/exotic-builds-testing.yml
>>>> @@ -32,6 +32,7 @@ jobs:
>>>> fail-fast: false
>>>> matrix:
>>>> BUILDTYPE: [Debug, Release]
>>>> + OS: [Linux, macOS]
>>>> ARCH: [ARM64, x86_64]
>>>> GC64: [ON, OFF]
>>>> FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
>>>> @@ -50,13 +51,15 @@ jobs:
>>>> FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
>>> https://github.com/tarantool/luajit/actions/runs/8279362128
>>>> - FLAVOR: nounwind
>>>> FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
>>>> + - FLAVOR: avx512
>>>> + CMAKEFLAGS: -DCMAKE_C_FLAGS=skylake-avx512 -DCMAKE_C_COMPILER=gcc
>>>> exclude:
>>>> - ARCH: ARM64
>>>> GC64: OFF
>>>> # DUALNUM is default for ARM64, no need for additional testing.
>>>> - FLAVOR: dualnum
>>>> ARCH: ARM64
>>>> - runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
>>>> + runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}', '${ matrix.OS }']
>>> The matrix.OS variable should be wrapped with double curly braces,
>>> instead of singular ones. So, it should be like this:
>>> | '${{ matrix.OS }}'
>> Fixed, thanks!
>>
>>
>>> Currently, exotic build testing fails to start because of this mistake.
>>> https://github.com/tarantool/luajit/actions/runs/8279362128
> That's unfortunate, but the workflow run still fails to start. It seems
> like double quotation marks are required around the computed properties.
> At least, they are mentioned specifically in the documentation:
> https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idruns-on
>
> So both variables should actually be:
> | "${{ matrix.OS }}", "${{ matrix.ARCH }}"
Fixed. Patch with fix is below:
--- a/.github/workflows/exotic-builds-testing.yml
+++ b/.github/workflows/exotic-builds-testing.yml
@@ -35,7 +35,7 @@ jobs:
OS: [Linux, macOS]
ARCH: [ARM64, x86_64]
GC64: [ON, OFF]
- FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
+ FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind, avx512]
include:
- BUILDTYPE: Debug
CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON
-DLUA_USE_APICHECK=ON
>>>> name: >
>>>> LuaJIT ${{ matrix.FLAVOR }}
>>>> (Linux/${{ matrix.ARCH }})
>>>> 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/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>>>> index 8a97a441..0bb92da6 100644
>>>> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>>>> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>>> Let's move this update to a separate patch alongside with the added
>>> utility function. Currently, this change in unrelated test file is
>>> a bit confusing.
>>
>> Moved to a separate patch:
>>
>>
>> commit 138bcda850ed4aad4bbbae4d30315d9a0934cb26
>> Author: Sergey Bronnikov <sergeyb@tarantool.org>
>> Date: Tue Mar 19 09:56:51 2024 +0300
>>
>> test: introduce a helper read_file
>>
>> The test `lj-366-strtab-correct-size.test.lua` has a test helper
>> `read_file` that read a file's content and returns it.
> Typo: s/read/reads/
Fixed.
>> This helper will be useful for a test upcoming in the next commit,
>> so it is moved to a test tools.
> Typo: s/to a/to/
Fixed.
>> 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..0bb92da6 100644
>> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> @@ -138,14 +138,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 +164,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..26b8c08d 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 in 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
>>
>>
>>>> @@ -138,14 +138,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 +164,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)
> The newly added patch LGTM.
>
>>>> 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
>>>> new file mode 100644
>>>> index 00000000..0519e134
>>>> --- /dev/null
>>>> +++ b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
>>>> @@ -0,0 +1,271 @@
> <snipped>
>>> Side note: I feel like the comments for the sections are not elaborate
>>> enough for unprepared reader. I think you should briefly desribe the
>>> basic structure of a FAT object (FAT header, then array of per-segment
>>> headers, then object files)
>> Added a short description and an ASCII scheme.
> The scheme is very nice and informative, thanks!
>
> <snipped>
>
> Regards,
> Maxim Kokryashkin
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.
2024-03-19 16:15 ` Maxim Kokryashkin via Tarantool-patches
@ 2024-03-26 14:01 ` Sergey Bronnikov via Tarantool-patches
2024-03-26 15:45 ` Maxim Kokryashkin via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-03-26 14:01 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: Sergey Bronnikov, tarantool-patches
Max,
changes force-pushed.
On 3/19/24 19:15, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the fixes!
> See my answer for the last unresolved comment below.
> On Tue, Mar 19, 2024 at 11:22:20AM +0300, Sergey Bronnikov wrote:
>> Max,
>>
>> thanks for review. See my comments below.
>>
>> Changes force-pushed.
>>
>> On 3/18/24 16:44, Maxim Kokryashkin wrote:
> <snipped>
>>>> 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 0519e134..0a11f163 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
>>>> @@ -7,7 +7,7 @@ local test = tap.test('lj-865-fix_generation_of_mach-o_object_files'):skipcond({
>>>> ['Test uses exotic type of loaders (see #9671)'] = _TARANTOOL,
>>>> })
>>>>
>>>> -test:plan(4)
>>>> +test:plan(8)
>>>>
>>>> -- Test creates an object file in Mach-O format with LuaJIT bytecode
>>>> -- and checks validness of the object file fields.
> Typo: s/validness/the validity/
Fixed.
>>>> @@ -267,5 +267,7 @@ end
>>>>
>>>> -- ARM
>>>> build_and_check_mach_o(false)
>>>> +-- ARM64
>>>> +build_and_check_mach_o(true)
>>> These `true/false` should be explained as platform toggle.
>>> An even better solution would be to pass the platform name
>>> explicitly.
>> Why comment above is not sufficient?
> Comment is sufficient, sure, but why should you opt for a comment, when
> you can write self-explanatory code instead?
Fixed.
'arm' or 'arm64' are passed to a test func.
I believe this is clear enough for reader.
>>>> test:done(true)
>>>> --
>>>> 2.34.1
>>>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.
2024-03-26 13:53 ` Sergey Bronnikov via Tarantool-patches
@ 2024-03-26 15:44 ` Maxim Kokryashkin via Tarantool-patches
2024-04-08 15:01 ` Sergey Kaplun via Tarantool-patches
1 sibling, 0 replies; 24+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-03-26 15:44 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: Sergey Bronnikov, tarantool-patches
Hi, Sergey!
Thanks for the patch!
LGTM
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.
2024-03-26 14:01 ` Sergey Bronnikov via Tarantool-patches
@ 2024-03-26 15:45 ` Maxim Kokryashkin via Tarantool-patches
0 siblings, 0 replies; 24+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-03-26 15:45 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: Sergey Bronnikov, tarantool-patches
Hi, Sergey!
Thanks for the patch!
LGTM
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes
2024-03-14 11:39 [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes Sergey Bronnikov via Tarantool-patches
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files Sergey Bronnikov via Tarantool-patches
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file Sergey Bronnikov via Tarantool-patches
@ 2024-04-08 7:47 ` Sergey Kaplun via Tarantool-patches
2024-04-08 13:06 ` Sergey Kaplun via Tarantool-patches
2024-04-11 8:08 ` Sergey Bronnikov via Tarantool-patches
2 siblings, 2 replies; 24+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-04-08 7:47 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the patch set!
I'll proceed with the review according version on the branch:
===================================================================
> Subject: [PATCH 1/3] test: introduce a helper read_file
This patch LGTM, after fixing several nits below.
>
> 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.
So, missed the ticket mentioning:
| Needed for tarantool/tarantool#9595
> ---
> .../lj-366-strtab-correct-size.test.lua | 10 +---------
> test/tarantool-tests/utils/tools.lua | 8 ++++++++
> 2 files changed, 9 insertions(+), 9 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..0bb92da6 100644
> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> @@ -138,14 +138,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 +164,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)
Minor: I suggest avoiding the change at this line.
According to codestyle in other tests, we require helpers separately,
even if they are used only once.
> 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..26b8c08d 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 in a specified path and returns its content.
Typo: s/in/at/
> +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.44.0
===================================================================
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes
2024-04-08 7:47 ` [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes Sergey Kaplun via Tarantool-patches
@ 2024-04-08 13:06 ` Sergey Kaplun via Tarantool-patches
2024-04-11 8:08 ` Sergey Bronnikov via Tarantool-patches
1 sibling, 0 replies; 24+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-04-08 13:06 UTC (permalink / raw)
To: Sergey Bronnikov, tarantool-patches
On 08.04.24, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch set!
>
> I'll proceed with the review according version on the branch:
>
> ===================================================================
> > Subject: [PATCH 1/3] test: introduce a helper read_file
>
> This patch LGTM, after fixing several nits below.
>
> >
> > 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.
>
> So, missed the ticket mentioning:
> | Needed for tarantool/tarantool#9595
>
> > ---
> > .../lj-366-strtab-correct-size.test.lua | 10 +---------
> > test/tarantool-tests/utils/tools.lua | 8 ++++++++
> > 2 files changed, 9 insertions(+), 9 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..0bb92da6 100644
> > --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> > +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> > @@ -138,14 +138,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 +164,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)
>
> Minor: I suggest avoiding the change at this line.
> According to codestyle in other tests, we require helpers separately,
> even if they are used only once.
>
> > 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..26b8c08d 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 in a specified path and returns its content.
>
> Typo: s/in/at/
Also, comment 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.44.0
> ===================================================================
>
> --
> Best regards,
> Sergey Kaplun
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.
2024-03-26 13:53 ` Sergey Bronnikov via Tarantool-patches
2024-03-26 15:44 ` Maxim Kokryashkin via Tarantool-patches
@ 2024-04-08 15:01 ` Sergey Kaplun via Tarantool-patches
2024-04-09 11:07 ` Sergey Bronnikov via Tarantool-patches
1 sibling, 1 reply; 24+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-04-08 15:01 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: Sergey Bronnikov, tarantool-patches
Hi, Sergey!
Thanks for the patch!
I'll proceed with the version from the branch.
Please consider my comments below.
Please rebase your branch to the tarantool/master and solve the
conflicts.
On 26.03.24, Sergey Bronnikov wrote:
> Subject: [PATCH 2/3] OSX/iOS/ARM64: Fix generation of Mach-O object files.
>
> Thanks to Carlo Cabrera.
>
> (cherry picked from commit 3065c910ad6027031aabe2dfd3c26a3d0f014b4f)
>
<snipped>
>
> Part of tarantool/tarantool#9595
> ---
> .github/workflows/exotic-builds-testing.yml | 6 +-
> src/jit/bcsave.lua | 6 +-
> test/LuaJIT-tests/CMakeLists.txt | 9 +
> test/LuaJIT-tests/lib/ffi/index | 2 +-
> ...generation_of_mach-o_object_files.test.lua | 343 ++++++++++++++++++
> 5 files changed, 361 insertions(+), 5 deletions(-)
> create mode 100644 test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
>
> diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
> index 859603bd..9cace0bc 100644
> --- a/.github/workflows/exotic-builds-testing.yml
> +++ b/.github/workflows/exotic-builds-testing.yml
> @@ -34,7 +34,7 @@ jobs:
> BUILDTYPE: [Debug, Release]
> ARCH: [ARM64, x86_64]
> GC64: [ON, OFF]
> - FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
> + FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind, avx512]
Please sort entries alphabetically.
> include:
> - BUILDTYPE: Debug
> CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> @@ -50,12 +50,16 @@ jobs:
> FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
> - FLAVOR: nounwind
> FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
> + - FLAVOR: avx512
> + CMAKEFLAGS: -DCMAKE_C_FLAGS=-march=skylake-avx512 -DCMAKE_C_COMPILER=gcc
Is there any guarantee that the given runner has avx512 support?
If not, our tests will fail due to illegal instruction error.
> exclude:
> - ARCH: ARM64
> GC64: OFF
> # DUALNUM is default for ARM64, no need for additional testing.
> - FLAVOR: dualnum
> ARCH: ARM64
> + - FLAVOR: avx512
> + ARCH: ARM64
Please sort entries alphabetically.
> runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
> name: >
> LuaJIT ${{ matrix.FLAVOR }}
> 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 a0fb5440..29146113 100644
> --- a/test/LuaJIT-tests/CMakeLists.txt
> +++ b/test/LuaJIT-tests/CMakeLists.txt
> @@ -64,6 +64,15 @@ if(LUAJIT_NO_UNWIND)
> set(LUAJIT_TEST_TAGS_EXTRA +internal_unwinder)
> endif()
>
> +if(CMAKE_C_FLAGS MATCHES "-march=skylake-avx512")
> + # Test <bit64.lua> verifies bitwise operations on numbers.
Maybe we should add FIXME: tag here. Fill free to ignore.
> + # There is a known issue - bitop doesn't working in LuaJIT
Typo: s/working/work/
> + # built with enabled AVX512 instruction set,
Typo: s/enabled/the enabled/
> + # see https://github.com/tarantool/tarantool/issues/6787.
> + # Hence, skip this when "skylake-avx512" is passed.
> + set(LUAJIT_TEST_TAGS_EXTRA +avx512)
> +endif()
> +
> add_custom_command(TARGET LuaJIT-tests
> COMMENT "Running LuaJIT-tests"
> COMMAND
> diff --git a/test/LuaJIT-tests/lib/ffi/index b/test/LuaJIT-tests/lib/ffi/index
> index e3a34e30..e7c168c2 100644
> --- a/test/LuaJIT-tests/lib/ffi/index
> +++ b/test/LuaJIT-tests/lib/ffi/index
> @@ -1,4 +1,4 @@
> -bit64.lua +luajit>=2.1
> +bit64.lua +luajit>=2.1 -avx512
Do we need to skip all tests instead of only failed one?
> cdata_var.lua
> copy_fill.lua
> err.lua
> diff --git a/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
Please, use "-" instead of "_" as a separator, according to other test
names.
Also, the test name is too long. I suggest something like the
following:
| lj-865-cross-generation-mach-o-file.test.lua
Don't forget to update the testname below as well.
> new file mode 100644
> index 00000000..40bd1c74
> --- /dev/null
> +++ b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
> @@ -0,0 +1,343 @@
> +local tap = require('tap')
> +local test = tap.test('lj-865-fix_generation_of_mach-o_object_files')
> +
> +test:plan(4)
> +
> +-- The test creates an object file in Mach-O format with LuaJIT
> +-- bytecode and checks the validness of the object file fields.
Typo? s/validness/validity/
IINM, validness is the obsolete form.
> +--
> +-- 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 CPU codename.
Typo: s/CPU codename/a CPU codename/
> +-- Please consult for the available model architecture on
> +-- GCC Online Documentation [1] for available CPU codenames.
> +-- and Wikipedia for CPUs with AVX-512 support [2].
I suggest the following rewording:
| Please consult the GCC Online Documentation [1] for available CPU
| codenames. Also, see the Wikipedia for CPUs with AVX-512 support [2].
Or the follwing:
| Please take a look at the GCC Online Documentation [1] for available
| CPU codenames. Also, see the Wikipedia for CPUs with AVX-512 support [2].
Feel free to reword it like you want.
> +-- To detect CPU codename execute:
Typo: s/CPU codename/the CPU 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:
I suppose you mean "from the original issue". Or we can use
| -DCMAKE_C_FLAGS="-march=skylake-avx512"
> +--
> +-- $ make CC=gcc TARGET_CFLAGS='skylake-avx512' -f Makefile.original
> +-- $ 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 bytetcode.
Comment width is more than 66 symbols.
> +-- 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.
> +--
> +-- Format of the Mach-O is described in the document
Typo: s/Format/The format/
> +-- "OS X ABI Mach-O File Format Reference", published by Apple company.
Typo? s/Reference",/Reference,"/
> +-- Copy of the (now removed) official documentation can be found
Typo: s/Copy/The copy/
> +-- here [1]. Yet another source of thruth are XNU headers,
Typo: s/thruth/truth/
> +-- see definition of C-structures in: [2] (`nlist_64`),
Typo: s/definition/the definition/
> +-- [3] (`fat_arch` and `fat_header`).
> +--
> +-- 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 [6].
Typo: s/in/in the/g
> +-- Below is schematic structure of Universal Binary that
Typo: s/schematic/the schematic/
Typo: s/Binary that/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
> +-- ---------------------------------------
Side note: This diagramm is amazing thanks!
> +--
> +-- < x86 executable >
> +--
> +--
Nit: excess empty line.
> +-- 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
> +{
> + mach_header; uint32_t reserved;
> +} mach_header_64;
> +
> +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 {
> + 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;
> + uint32_t offset, align, reloff, nreloc, flags;
> + 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;
> +
> +typedef struct {
> + int32_t strx;
> + uint8_t type, sect;
> + int16_t desc;
> + uint32_t value;
> +} mach_nlist;
> +
> +typedef struct {
> + uint32_t strx;
Should we use here `int32_t` as it is after the changes?
> + uint8_t type, sect;
> + uint16_t desc;
> + uint64_t value;
> +} mach_nlist_64;
> +
> +typedef struct
> +{
> + uint32_t magic, nfat_arch;
Should we use here `int32_t` as it is after the changes?
> +} mach_fat_header;
> +
> +typedef struct
> +{
> + uint32_t cputype, cpusubtype, offset, size, align;
Should we use here `int32_t` as it is after the changes?
> +} 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;
> +
> +typedef struct {
> + mach_fat_header fat;
> + mach_fat_arch fat_arch[2];
> + struct {
> + mach_header_64 hdr;
> + mach_segment_command_64 seg;
> + mach_section_64 sec;
> + mach_symtab_command sym;
> + } arch[2];
> + mach_nlist_64 sym_entry;
> + uint8_t space[4096];
> +} mach_fat_obj_64;
Should this part be defined in the next patch, since there is no such
cdef at the moment?
OTOH, it makes test more arch-agnostic. So, the adding of the other case
is more simple.
> +]]
> +
> +local function create_obj_file(name, arch)
> + local mach_o_path = os.tmpname() .. '.o'
> + local lua_path = os.getenv('LUA_PATH')
> + local lua_bin = require('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, hw_arch)
> + local res = {
> + header = {
> + magic = 0,
> + nfat_arch = 0,
> + },
> + fat_arch = {},
> + }
> +
> + local is64 = hw_arch == 'arm64'
> +
> + -- Mach-O FAT object.
> + local mach_fat_obj_type = ffi.typeof(is64 and
> + 'mach_fat_obj_64 *' or 'mach_fat_obj *')
> + local obj = ffi.cast(mach_fat_obj_type, buf)
> +
> + -- Mach-O FAT object header.
> + local mach_fat_header_type = ffi.typeof('mach_fat_header *')
> + local mach_fat_header = ffi.cast(mach_fat_header_type, obj.fat)
Looks like this cast is excess:
| src/luajit -e '
| local ffi = require"ffi"
| ffi.cdef[[
| struct test {int a; int b;};
| struct test_outer { struct test t; int c;};
| ]]
| local s = ffi.new("struct test_outer", {{0}, 0})
| print(s.t.a)
| '
| 0
> + local be32 = bit.bswap -- Mach-O FAT is BE, target arch is LE.
Please, use the separate line for the comment.
> + res.header.magic = be32(mach_fat_header.magic)
> + res.header.nfat_arch = be32(mach_fat_header.nfat_arch)
> +
> + -- Mach-O FAT object arches.
> + local mach_fat_arch_type = ffi.typeof('mach_fat_arch *')
> + for i = 0, res.header.nfat_arch - 1 do
> + local fat_arch = ffi.cast(mach_fat_arch_type, obj.fat_arch[i])
Looks like this cast is excess.
> + 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
Typo: s/For simplicity/For simplicity,/
Nit: The comment line is "underfilled".
> +-- sum of CPU types and CPU subtypes.
Typo: s/sum/the sum/
> +-- Numbers below are defined in <src/jit/bcsave.lua:bcsave_machobj>.
Typo: s/Numbers/The numbers/
> +-- Original source is XNU source code, <osfmk/mach/machine.h> [1].
Typo: s/Original/The original/
Typo: s/XNU/the XNU/
> +--
> +-- 1. https://opensource.apple.com/source/xnu/xnu-4570.41.2/osfmk/mach/machine.h.auto.html
> +local sum_cputype = {
> + x86 = 7,
> + x64 = 0x01000007,
> + arm = 7 + 12,
> + arm64 = 0x01000007 + 0x0100000c,
> +}
> +local sum_cpusubtype = {
> + x86 = 3,
> + x64 = 3,
> + arm = 3 + 9,
> + arm64 = 3 + 0,
> +}
I suggest the following for the clarification:
| local cputype = {
| x86 = 7,
| x64 = 0x01000007,
| arm = 7,
| arm64 = 0x0100000c,
| }
| local cpusubtype = {
| x86 = 3,
| x64 = 3,
| arm = 9,
| arm64 = 0,
| }
And declare next:
| local sum_cputype = {
| arm = cputype.arm + cputype.x86,
| arm64 = cputype.arm64 + cputype.x64,
| }
| local sum_cpusubtype = {
| arm = cpusubtype.arm + cpusubtype.x86,
| arm64 = cpusubtype.arm64 + cpusubtype.x64,
| }
Also, it is preferable to use upper case since values are constants.
> +
> +-- The function builds Mach-O FAT object file and retrieves
> +-- its header fields (magic and nfat_arch)
> +-- and fields of the each arch (cputype, cpusubtype).
Typo: s/the each/each/
> +--
> +-- Mach-O FAT object header can be retrieved with `otool` on macOS:
Typo: s/Mach-O FAT object/A Mach-O FAT object/
Typo: s/header/headers/
> +--
> +-- $ otool -f empty.o
> +-- Fat headers
> +-- fat_magic 0xcafebabe
> +-- nfat_arch 2
> +-- <snipped>
Minor: it is better to separate quoting in the comment like the
following to simplify reading:
| $ otool -f empty.o
| Fat headers
| fat_magic 0xcafebabe
| nfat_arch 2
| ...
Feel free to ignore.
> +--
> +-- 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(hw_arch)
Maybe it is better to use a subtest here:
The number of tests is fixed for this function, and it becomes more
clear when we add the additional arch for testing.
> + assert(hw_arch == 'arm' or hw_arch == 'arm64')
> +
> + -- FAT_MAGIC is an integer containing the value 0xCAFEBABE in
> + -- big-endian byte order format. On a big-endian host CPU,
> + -- this can be validated using the constant FAT_MAGIC;
> + -- on a little-endian host CPU, it can be validated using
> + -- the constant FAT_CIGAM.
> + --
> + -- FAT_NARCH is an integer specifying the number of fat_arch
> + -- data structures that follow. This is the number of
> + -- architectures contained in this binary.
> + --
> + -- See aforementioned "OS X ABI Mach-O File Format Reference".
Typo: s/aforementioned/the aforementioned/
> + --
Excess empty line.
> + local FAT_MAGIC = '0xffffffffcafebabe'
> + local FAT_NARCH = 2
> +
> + local MODULE_NAME = 'lango_team'
> +
> + local mach_o_obj_path = create_obj_file(MODULE_NAME, hw_arch)
> + local mach_o_buf = require('utils').tools.read_file(mach_o_obj_path)
Please move `require` of the necessary tool to the start of the file to
be consistent with the code style in tests.
> + assert(mach_o_buf == nil or #mach_o_buf ~= 0, 'cannot read an object file')
Should it be:
| mach_o_buf ~= nil and #mach_o_buf ~= 0
> +
> + local mach_o = read_mach_o(mach_o_buf, hw_arch)
> +
> + -- Teardown.
> + local retcode = os.remove(mach_o_obj_path)
> + assert(retcode == true, 'remove an object file')
Minor: I suggest refactoring this as the following (`retcode` isn't used
anywhere else):
| assert(os.remove(mach_o_obj_path), 'remove an object file')
> +
> + local magic_str = string.format('0x%02x', mach_o.header.magic)
Why do we need 02 in the format string?
Also, you may use '%#x' for the same result.
> + test:is(magic_str, FAT_MAGIC,
> + 'fat_magic is correct in Mach-O, ' .. hw_arch)
> + test:is(mach_o.header.nfat_arch, FAT_NARCH,
> + 'nfat_arch is correct in Mach-O, ' .. hw_arch)
> +
> + local total_cputype = 0
> + local total_cpusubtype = 0
> + for i = 1, mach_o.header.nfat_arch do
Minor: I suggest using `FAT_NARCH` here since we've checked their
equality above.
> + 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[hw_arch],
> + 'cputype is correct in Mach-O, ' .. hw_arch)
> + test:is(total_cpusubtype, sum_cpusubtype[hw_arch],
> + 'cpusubtype is correct in Mach-O, ' .. hw_arch)
> +end
> +
> +build_and_check_mach_o('arm')
> +
> +test:done(true)
> --
> 2.44.0
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file Sergey Bronnikov via Tarantool-patches
2024-03-18 13:44 ` Maxim Kokryashkin via Tarantool-patches
@ 2024-04-08 15:16 ` Sergey Kaplun via Tarantool-patches
2024-04-11 7:56 ` Sergey Bronnikov via Tarantool-patches
1 sibling, 1 reply; 24+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-04-08 15:16 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Hi, Sergey!
The version on branch LGTM, except a single nit regarding the commit
message below.
On 14.03.24, Sergey Bronnikov wrote:
> From: sergeyb@tarantool.org
>
> Thanks to Carlo Cabrera.
>
> (cherry picked from commit b98b37231bd2dcb79e10b0f974cefd91eb0d7b3a)
>
> Mach-O FAR object files generated by LuaJIT for arm64 had an incorrect
Typo: s/FAR/FAT/
> format because FFI structure used for generation was wrong:
> `mach_fat_obj` instead of `mach_fat_obj_64`.
I suggest reformulating this as the following:
| 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 a test for the problem
>
> Part of tarantool/tarantool#9595
> ---
> src/jit/bcsave.lua | 14 +++++++++++++-
> ...-fix_generation_of_mach-o_object_files.test.lua | 4 +++-
> 2 files changed, 16 insertions(+), 2 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
<snipped>
> 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 0519e134..0a11f163 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
<snipped>
> --
> 2.34.1
>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.
2024-04-08 15:01 ` Sergey Kaplun via Tarantool-patches
@ 2024-04-09 11:07 ` Sergey Bronnikov via Tarantool-patches
2024-04-09 12:47 ` Sergey Kaplun via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-04-09 11:07 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: Sergey Bronnikov, tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 28713 bytes --]
Hi, Sergey
thanks for review! Please see my comment below.
On 08.04.2024 18:01, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> I'll proceed with the version from the branch.
>
> Please consider my comments below.
>
> Please rebase your branch to the tarantool/master and solve the
> conflicts.
>
> On 26.03.24, Sergey Bronnikov wrote:
>> Subject: [PATCH 2/3] OSX/iOS/ARM64: Fix generation of Mach-O object files.
>>
>> Thanks to Carlo Cabrera.
>>
>> (cherry picked from commit 3065c910ad6027031aabe2dfd3c26a3d0f014b4f)
>>
> <snipped>
>
>> Part of tarantool/tarantool#9595
>> ---
>> .github/workflows/exotic-builds-testing.yml | 6 +-
>> src/jit/bcsave.lua | 6 +-
>> test/LuaJIT-tests/CMakeLists.txt | 9 +
>> test/LuaJIT-tests/lib/ffi/index | 2 +-
>> ...generation_of_mach-o_object_files.test.lua | 343 ++++++++++++++++++
>> 5 files changed, 361 insertions(+), 5 deletions(-)
>> create mode 100644 test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
>>
>> diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
>> index 859603bd..9cace0bc 100644
>> --- a/.github/workflows/exotic-builds-testing.yml
>> +++ b/.github/workflows/exotic-builds-testing.yml
>> @@ -34,7 +34,7 @@ jobs:
>> BUILDTYPE: [Debug, Release]
>> ARCH: [ARM64, x86_64]
>> GC64: [ON, OFF]
>> - FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
>> + FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind, avx512]
> Please sort entries alphabetically.
Done.
--- a/.github/workflows/exotic-builds-testing.yml
+++ b/.github/workflows/exotic-builds-testing.yml
@@ -34,7 +34,7 @@ jobs:
BUILDTYPE: [Debug, Release]
ARCH: [ARM64, x86_64]
GC64: [ON, OFF]
- FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind, avx512]
+ FLAVOR: [ avx512, checkhook, dualnum, gdbjit, nojit, nounwind ]
include:
- BUILDTYPE: Debug
CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON
-DLUA_USE_APICHECK=ON
>> include:
>> - BUILDTYPE: Debug
>> CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
>> @@ -50,12 +50,16 @@ jobs:
>> FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
>> - FLAVOR: nounwind
>> FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
>> + - FLAVOR: avx512
>> + CMAKEFLAGS: -DCMAKE_C_FLAGS=-march=skylake-avx512 -DCMAKE_C_COMPILER=gcc
> Is there any guarantee that the given runner has avx512 support?
> If not, our tests will fail due to illegal instruction error.
I'll add a check for AVX support and condition that will skip job if
there is no AVX support.
>> exclude:
>> - ARCH: ARM64
>> GC64: OFF
>> # DUALNUM is default for ARM64, no need for additional testing.
>> - FLAVOR: dualnum
>> ARCH: ARM64
>> + - FLAVOR: avx512
>> + ARCH: ARM64
> Please sort entries alphabetically.
Fixed:
--- a/.github/workflows/exotic-builds-testing.yml
+++ b/.github/workflows/exotic-builds-testing.yml
@@ -34,12 +34,14 @@ jobs:
BUILDTYPE: [Debug, Release]
ARCH: [ARM64, x86_64]
GC64: [ON, OFF]
- FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind, avx512]
+ FLAVOR: [ avx512, checkhook, dualnum, gdbjit, nojit, nounwind ]
include:
- BUILDTYPE: Debug
CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON
-DLUA_USE_APICHECK=ON
- BUILDTYPE: Release
CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
+ - FLAVOR: avx512
+ CMAKEFLAGS: -DCMAKE_C_FLAGS=-march=skylake-avx512
-DCMAKE_C_COMPILER=gcc
- FLAVOR: dualnum
FLAVORFLAGS: -DLUAJIT_NUMMODE=2
- FLAVOR: checkhook
>> runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
>> name: >
>> LuaJIT ${{ matrix.FLAVOR }}
>> 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 a0fb5440..29146113 100644
>> --- a/test/LuaJIT-tests/CMakeLists.txt
>> +++ b/test/LuaJIT-tests/CMakeLists.txt
>> @@ -64,6 +64,15 @@ if(LUAJIT_NO_UNWIND)
>> set(LUAJIT_TEST_TAGS_EXTRA +internal_unwinder)
>> endif()
>>
>> +if(CMAKE_C_FLAGS MATCHES "-march=skylake-avx512")
>> + # Test <bit64.lua> verifies bitwise operations on numbers.
> Maybe we should add FIXME: tag here. Fill free to ignore.
Added.
>> + # There is a known issue - bitop doesn't working in LuaJIT
> Typo: s/working/work/
Fixed.
>
>> + # built with enabled AVX512 instruction set,
> Typo: s/enabled/the enabled/
Fixed.
>
>> + # seehttps://github.com/tarantool/tarantool/issues/6787.
>> + # Hence, skip this when "skylake-avx512" is passed.
>> + set(LUAJIT_TEST_TAGS_EXTRA +avx512)
>> +endif()
>> +
>> add_custom_command(TARGET LuaJIT-tests
>> COMMENT "Running LuaJIT-tests"
>> COMMAND
>> diff --git a/test/LuaJIT-tests/lib/ffi/index b/test/LuaJIT-tests/lib/ffi/index
>> index e3a34e30..e7c168c2 100644
>> --- a/test/LuaJIT-tests/lib/ffi/index
>> +++ b/test/LuaJIT-tests/lib/ffi/index
>> @@ -1,4 +1,4 @@
>> -bit64.lua +luajit>=2.1
>> +bit64.lua +luajit>=2.1 -avx512
> Do we need to skip all tests instead of only failed one?
>
>> cdata_var.lua
>> copy_fill.lua
>> err.lua
>> diff --git a/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
> Please, use "-" instead of "_" as a separator, according to other test
> names.
> Also, the test name is too long. I suggest something like the
> following:
>
> | lj-865-cross-generation-mach-o-file.test.lua
>
> Don't forget to update the testname below as well.
Fixed.
>> new file mode 100644
>> index 00000000..40bd1c74
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
>> @@ -0,0 +1,343 @@
>> +local tap = require('tap')
>> +local test = tap.test('lj-865-fix_generation_of_mach-o_object_files')
>> +
>> +test:plan(4)
>> +
>> +-- The test creates an object file in Mach-O format with LuaJIT
>> +-- bytecode and checks the validness of the object file fields.
> Typo? s/validness/validity/
> IINM, validness is the obsolete form.
Fixed.
>> +--
>> +-- 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 CPU codename.
> Typo: s/CPU codename/a CPU codename/
Fixed
>> +-- Please consult for the available model architecture on
>> +-- GCC Online Documentation [1] for available CPU codenames.
>> +-- and Wikipedia for CPUs with AVX-512 support [2].
> I suggest the following rewording:
>
> | Please consult the GCC Online Documentation [1] for available CPU
> | codenames. Also, see the Wikipedia for CPUs with AVX-512 support [2].
>
> Or the follwing:
>
> | Please take a look at the GCC Online Documentation [1] for available
> | CPU codenames. Also, see the Wikipedia for CPUs with AVX-512 support [2].
>
> Feel free to reword it like you want.
What's wrong with provided text?
Why should I change it?
>> +-- To detect CPU codename execute:
> Typo: s/CPU codename/the CPU 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:
> I suppose you mean "from the original issue". Or we can use
> | -DCMAKE_C_FLAGS="-march=skylake-avx512"
These steps works in our fork too.
>> +--
>> +-- $ make CC=gcc TARGET_CFLAGS='skylake-avx512' -f Makefile.original
>> +-- $ 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 bytetcode.
> Comment width is more than 66 symbols.
Fixed.
>
>> +-- 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.
>> +--
>> +-- Format of the Mach-O is described in the document
> Typo: s/Format/The format/
Fixed.
>
>> +-- "OS X ABI Mach-O File Format Reference", published by Apple company.
> Typo? s/Reference",/Reference,"/
Don't get it. Why comma should be inside quotes?
>
>> +-- Copy of the (now removed) official documentation can be found
> Typo: s/Copy/The copy/
Fixed.
>
>> +-- here [1]. Yet another source of thruth are XNU headers,
> Typo: s/thruth/truth/
Fixed.
>
>
>> +-- see definition of C-structures in: [2] (`nlist_64`),
> Typo: s/definition/the definition/
Fixed.
>
>> +-- [3] (`fat_arch` and `fat_header`).
>> +--
>> +-- 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 [6].
> Typo: s/in/in the/g
Fixed.
>
>> +-- Below is schematic structure of Universal Binary that
> Typo: s/schematic/the schematic/
> Typo: s/Binary that/Binary, which/
Fixed.
>
>> +-- 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
>> +-- ---------------------------------------
> Side note: This diagramm is amazing thanks!
>
>> +--
>> +-- < x86 executable >
>> +--
>> +--
> Nit: excess empty line.
Removed.
>> +-- 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
>> +{
>> + mach_header; uint32_t reserved;
>> +} mach_header_64;
>> +
>> +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 {
>> + 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;
>> + uint32_t offset, align, reloff, nreloc, flags;
>> + 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;
>> +
>> +typedef struct {
>> + int32_t strx;
>> + uint8_t type, sect;
>> + int16_t desc;
>> + uint32_t value;
>> +} mach_nlist;
>> +
>> +typedef struct {
>> + uint32_t strx;
> Should we use here `int32_t` as it is after the changes?
Yes, fixed.
>
>> + uint8_t type, sect;
>> + uint16_t desc;
>> + uint64_t value;
>> +} mach_nlist_64;
>> +
>> +typedef struct
>> +{
>> + uint32_t magic, nfat_arch;
> Should we use here `int32_t` as it is after the changes?
Fixed.
>
>> +} mach_fat_header;
>> +
>> +typedef struct
>> +{
>> + uint32_t cputype, cpusubtype, offset, size, align;
> Should we use here `int32_t` as it is after the changes?
Fixed.
>
>> +} 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;
>> +
>> +typedef struct {
>> + mach_fat_header fat;
>> + mach_fat_arch fat_arch[2];
>> + struct {
>> + mach_header_64 hdr;
>> + mach_segment_command_64 seg;
>> + mach_section_64 sec;
>> + mach_symtab_command sym;
>> + } arch[2];
>> + mach_nlist_64 sym_entry;
>> + uint8_t space[4096];
>> +} mach_fat_obj_64;
> Should this part be defined in the next patch, since there is no such
> cdef at the moment?
>
> OTOH, it makes test more arch-agnostic. So, the adding of the other case
> is more simple.
Moved to the third patch.
>> +]]
>> +
>> +local function create_obj_file(name, arch)
>> + local mach_o_path = os.tmpname() .. '.o'
>> + local lua_path = os.getenv('LUA_PATH')
>> + local lua_bin = require('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, hw_arch)
>> + local res = {
>> + header = {
>> + magic = 0,
>> + nfat_arch = 0,
>> + },
>> + fat_arch = {},
>> + }
>> +
>> + local is64 = hw_arch == 'arm64'
>> +
>> + -- Mach-O FAT object.
>> + local mach_fat_obj_type = ffi.typeof(is64 and
>> + 'mach_fat_obj_64 *' or 'mach_fat_obj *')
>> + local obj = ffi.cast(mach_fat_obj_type, buf)
>> +
>> + -- Mach-O FAT object header.
>> + local mach_fat_header_type = ffi.typeof('mach_fat_header *')
>> + local mach_fat_header = ffi.cast(mach_fat_header_type, obj.fat)
> Looks like this cast is excess:
> | src/luajit -e '
> | local ffi = require"ffi"
> | ffi.cdef[[
> | struct test {int a; int b;};
> | struct test_outer { struct test t; int c;};
> | ]]
> | local s = ffi.new("struct test_outer", {{0}, 0})
> | print(s.t.a)
> | '
> | 0
Code in the test casts cdata to ffi type, you snippet demonstrate how to
create cdata using ffi.new.
It is similar cases?
>
>> + local be32 = bit.bswap -- Mach-O FAT is BE, target arch is LE.
> Please, use the separate line for the comment.
Moved to a new line.
>
>> + res.header.magic = be32(mach_fat_header.magic)
>> + res.header.nfat_arch = be32(mach_fat_header.nfat_arch)
>> +
>> + -- Mach-O FAT object arches.
>> + local mach_fat_arch_type = ffi.typeof('mach_fat_arch *')
>> + for i = 0, res.header.nfat_arch - 1 do
>> + local fat_arch = ffi.cast(mach_fat_arch_type, obj.fat_arch[i])
> Looks like this cast is excess.
>
>> + 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
> Typo: s/For simplicity/For simplicity,/
Fixed.
> Nit: The comment line is "underfilled".
Fixed.
>
>> +-- sum of CPU types and CPU subtypes.
> Typo: s/sum/the sum/
Fixed.
>
>> +-- Numbers below are defined in <src/jit/bcsave.lua:bcsave_machobj>.
> Typo: s/Numbers/The numbers/
There is no space for "<src/jit/bcsave.lua:bcsave_machob>" on the line
after adding "The"as you requested. And line become underfileld if
"<src/jit/bcsave.lua:bcsave_machob>" moved to the next line. How do it
better:
leave a free space on the line or left "Number" without article?
>
>> +-- Original source is XNU source code, <osfmk/mach/machine.h> [1].
> Typo: s/Original/The original/
> Typo: s/XNU/the XNU/
the same as above, with added articles line looks underfilled and
therefore it looks ugly.
>
>> +--
>> +-- 1.https://opensource.apple.com/source/xnu/xnu-4570.41.2/osfmk/mach/machine.h.auto.html
>> +local sum_cputype = {
>> + x86 = 7,
>> + x64 = 0x01000007,
>> + arm = 7 + 12,
>> + arm64 = 0x01000007 + 0x0100000c,
>> +}
>> +local sum_cpusubtype = {
>> + x86 = 3,
>> + x64 = 3,
>> + arm = 3 + 9,
>> + arm64 = 3 + 0,
>> +}
> I suggest the following for the clarification:
>
> | local cputype = {
> | x86 = 7,
> | x64 = 0x01000007,
> | arm = 7,
> | arm64 = 0x0100000c,
> | }
> | local cpusubtype = {
> | x86 = 3,
> | x64 = 3,
> | arm = 9,
> | arm64 = 0,
> | }
>
> And declare next:
>
> | local sum_cputype = {
> | arm = cputype.arm + cputype.x86,
> | arm64 = cputype.arm64 + cputype.x64,
> | }
> | local sum_cpusubtype = {
> | arm = cpusubtype.arm + cpusubtype.x86,
> | arm64 = cpusubtype.arm64 + cpusubtype.x64,
> | }
You proposed changes breaks the test.
See original dictionaries
https://github.com/tarantool/luajit/blob/ecc45358ddab52c2d0100c74da8efddef2aaf365/src/jit/bcsave.lua#L512-L513
local cputype = ({ x86={7}, x64={0x01000007}, arm={7,12},
arm64={0x01000007,0x0100000c} })[ctx.arch]
converted to sum_cputype = { x86 = 7, x64 = 0x01000007, arm = 7 + 12,
arm64 = 0x01000007 + 0x0100000c}
local cpusubtype = ({ x86={3}, x64={3}, arm={3,9}, arm64={3,0}
})[ctx.arch]
converted to sum_cpusubtype = { x86 = 3, x64 = 3, arm = 3 + 9, arm64 = 3
+ 0 }
I don't know how to make this place in the test more clear. See comment
that explains these arrays:
-- Universal Binary can contain executables for more than one
-- CPU architecture. For simplicity, the test compares the sum of
-- CPU types and CPU subtypes.
-- Numbers below are defined in <src/jit/bcsave.lua:bcsave_machobj>.
-- The original source is the XNU source code,
-- <osfmk/mach/machine.h> [1].
--
Ignored.
>
> Also, it is preferable to use upper case since values are constants.
Made everything uppercase.
>
>> +
>> +-- The function builds Mach-O FAT object file and retrieves
>> +-- its header fields (magic and nfat_arch)
>> +-- and fields of the each arch (cputype, cpusubtype).
> Typo: s/the each/each/
Fixed. Also line above was underfilled, fixed that.
>
>> +--
>> +-- Mach-O FAT object header can be retrieved with `otool` on macOS:
> Typo: s/Mach-O FAT object/A Mach-O FAT object/
Fixed to "The Mach-O FAT object".
> Typo: s/header/headers/
single header, why "s"?
>> +--
>> +-- $ otool -f empty.o
>> +-- Fat headers
>> +-- fat_magic 0xcafebabe
>> +-- nfat_arch 2
>> +-- <snipped>
> Minor: it is better to separate quoting in the comment like the
> following to simplify reading:
>
> | $ otool -f empty.o
> | Fat headers
> | fat_magic 0xcafebabe
> | nfat_arch 2
> | ...
>
> Feel free to ignore.
Ignored.
>
>> +--
>> +-- 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(hw_arch)
> Maybe it is better to use a subtest here:
> The number of tests is fixed for this function, and it becomes more
> clear when we add the additional arch for testing.
>
>> + assert(hw_arch == 'arm' or hw_arch == 'arm64')
>> +
>> + -- FAT_MAGIC is an integer containing the value 0xCAFEBABE in
>> + -- big-endian byte order format. On a big-endian host CPU,
>> + -- this can be validated using the constant FAT_MAGIC;
>> + -- on a little-endian host CPU, it can be validated using
>> + -- the constant FAT_CIGAM.
>> + --
>> + -- FAT_NARCH is an integer specifying the number of fat_arch
>> + -- data structures that follow. This is the number of
>> + -- architectures contained in this binary.
>> + --
>> + -- See aforementioned "OS X ABI Mach-O File Format Reference".
> Typo: s/aforementioned/the aforementioned/
With added article I need to move name of the reference to a next line,
and line looks under-filled and ugly.
>> + --
> Excess empty line.
It is a visual splitter.
>
>> + local FAT_MAGIC = '0xffffffffcafebabe'
>> + local FAT_NARCH = 2
>> +
>> + local MODULE_NAME = 'lango_team'
>> +
>> + local mach_o_obj_path = create_obj_file(MODULE_NAME, hw_arch)
>> + local mach_o_buf = require('utils').tools.read_file(mach_o_obj_path)
> Please move `require` of the necessary tool to the start of the file to
> be consistent with the code style in tests.
moved.
I found that 20 tests in our repository are not consistent too:
test/tarantool-tests/lj-946-print-errors-from-gc-fin-default.test.lua:local
script = require('utils').exec.makecmd(arg, { redirect = '2>&1' })
test/tarantool-tests/unit-jit-parse.test.lua:local jparse =
require('utils').jit.parse
test/tarantool-tests/lj-981-folding-0.test.lua:local jparse =
require('utils').jit.parse
test/tarantool-tests/lj-366-strtab-correct-size.test.lua: local lua_bin
= require('utils').exec.luacmd(arg):match('%S+')
test/tarantool-tests/lj-366-strtab-correct-size.test.lua:local
elf_content = require('utils').tools.read_file(elf_filename)
test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua:local script =
require('utils').exec.makecmd(arg)
grep:
test/tarantool-tests/.lj-736-BC_UCLO-triggers-infinite-loop.lua.swp:
binary file matches
test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua:local
generators = require('utils').jit.generators
test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua:local
frontend = require('utils').frontend
test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua:local jparse =
require('utils').jit.parse
test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64.test.lua:local
generators = require('utils').jit.generators
test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64.test.lua:local
frontend = require('utils').frontend
test/tarantool-tests/lj-351-print-tostring-number.test.lua:local script
= require('utils').exec.makecmd(arg)
test/tarantool-tests/lj-flush-on-trace.test.lua:local script =
require('utils').exec.makecmd(arg, {
test/tarantool-tests/lj-366-strtab-correct-size.test.lua_: local
lua_bin = require('utils').exec.luacmd(arg):match('%S+')
test/tarantool-tests/lj-366-strtab-correct-size.test.lua_:local
elf_content = require('utils').tools.read_file(elf_filename)
test/tarantool-tests/fix-gc-setupvalue.test.lua:local gcisblack =
require('utils').gc.isblack
test/tarantool-tests/misclib-getmetrics-lapi.test.lua:local MAXNINS =
require('utils').jit.const.maxnins
test/tarantool-tests/gh-4427-ffi-sandwich.test.lua:local script =
require('utils').exec.makecmd(arg, {
test/tarantool-tests/fix-jit-dump-ir-conv.test.lua:local jparse =
require('utils').jit.parse
test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua:local
script = require('utils').exec.makecmd(arg, {
test/tarantool-tests/lj-962-stack-overflow-report.test.lua:local LUABIN
= require('utils').exec.luabin(arg)
>
>> + assert(mach_o_buf == nil or #mach_o_buf ~= 0, 'cannot read an object file')
> Should it be:
> | mach_o_buf ~= nil and #mach_o_buf ~= 0
Fixed to " assert(mach_o_buf ~= nil and #mach_o_buf ~= 0, 'cannot read
an object file')".
>
>> +
>> + local mach_o = read_mach_o(mach_o_buf, hw_arch)
>> +
>> + -- Teardown.
>> + local retcode = os.remove(mach_o_obj_path)
>> + assert(retcode == true, 'remove an object file')
> Minor: I suggest refactoring this as the following (`retcode` isn't used
> anywhere else):
>
> | assert(os.remove(mach_o_obj_path), 'remove an object file')
Updated.
>
>> +
>> + local magic_str = string.format('0x%02x', mach_o.header.magic)
> Why do we need 02 in the format string?
> Also, you may use '%#x' for the same result.
Fixed.
>
>> + test:is(magic_str, FAT_MAGIC,
>> + 'fat_magic is correct in Mach-O, ' .. hw_arch)
>> + test:is(mach_o.header.nfat_arch, FAT_NARCH,
>> + 'nfat_arch is correct in Mach-O, ' .. hw_arch)
>> +
>> + local total_cputype = 0
>> + local total_cpusubtype = 0
>> + for i = 1, mach_o.header.nfat_arch do
> Minor: I suggest using `FAT_NARCH` here since we've checked their
> equality above.
Updated.
>
>> + 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[hw_arch],
>> + 'cputype is correct in Mach-O, ' .. hw_arch)
>> + test:is(total_cpusubtype, sum_cpusubtype[hw_arch],
>> + 'cpusubtype is correct in Mach-O, ' .. hw_arch)
>> +end
>> +
>> +build_and_check_mach_o('arm')
>> +
>> +test:done(true)
>> --
>> 2.44.0
[-- Attachment #2: Type: text/html, Size: 44842 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files.
2024-04-09 11:07 ` Sergey Bronnikov via Tarantool-patches
@ 2024-04-09 12:47 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 24+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-04-09 12:47 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: Sergey Bronnikov, tarantool-patches
Thanks for the fixes!
Please consider my answers below.
May you please resend the patch series after adding the AVX512 checker?
On 09.04.24, Sergey Bronnikov wrote:
> Hi, Sergey
>
> thanks for review! Please see my comment below.
>
>
> On 08.04.2024 18:01, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > I'll proceed with the version from the branch.
> >
> > Please consider my comments below.
> >
> > Please rebase your branch to the tarantool/master and solve the
> > conflicts.
> >
> > On 26.03.24, Sergey Bronnikov wrote:
> >> Subject: [PATCH 2/3] OSX/iOS/ARM64: Fix generation of Mach-O object files.
> >>
> >> Thanks to Carlo Cabrera.
> >>
> >> (cherry picked from commit 3065c910ad6027031aabe2dfd3c26a3d0f014b4f)
> >>
<snipped>
> > Is there any guarantee that the given runner has avx512 support?
> > If not, our tests will fail due to illegal instruction error.
>
> I'll add a check for AVX support and condition that will skip job if
> there is no AVX support.
Looking forward for it.
>
<snipped>
> --- a/.github/workflows/exotic-builds-testing.yml
> +++ b/.github/workflows/exotic-builds-testing.yml
<snipped>
> >> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
> >> index a0fb5440..29146113 100644
> >> --- a/test/LuaJIT-tests/CMakeLists.txt
> >> +++ b/test/LuaJIT-tests/CMakeLists.txt
<snipped>
> >> diff --git a/test/LuaJIT-tests/lib/ffi/index b/test/LuaJIT-tests/lib/ffi/index
> >> index e3a34e30..e7c168c2 100644
> >> --- a/test/LuaJIT-tests/lib/ffi/index
> >> +++ b/test/LuaJIT-tests/lib/ffi/index
> >> @@ -1,4 +1,4 @@
> >> -bit64.lua +luajit>=2.1
> >> +bit64.lua +luajit>=2.1 -avx512
> > Do we need to skip all tests instead of only failed one?
What about this?
> >
> >> cdata_var.lua
> >> copy_fill.lua
> >> err.lua
> >> diff --git a/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua
<snipped>
> >> +-- Please consult for the available model architecture on
> >> +-- GCC Online Documentation [1] for available CPU codenames.
> >> +-- and Wikipedia for CPUs with AVX-512 support [2].
> > I suggest the following rewording:
> >
> > | Please consult the GCC Online Documentation [1] for available CPU
> > | codenames. Also, see the Wikipedia for CPUs with AVX-512 support [2].
> >
> > Or the follwing:
> >
> > | Please take a look at the GCC Online Documentation [1] for available
> > | CPU codenames. Also, see the Wikipedia for CPUs with AVX-512 support [2].
> >
> > Feel free to reword it like you want.
>
> What's wrong with provided text?
>
> Why should I change it?
Quillbot[1] suggests dropping the "for" preposition since "it is not
needed to show relationships between nouns and other words in the
sentence." Also, I found that "consult for" lacks the semantics of the
sentence you want to tell [2][3]. So I suggest rephrasing this part as
"consult the" or "consult with" or "take a look", whatever you prefer.
Regarding the part:
| CPU codenames. and Wikipedia
This dot in the middle looks clumsy, so I suggest joining sentences
with the parenthesis "Also".
>
<snipped>
> >> +--
> >> +-- Manual steps for reproducing are the following:
> > I suppose you mean "from the original issue". Or we can use
> > | -DCMAKE_C_FLAGS="-march=skylake-avx512"
> These steps works in our fork too.
But this is unsupported by us build, we prefer to declare CMake flags
instead, don't we?
> >> +--
<snipped>
> >> +typedef struct {
> >> + mach_fat_header fat;
> >> + mach_fat_arch fat_arch[2];
> >> + struct {
> >> + mach_header_64 hdr;
> >> + mach_segment_command_64 seg;
> >> + mach_section_64 sec;
> >> + mach_symtab_command sym;
> >> + } arch[2];
> >> + mach_nlist_64 sym_entry;
> >> + uint8_t space[4096];
> >> +} mach_fat_obj_64;
> > Should this part be defined in the next patch, since there is no such
> > cdef at the moment?
> >
> > OTOH, it makes test more arch-agnostic. So, the adding of the other case
> > is more simple.
> Moved to the third patch.
Since this part has been moved, should we move all arm64-related
declarations and their usage to the third patch to be consistent? (*)
> >> +]]
<snipped>
> >> +
> >> + local is64 = hw_arch == 'arm64'
> >> +
> >> + -- Mach-O FAT object.
> >> + local mach_fat_obj_type = ffi.typeof(is64 and
> >> + 'mach_fat_obj_64 *' or 'mach_fat_obj *')
(*) Like here.
> >> + local obj = ffi.cast(mach_fat_obj_type, buf)
> >> +
> >> + -- Mach-O FAT object header.
> >> + local mach_fat_header_type = ffi.typeof('mach_fat_header *')
> >> + local mach_fat_header = ffi.cast(mach_fat_header_type, obj.fat)
> > Looks like this cast is excess:
> > | src/luajit -e '
> > | local ffi = require"ffi"
> > | ffi.cdef[[
> > | struct test {int a; int b;};
> > | struct test_outer { struct test t; int c;};
> > | ]]
> > | local s = ffi.new("struct test_outer", {{0}, 0})
> > | print(s.t.a)
> > | '
> > | 0
>
> Code in the test casts cdata to ffi type, you snippet demonstrate how to
> create cdata using ffi.new.
>
> It is similar cases?
Yes, since the resulting cdata objects iherit their type as declared in
the parent object.
The following patch works fine for me (**):
===================================================================
diff --git a/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
index 89ef9f33..6d0fef95 100644
--- a/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
+++ b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
@@ -231,17 +231,15 @@ local function read_mach_o(buf, hw_arch)
local obj = ffi.cast(mach_fat_obj_type, buf)
-- Mach-O FAT object header.
- local mach_fat_header_type = ffi.typeof('mach_fat_header *')
- local mach_fat_header = ffi.cast(mach_fat_header_type, obj.fat)
+ local mach_fat_header = obj.fat
-- Mach-O FAT is BE, target arch is LE.
local be32 = bit.bswap
res.header.magic = be32(mach_fat_header.magic)
res.header.nfat_arch = be32(mach_fat_header.nfat_arch)
-- Mach-O FAT object arches.
- local mach_fat_arch_type = ffi.typeof('mach_fat_arch *')
for i = 0, res.header.nfat_arch - 1 do
- local fat_arch = ffi.cast(mach_fat_arch_type, obj.fat_arch[i])
+ local fat_arch = obj.fat_arch[i]
local arch = {
cputype = be32(fat_arch.cputype),
cpusubtype = be32(fat_arch.cpusubtype),
===================================================================
| $ ctest -R lj-865
| Test project /home/burii/reviews/luajit/mach-o-fix
| Start 190: test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
| 1/1 Test #190: test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua ... Passed 0.04 sec
|
| 100% tests passed, 0 tests failed out of 1
|
| Label Time Summary:
| tarantool-tests = 0.04 sec*proc (1 test)
|
| Total Test time (real) = 0.15 sec
>
<snipped>
> >> + local mach_fat_arch_type = ffi.typeof('mach_fat_arch *')
> >> + for i = 0, res.header.nfat_arch - 1 do
> >> + local fat_arch = ffi.cast(mach_fat_arch_type, obj.fat_arch[i])
> > Looks like this cast is excess.
See (**).
> >
<snipped>
> >
> >> +-- Numbers below are defined in <src/jit/bcsave.lua:bcsave_machobj>.
> > Typo: s/Numbers/The numbers/
>
> There is no space for "<src/jit/bcsave.lua:bcsave_machob>" on the line
>
> after adding "The"as you requested. And line become underfileld if
>
> "<src/jit/bcsave.lua:bcsave_machob>" moved to the next line. How do it
> better:
>
> leave a free space on the line or left "Number" without article?
I suppose something like this will satisfy both of us :)
| -- Universal Binary can contain executables for more than one
| -- CPU architecture. For simplicity, the test compares the sum of
| -- CPU types and CPU subtypes.
| --
| -- <src/jit/bcsave.lua:bcsave_machobj> has the definitions of the
| -- numbers below. The original XNU source code may be found in
| -- <osfmk/mach/machine.h> [1].
| --
| -- 1. https://opensource.apple.com/source/xnu/xnu-4570.41.2/osfmk/mach/machine.h.auto.html
>
> >
> >> +-- Original source is XNU source code, <osfmk/mach/machine.h> [1].
> > Typo: s/Original/The original/
> > Typo: s/XNU/the XNU/
> the same as above, with added articles line looks underfilled and
> therefore it looks ugly.
> >
> >> +--
> >> +-- 1.https://opensource.apple.com/source/xnu/xnu-4570.41.2/osfmk/mach/machine.h.auto.html
> >> +local sum_cputype = {
> >> + x86 = 7,
> >> + x64 = 0x01000007,
> >> + arm = 7 + 12,
> >> + arm64 = 0x01000007 + 0x0100000c,
> >> +}
> >> +local sum_cpusubtype = {
> >> + x86 = 3,
> >> + x64 = 3,
> >> + arm = 3 + 9,
> >> + arm64 = 3 + 0,
> >> +}
> > I suggest the following for the clarification:
> >
> > | local cputype = {
> > | x86 = 7,
> > | x64 = 0x01000007,
> > | arm = 7,
> > | arm64 = 0x0100000c,
> > | }
> > | local cpusubtype = {
> > | x86 = 3,
> > | x64 = 3,
> > | arm = 9,
> > | arm64 = 0,
> > | }
> >
> > And declare next:
> >
> > | local sum_cputype = {
> > | arm = cputype.arm + cputype.x86,
> > | arm64 = cputype.arm64 + cputype.x64,
> > | }
> > | local sum_cpusubtype = {
> > | arm = cpusubtype.arm + cpusubtype.x86,
> > | arm64 = cpusubtype.arm64 + cpusubtype.x64,
> > | }
>
> You proposed changes breaks the test.
My bad that was an error at copipasting here
The following patch works OK for me:
===================================================================
diff --git a/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
index 89ef9f33..c1fbe635 100644
--- a/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
+++ b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
@@ -260,18 +258,28 @@ end
-- <osfmk/mach/machine.h> [1].
--
-- 1. https://opensource.apple.com/source/xnu/xnu-4570.41.2/osfmk/mach/machine.h.auto.html
---
-local sum_cputype = {
+
+local cputype = {
x86 = 7,
x64 = 0x01000007,
- arm = 7 + 12,
- arm64 = 0x01000007 + 0x0100000c,
+ arm = 12,
+ arm64 = 0x0100000c,
}
-local sum_cpusubtype = {
+
+local cpusubtype = {
x86 = 3,
x64 = 3,
- arm = 3 + 9,
- arm64 = 3 + 0,
+ arm = 9,
+ arm64 = 0,
+}
+
+local sum_cputype = {
+ arm = cputype.arm + cputype.x86,
+ arm64 = cputype.arm64 + cputype.x64,
+}
+local sum_cpusubtype = {
+ arm = cpusubtype.arm + cpusubtype.x86,
+ arm64 = cpusubtype.arm64 + cpusubtype.x64,
}
===================================================================
>
> See original dictionaries
>
> https://github.com/tarantool/luajit/blob/ecc45358ddab52c2d0100c74da8efddef2aaf365/src/jit/bcsave.lua#L512-L513
>
> local cputype = ({ x86={7}, x64={0x01000007}, arm={7,12},
> arm64={0x01000007,0x0100000c} })[ctx.arch]
>
>
> converted to sum_cputype = { x86 = 7, x64 = 0x01000007, arm = 7 + 12,
> arm64 = 0x01000007 + 0x0100000c}
>
>
> local cpusubtype = ({ x86={3}, x64={3}, arm={3,9}, arm64={3,0}
> })[ctx.arch]
>
>
> converted to sum_cpusubtype = { x86 = 3, x64 = 3, arm = 3 + 9, arm64 = 3
> + 0 }
>
>
> I don't know how to make this place in the test more clear. See comment
> that explains these arrays:
Yes, I understand what these arrays are and how they are represented
here. I don't like the idea of copy-pasting the value of x86/x64
cputype/cpusubtype in the summ array.
Since it is only a suggestion (due to a matter of taste), feel free to
ignore.
>
>
> -- Universal Binary can contain executables for more than one
> -- CPU architecture. For simplicity, the test compares the sum of
> -- CPU types and CPU subtypes.
> -- Numbers below are defined in <src/jit/bcsave.lua:bcsave_machobj>.
> -- The original source is the XNU source code,
> -- <osfmk/mach/machine.h> [1].
> --
>
>
> Ignored.
>
> >
> > Also, it is preferable to use upper case since values are constants.
>
>
> Made everything uppercase.
Side note: What part do you change? I mean SUM_CPUTYPE, but I see no changes about
them on the branch.
>
> >
> >> +
> >> +-- The function builds Mach-O FAT object file and retrieves
> >> +-- its header fields (magic and nfat_arch)
> >> +-- and fields of the each arch (cputype, cpusubtype).
> > Typo: s/the each/each/
> Fixed. Also line above was underfilled, fixed that.
Nice, thanks.
> >
> >> +--
> >> +-- Mach-O FAT object header can be retrieved with `otool` on macOS:
> > Typo: s/Mach-O FAT object/A Mach-O FAT object/
> Fixed to "The Mach-O FAT object".
> > Typo: s/header/headers/
> single header, why "s"?
My bad. Just ignore this comment.
> >> +--
<snipped>
> >> +local function build_and_check_mach_o(hw_arch)
> > Maybe it is better to use a subtest here:
> > The number of tests is fixed for this function, and it becomes more
> > clear when we add the additional arch for testing.
What do you think about this?
> >
> >> + assert(hw_arch == 'arm' or hw_arch == 'arm64')
(*) And like here.
> >> +
> >> + -- FAT_MAGIC is an integer containing the value 0xCAFEBABE in
> >> + -- big-endian byte order format. On a big-endian host CPU,
> >> + -- this can be validated using the constant FAT_MAGIC;
> >> + -- on a little-endian host CPU, it can be validated using
> >> + -- the constant FAT_CIGAM.
> >> + --
> >> + -- FAT_NARCH is an integer specifying the number of fat_arch
> >> + -- data structures that follow. This is the number of
> >> + -- architectures contained in this binary.
> >> + --
> >> + -- See aforementioned "OS X ABI Mach-O File Format Reference".
> > Typo: s/aforementioned/the aforementioned/
>
> With added article I need to move name of the reference to a next line,
>
> and line looks under-filled and ugly.
For me the following formatting looks OK:
| -- FAT_NARCH is an integer specifying the number of fat_arch
| -- data structures that follow. This is the number of
| -- architectures contained in this binary.
| --
| -- See the aforementioned "OS X ABI Mach-O File Format
| -- Reference".
| local FAT_MAGIC = '0xffffffffcafebabe'
| local FAT_NARCH = 2
>
> >> + --
> > Excess empty line.
>
> It is a visual splitter.
So we may not use a comment here?
>
>
> >
> >> + local FAT_MAGIC = '0xffffffffcafebabe'
> >> + local FAT_NARCH = 2
> >> +
> >> + local MODULE_NAME = 'lango_team'
> >> +
> >> + local mach_o_obj_path = create_obj_file(MODULE_NAME, hw_arch)
> >> + local mach_o_buf = require('utils').tools.read_file(mach_o_obj_path)
> > Please move `require` of the necessary tool to the start of the file to
> > be consistent with the code style in tests.
>
> moved.
>
>
> I found that 20 tests in our repository are not consistent too:
I found only 8 (at tarantool/master branch):
| grep -P "require\('utils'\).*\(" -R test/tarantool-tests/
| test/tarantool-tests/lj-366-strtab-correct-size.test.lua: local lua_bin = require('utils').exec.luacmd(arg):match('%S+')
| test/tarantool-tests/fix-argv2ctype-cts-L-init.test.lua:local script = require('utils').exec.makecmd(arg)
| test/tarantool-tests/lj-946-print-errors-from-gc-fin-default.test.lua:local script = require('utils').exec.makecmd(arg, { redirect = '2>&1' })
| test/tarantool-tests/lj-962-stack-overflow-report.test.lua:local LUABIN = require('utils').exec.luabin(arg)
| test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua:local script = require('utils').exec.makecmd(arg, {
| test/tarantool-tests/lj-351-print-tostring-number.test.lua:local script = require('utils').exec.makecmd(arg)
| test/tarantool-tests/lj-flush-on-trace.test.lua:local script = require('utils').exec.makecmd(arg, {
| test/tarantool-tests/gh-4427-ffi-sandwich.test.lua:local script = require('utils').exec.makecmd(arg, {
Your listing also includes local definitions (such as those I proposed).
AFAICS, only `makecmd` and `luacmd` are used in a different way, and it
may definitely be refactored later if it is crucial for us.
<snipped>
> >> 2.44.0
[1]: https://quillbot.com/grammar-check
[2]: https://www.multitran.com/m.exe?l1=2&l2=1&s=consult%20for
[3]: https://translate.google.com/?sl=en&tl=ru&text=consult%20for%0A&op=translate
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file.
2024-04-08 15:16 ` Sergey Kaplun via Tarantool-patches
@ 2024-04-11 7:56 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 0 replies; 24+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-04-11 7:56 UTC (permalink / raw)
To: Sergey Kaplun, Sergey Bronnikov; +Cc: tarantool-patches
Sergey, thanks for review!
On 08.04.2024 18:16, Sergey Kaplun wrote:
> Hi, Sergey!
> The version on branch LGTM, except a single nit regarding the commit
> message below.
>
> On 14.03.24, Sergey Bronnikov wrote:
>> From: sergeyb@tarantool.org
>>
>> Thanks to Carlo Cabrera.
>>
>> (cherry picked from commit b98b37231bd2dcb79e10b0f974cefd91eb0d7b3a)
>>
>> Mach-O FAR object files generated by LuaJIT for arm64 had an incorrect
> Typo: s/FAR/FAT/
It was already highlighted by Maxim,
see https://lists.tarantool.org/tarantool-patches/ZhPrxdZKzOVXVChp@root/T/#t
>> format because FFI structure used for generation was wrong:
>> `mach_fat_obj` instead of `mach_fat_obj_64`.
> I suggest reformulating this as the following:
>
> | 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.
Updated.
>> Sergey Bronnikov:
>> * added the description and a test for the problem
>>
>> Part of tarantool/tarantool#9595
>> ---
>> src/jit/bcsave.lua | 14 +++++++++++++-
>> ...-fix_generation_of_mach-o_object_files.test.lua | 4 +++-
>> 2 files changed, 16 insertions(+), 2 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
> <snipped>
>
>> 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 0519e134..0a11f163 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
> <snipped>
>
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes
2024-04-08 7:47 ` [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes Sergey Kaplun via Tarantool-patches
2024-04-08 13:06 ` Sergey Kaplun via Tarantool-patches
@ 2024-04-11 8:08 ` Sergey Bronnikov via Tarantool-patches
2024-04-11 8:27 ` Sergey Kaplun via Tarantool-patches
1 sibling, 1 reply; 24+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-04-11 8:08 UTC (permalink / raw)
To: Sergey Kaplun, Sergey Bronnikov; +Cc: tarantool-patches
Hi,
On 08.04.2024 10:47, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch set!
>
> I'll proceed with the review according version on the branch:
>
> ===================================================================
>> Subject: [PATCH 1/3] test: introduce a helper read_file
> This patch LGTM, after fixing several nits below.
>
>> 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.
> So, missed the ticket mentioning:
> | Needed for tarantool/tarantool#9595
It is actually not needed for 9595.
>
>> ---
>> .../lj-366-strtab-correct-size.test.lua | 10 +---------
>> test/tarantool-tests/utils/tools.lua | 8 ++++++++
>> 2 files changed, 9 insertions(+), 9 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..0bb92da6 100644
>> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> @@ -138,14 +138,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 +164,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)
> Minor: I suggest avoiding the change at this line.
> According to codestyle in other tests, we require helpers separately,
> even if they are used only once.
Where can I read a codestyle guide which you want to follow?
>
>> 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..26b8c08d 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 in a specified path and returns its content.
> Typo: s/in/at/
Everything fine from Quillbot point of view.
>
>> +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.44.0
> ===================================================================
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes
2024-04-11 8:08 ` Sergey Bronnikov via Tarantool-patches
@ 2024-04-11 8:27 ` Sergey Kaplun via Tarantool-patches
2024-04-11 12:39 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-04-11 8:27 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: Sergey Bronnikov, tarantool-patches
Hi,
On 11.04.24, Sergey Bronnikov wrote:
> Hi,
>
> On 08.04.2024 10:47, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the patch set!
> >
> > I'll proceed with the review according version on the branch:
> >
> > ===================================================================
> >> Subject: [PATCH 1/3] test: introduce a helper read_file
> > This patch LGTM, after fixing several nits below.
> >
> >> 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.
> > So, missed the ticket mentioning:
> > | Needed for tarantool/tarantool#9595
> It is actually not needed for 9595.
OK, not insisting.
> >
> >> ---
> >> .../lj-366-strtab-correct-size.test.lua | 10 +---------
> >> test/tarantool-tests/utils/tools.lua | 8 ++++++++
> >> 2 files changed, 9 insertions(+), 9 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..0bb92da6 100644
> >> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> >> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
> >> @@ -138,14 +138,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 +164,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)
> > Minor: I suggest avoiding the change at this line.
> > According to codestyle in other tests, we require helpers separately,
> > even if they are used only once.
> Where can I read a codestyle guide which you want to follow?
This is just a general tradition for all test files. Usually, it is a
good thing to do it this way because we don't want to use a lookup for
the module every time the function is called. This is not related to
your case directly, but if somebody moves this code to the function, he
should also change this part anyway. Plus, as a bonus, we will get an
earlier failure if the `read_file` module doesn't exist.
> >
> >> 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..26b8c08d 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 in a specified path and returns its content.
> > Typo: s/in/at/
>
> Everything fine from Quillbot point of view.
But I have it, see [1].
Sometimes it freezes, so you should reload the current page.
>
>
> >
> >> +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.44.0
> > ===================================================================
> >
[1]: https://ibb.co/XzCPC13
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes
2024-04-11 8:27 ` Sergey Kaplun via Tarantool-patches
@ 2024-04-11 12:39 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 0 replies; 24+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-04-11 12:39 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: Sergey Bronnikov, tarantool-patches
Hi,
On 11.04.2024 11:27, Sergey Kaplun wrote:
> Hi,
>
> On 11.04.24, Sergey Bronnikov wrote:
>> Hi,
>>
>> On 08.04.2024 10:47, Sergey Kaplun wrote:
>>> Hi, Sergey!
>>> Thanks for the patch set!
>>>
>>> I'll proceed with the review according version on the branch:
>>>
>>> ===================================================================
>>>> Subject: [PATCH 1/3] test: introduce a helper read_file
>>> This patch LGTM, after fixing several nits below.
>>>
>>>> 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.
>>> So, missed the ticket mentioning:
>>> | Needed for tarantool/tarantool#9595
>> It is actually not needed for 9595.
> OK, not insisting.
>
>>>> ---
>>>> .../lj-366-strtab-correct-size.test.lua | 10 +---------
>>>> test/tarantool-tests/utils/tools.lua | 8 ++++++++
>>>> 2 files changed, 9 insertions(+), 9 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..0bb92da6 100644
>>>> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>>>> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>>>> @@ -138,14 +138,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 +164,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)
>>> Minor: I suggest avoiding the change at this line.
>>> According to codestyle in other tests, we require helpers separately,
>>> even if they are used only once.
>> Where can I read a codestyle guide which you want to follow?
> This is just a general tradition for all test files. Usually, it is a
> good thing to do it this way because we don't want to use a lookup for
> the module every time the function is called. This is not related to
> your case directly, but if somebody moves this code to the function, he
> should also change this part anyway. Plus, as a bonus, we will get an
> earlier failure if the `read_file` module doesn't exist.
>
So, the answer is "we don't have nor a codestyle guide
nor a static analysis that will force a certain style".
That's why we always get stuck with things like this in code review.
I've updated as you suggested.
--- 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)
>>>> 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..26b8c08d 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 in a specified path and returns its content.
>>> Typo: s/in/at/
>> Everything fine from Quillbot point of view.
> But I have it, see [1].
> Sometimes it freezes, so you should reload the current page.
Seems so.
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.44.0
>>> ===================================================================
>>>
> [1]: https://ibb.co/XzCPC13
>
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2024-04-11 12:39 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-14 11:39 [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes Sergey Bronnikov via Tarantool-patches
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 1/2] OSX/iOS/ARM64: Fix generation of Mach-O object files Sergey Bronnikov via Tarantool-patches
2024-03-18 12:53 ` Maxim Kokryashkin via Tarantool-patches
2024-03-19 8:19 ` Sergey Bronnikov via Tarantool-patches
2024-03-19 16:28 ` Maxim Kokryashkin via Tarantool-patches
2024-03-26 13:53 ` Sergey Bronnikov via Tarantool-patches
2024-03-26 15:44 ` Maxim Kokryashkin via Tarantool-patches
2024-04-08 15:01 ` Sergey Kaplun via Tarantool-patches
2024-04-09 11:07 ` Sergey Bronnikov via Tarantool-patches
2024-04-09 12:47 ` Sergey Kaplun via Tarantool-patches
2024-03-18 12:55 ` Maxim Kokryashkin via Tarantool-patches
2024-03-14 11:39 ` [Tarantool-patches] [PATCH luajit 2/2] OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file Sergey Bronnikov via Tarantool-patches
2024-03-18 13:44 ` Maxim Kokryashkin via Tarantool-patches
2024-03-19 8:22 ` Sergey Bronnikov via Tarantool-patches
2024-03-19 16:15 ` Maxim Kokryashkin via Tarantool-patches
2024-03-26 14:01 ` Sergey Bronnikov via Tarantool-patches
2024-03-26 15:45 ` Maxim Kokryashkin via Tarantool-patches
2024-04-08 15:16 ` Sergey Kaplun via Tarantool-patches
2024-04-11 7:56 ` Sergey Bronnikov via Tarantool-patches
2024-04-08 7:47 ` [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes Sergey Kaplun via Tarantool-patches
2024-04-08 13:06 ` Sergey Kaplun via Tarantool-patches
2024-04-11 8:08 ` Sergey Bronnikov via Tarantool-patches
2024-04-11 8:27 ` Sergey Kaplun via Tarantool-patches
2024-04-11 12:39 ` Sergey Bronnikov via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox