<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey<br>
</p>
<div class="moz-cite-prefix">On 12.04.2024 14:19, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:ZhkYx7YWe30lu3dJ@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the fixes!
LGTM after fixing a few minor nits below.
On 11.04.24, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">From: Mike Pall <mike>
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">---
src/jit/bcsave.lua | 6 +-
test/LuaJIT-tests/CMakeLists.txt | 9 +
...-865-cross-generation-mach-o-file.test.lua | 300 ++++++++++++++++++
3 files changed, 312 insertions(+), 3 deletions(-)
create mode 100644 test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua
index a287d675..7aec1555 100644
--- a/src/jit/bcsave.lua
+++ b/src/jit/bcsave.lua
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
index b8e4dfc4..6d073700 100644
--- a/test/LuaJIT-tests/CMakeLists.txt
+++ b/test/LuaJIT-tests/CMakeLists.txt
@@ -52,6 +52,15 @@ if(LUAJIT_NO_UNWIND)
set(LUAJIT_TEST_TAGS_EXTRA +internal_unwinder)
endif()
+if(CMAKE_C_FLAGS MATCHES "-march=skylake-avx512")
+ # FIXME: Test <bit64.lua> verifies bitwise operations on numbers.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: comment line width is more than 66 symbols.
</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZhkYx7YWe30lu3dJ@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ # There is a known issue - bitop doesn't work in LuaJIT built
+ # with the enabled AVX512 instruction set, see
+ # <a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/6787">https://github.com/tarantool/tarantool/issues/6787</a>.
+ # Hence, skip this when "skylake-avx512" is passed.
+ set(LUAJIT_TEST_TAGS_EXTRA +avx512)
+endif()
+
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Should this be a part of the first commit?</pre>
</blockquote>
<p>Moved to the first commit.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZhkYx7YWe30lu3dJ@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> set(TEST_SUITE_NAME "LuaJIT-tests")
# XXX: The call produces both test and target <LuaJIT-tests-deps>
diff --git a/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
new file mode 100644
index 00000000..04fb5495
--- /dev/null
+++ b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
@@ -0,0 +1,300 @@
+local tap = require('tap')
+local test = tap.test('lj-865-cross-generation-mach-o-file')
+local utils = require('utils')
+
+test:plan(1)
+
+-- The test creates an object file in Mach-O format with LuaJIT
+-- bytecode and checks the validity of the object file fields.
+--
+-- The original problem is reproduced with LuaJIT that built with
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/LuaJIT that built/LuaJIT, which is built/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZhkYx7YWe30lu3dJ@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+-- enabled AVX512F instructions. The support for AVX512F could be
+-- checked in `/proc/cpuinfo` on Linux and
+-- `sysctl hw.optional.avx512f` on Mac. AVX512F must be
+-- implicitly enabled in a C compiler by passing a CPU codename.
+-- Please take a look at the GCC Online Documentation [1] for
+-- available CPU codenames. Also, see the Wikipedia for CPUs with
+-- AVX-512 support [2].
+-- To detect the CPU codename execute:
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/codename/codename,/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZhkYx7YWe30lu3dJ@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+-- `gcc -march=native -Q --help=target | grep march`.
+--
+-- 1. <a class="moz-txt-link-freetext" href="https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html">https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html</a>
+-- 2. <a class="moz-txt-link-freetext" href="https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512">https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512</a>
+--
+-- Manual steps for reproducing are the following:
+--
+-- $ CC=gcc TARGET_CFLAGS='skylake-avx512' cmake -S . -B build
+-- $ cmake --build build --parallel
+-- $ echo > test.lua
+-- $ LUA_PATH="src/?.lua;;" luajit -b -o osx -a arm test.lua test.o
+-- $ file test.o
+-- empty.o: DOS executable (block device driver)
+
+local ffi = require('ffi')
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: Why do we require the ffi here alongside from others requires?
</pre>
</blockquote>
Moved to another place.<br>
<blockquote type="cite" cite="mid:ZhkYx7YWe30lu3dJ@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+-- LuaJIT can generate so called Universal Binary with Lua
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+--
+-- There are a good visual representation of Universal Binary
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/are/is/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZhkYx7YWe30lu3dJ@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+-- in "Mac OS X Internals" book (pages 67-68) [5] and in the [6].
+-- Below is the schematic structure of Universal Binary, which
+-- includes two executables for PowerPC and Intel i386 (omitted):
+--
+-- 0x0000000 ---------------------------------------
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+local function create_obj_file(name, arch)
+ local mach_o_path = os.tmpname() .. '.o'
+ local lua_path = os.getenv('LUA_PATH')
+ local lua_bin = utils.exec.luacmd(arg):match('%S+')
+ local cmd_fmt = 'LUA_PATH="%s" %s -b -n "%s" -o osx -a %s -e "print()" %s'
+ local cmd = (cmd_fmt):format(lua_path, lua_bin, name, arch, mach_o_path)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: Typo: s/(cmd_fmt)/cmd_fmt/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZhkYx7YWe30lu3dJ@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: The comment line looks underfilled.</pre>
</blockquote>
<span class="HwtZe" lang="en"><span class="jCAhz ChMk0b"><span
class="ryNqvb">Refilled it.<br>
</span></span></span>
<blockquote type="cite" cite="mid:ZhkYx7YWe30lu3dJ@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+-- the FAT magic number and `nfat_arch`.
+local function read_mach_o(buf)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+local SUM_CPUTYPE = {
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: It will be nice to add the comment:
| -- x86 + arm.
</pre>
</blockquote>
Added a comment<br>
<blockquote type="cite" cite="mid:ZhkYx7YWe30lu3dJ@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ arm = 7 + 12,
+}
+local SUM_CPUSUBTYPE = {
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: It will be nice to add the comment:
| -- x86 + arm.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ arm = 3 + 9,
+}
+
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+local function build_and_check_mach_o(subtest, hw_arch)
+ assert(hw_arch == 'arm')
+
+ subtest:plan(4)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+test:test('arm', build_and_check_mach_o, 'arm')
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: we can use `subtest.name` as the definition of the `hw_arch` in the
`build_and_check_mach_o()`, so it helps to avoid duplication of arch
usage.
Matter of taste.
Feel free to ignore.
</pre>
</blockquote>
ignored<br>
<blockquote type="cite" cite="mid:ZhkYx7YWe30lu3dJ@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+test:done(true)
--
2.34.1
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>