From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 6B0F1CE9500; Tue, 16 Apr 2024 18:29:40 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6B0F1CE9500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1713281380; bh=t+wRK+E26m3m577S8EN/3GebqI4sDSIbkzsdeE5QmpM=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=Z6PtD6vVls0wjza/i76h5fuJgMbqApBgQaFaGixr83ZvMwILHi5pUZvDRxwAjAVIy jP6B4Nh6EKRN5T3xNmN8H7ndg8lllwN5L+IrpfdHKwmDIoWgvIVFNKNwargADlxIWn FP+2iM/hOcXtIU0TMrr/kgKTncO36Akxx7zjoSfQ= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [95.163.41.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E92C4CE9500 for ; Tue, 16 Apr 2024 18:29:38 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E92C4CE9500 Received: by smtp45.i.mail.ru with esmtpa (envelope-from ) id 1rwkkb-0000000G85Z-36P6; Tue, 16 Apr 2024 18:29:38 +0300 Content-Type: multipart/alternative; boundary="------------9H09pU1eZzzHj9f0UYZufVja" Message-ID: <052764ac-c566-4551-81e0-777c60cc7845@tarantool.org> Date: Tue, 16 Apr 2024 18:29:35 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun , Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org References: <7bdffd2650a785877e03584e6d532e855d09de8a.1712841312.git.sergeyb@tarantool.org> In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9D327C87852EB66D3FD55D334CC1A2A9D82FA4BDBDE8D80FF182A05F538085040E5FF2E541CEF7F08479CDAE959BF64244830FD42F18D26A8C4421B037CA1C4C948EE0F73B0FEDEF0 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7922D113DFDC6D5A3EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006371ECA800C705ED7E68638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D878BC7D91A4C603327DCEF2CAA8C51C07622ABC6EC951304FCC7F00164DA146DAFE8445B8C89999728AA50765F7900637028599BB38096F4F389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC85FF72824B19451C6F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C2D01283D1ACF37BA040F9FF01DFDA4A84AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C315D302B03A124028BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7ED6B002596C5A4F0731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5D0EC64F7B9C863A45002B1117B3ED69647E9E3430BA049867E0012C66AE17B00823CB91A9FED034534781492E4B8EEAD05E80F4396618BB2BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF321DE7E2CBBA533C36803FF0637D4B164D91A41693C67B76A78B64E9BFB852C12B6ADA43BC89BCA9C84F44966DD9651B2591E1A3B7499D1CF8A66EDFA71B1451EC1ED3A270E0E8765F4332CA8FE04980913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojwhzzK0WJvz2bCvS7Ezcqig== X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B59B6DC1CABCBAA6086479CDAE959BF64244830FD42F18D26A8B7CBEF92542CD7C8795FA72BAB74744FC77752E0C033A69EA16A481184E8BB1C9B38E6EA4F046BE03A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 3/4][v2] OSX/iOS/ARM64: Fix generation of Mach-O object files. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------9H09pU1eZzzHj9f0UYZufVja Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey On 12.04.2024 14:19, Sergey Kaplun via Tarantool-patches wrote: > Hi, Sergey! > Thanks for the fixes! > LGTM after fixing a few minor nits below. > > On 11.04.24, Sergey Bronnikov wrote: >> From: Mike Pall >> > > >> --- >> 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 > > >> 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 verifies bitwise operations on numbers. > Nit: comment line width is more than 66 symbols. Fixed. >> + # There is a known issue - bitop doesn't work in LuaJIT built >> + # with the enabled AVX512 instruction set, see >> + #https://github.com/tarantool/tarantool/issues/6787. >> + # Hence, skip this when "skylake-avx512" is passed. >> + set(LUAJIT_TEST_TAGS_EXTRA +avx512) >> +endif() >> + > Should this be a part of the first commit? Moved to the first commit. > >> set(TEST_SUITE_NAME "LuaJIT-tests") >> >> # XXX: The call produces both test and target >> diff --git a/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua >> new file mode 100644 >> index 00000000..04fb5495 >> --- /dev/null >> +++ b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua >> @@ -0,0 +1,300 @@ >> +local tap = require('tap') >> +local test = tap.test('lj-865-cross-generation-mach-o-file') >> +local utils = require('utils') >> + >> +test:plan(1) >> + >> +-- The test creates an object file in Mach-O format with LuaJIT >> +-- bytecode and checks the validity of the object file fields. >> +-- >> +-- The original problem is reproduced with LuaJIT that built with > Typo: s/LuaJIT that built/LuaJIT, which is built/ Fixed. > >> +-- enabled AVX512F instructions. The support for AVX512F could be >> +-- checked in `/proc/cpuinfo` on Linux and >> +-- `sysctl hw.optional.avx512f` on Mac. AVX512F must be >> +-- implicitly enabled in a C compiler by passing a CPU codename. >> +-- Please take a look at the GCC Online Documentation [1] for >> +-- available CPU codenames. Also, see the Wikipedia for CPUs with >> +-- AVX-512 support [2]. >> +-- To detect the CPU codename execute: > Typo: s/codename/codename,/ Fixed. > >> +-- `gcc -march=native -Q --help=target | grep march`. >> +-- >> +-- 1.https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html >> +-- 2.https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512 >> +-- >> +-- Manual steps for reproducing are the following: >> +-- >> +-- $ CC=gcc TARGET_CFLAGS='skylake-avx512' cmake -S . -B build >> +-- $ cmake --build build --parallel >> +-- $ echo > test.lua >> +-- $ LUA_PATH="src/?.lua;;" luajit -b -o osx -a arm test.lua test.o >> +-- $ file test.o >> +-- empty.o: DOS executable (block device driver) >> + >> +local ffi = require('ffi') > Nit: Why do we require the ffi here alongside from others requires? Moved to another place. >> + >> +-- LuaJIT can generate so called Universal Binary with Lua > > >> +-- >> +-- There are a good visual representation of Universal Binary > Typo: s/are/is/ Fixed. > >> +-- in "Mac OS X Internals" book (pages 67-68) [5] and in the [6]. >> +-- Below is the schematic structure of Universal Binary, which >> +-- includes two executables for PowerPC and Intel i386 (omitted): >> +-- >> +-- 0x0000000 --------------------------------------- > > >> +local function create_obj_file(name, arch) >> + local mach_o_path = os.tmpname() .. '.o' >> + local lua_path = os.getenv('LUA_PATH') >> + local lua_bin = utils.exec.luacmd(arg):match('%S+') >> + local cmd_fmt = 'LUA_PATH="%s" %s -b -n "%s" -o osx -a %s -e "print()" %s' >> + local cmd = (cmd_fmt):format(lua_path, lua_bin, name, arch, mach_o_path) > Nit: Typo: s/(cmd_fmt)/cmd_fmt/ Fixed. > >> + local ret = os.execute(cmd) >> + assert(ret == 0, 'cannot create an object file') >> + return mach_o_path >> +end >> + >> +-- Parses a buffer in the Mach-O format and returns > Nit: The comment line looks underfilled. Refilled it. > >> +-- the FAT magic number and `nfat_arch`. >> +local function read_mach_o(buf) > > >> +local SUM_CPUTYPE = { > Minor: It will be nice to add the comment: > | -- x86 + arm. Added a comment >> + arm = 7 + 12, >> +} >> +local SUM_CPUSUBTYPE = { > Minor: It will be nice to add the comment: > | -- x86 + arm. > >> + arm = 3 + 9, >> +} >> + > > >> +local function build_and_check_mach_o(subtest, hw_arch) >> + assert(hw_arch == 'arm') >> + >> + subtest:plan(4) > > >> +test:test('arm', build_and_check_mach_o, 'arm') > Minor: we can use `subtest.name` as the definition of the `hw_arch` in the > `build_and_check_mach_o()`, so it helps to avoid duplication of arch > usage. > > Matter of taste. > Feel free to ignore. ignored >> + >> +test:done(true) >> -- >> 2.34.1 >> --------------9H09pU1eZzzHj9f0UYZufVja Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Hi, Sergey

On 12.04.2024 14:19, Sergey Kaplun via Tarantool-patches wrote:
Hi, Sergey!
Thanks for the fixes!
LGTM after fixing a few minor nits below.

On 11.04.24, Sergey Bronnikov wrote:

From: Mike Pall <mike>


<snipped>


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

diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua
index a287d675..7aec1555 100644
--- a/src/jit/bcsave.lua
+++ b/src/jit/bcsave.lua

<snipped>


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

Nit: comment line width is more than 66 symbols.

Fixed.



      

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


      

      
Should this be a part of the first commit?

    

    
Moved to the first commit.

    


    

    

      



      

        
 set(TEST_SUITE_NAME "LuaJIT-tests")
 
 # XXX: The call produces both test and target <LuaJIT-tests-deps>
diff --git a/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
new file mode 100644
index 00000000..04fb5495
--- /dev/null
+++ b/test/tarantool-tests/lj-865-cross-generation-mach-o-file.test.lua
@@ -0,0 +1,300 @@
+local tap = require('tap')
+local test = tap.test('lj-865-cross-generation-mach-o-file')
+local utils = require('utils')
+
+test:plan(1)
+
+-- The test creates an object file in Mach-O format with LuaJIT
+-- bytecode and checks the validity of the object file fields.
+--
+-- The original problem is reproduced with LuaJIT that built with


      

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

    

    Fixed.

    

      



      

        
+-- enabled AVX512F instructions. The support for AVX512F could be
+-- checked in `/proc/cpuinfo` on Linux and
+-- `sysctl hw.optional.avx512f` on Mac. AVX512F must be
+-- implicitly enabled in a C compiler by passing a CPU codename.
+-- Please take a look at the GCC Online Documentation [1] for
+-- available CPU codenames. Also, see the Wikipedia for CPUs with
+-- AVX-512 support [2].
+-- To detect the CPU codename execute:


      

      
Typo: s/codename/codename,/

    

    Fixed.

    

      



      

        
+-- `gcc -march=native -Q --help=target | grep march`.
+--
+-- 1. https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
+-- 2. https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512
+--
+-- Manual steps for reproducing are the following:
+--
+-- $ CC=gcc TARGET_CFLAGS='skylake-avx512' cmake -S . -B build
+-- $ cmake --build build --parallel
+-- $ echo > test.lua
+-- $ LUA_PATH="src/?.lua;;" luajit -b -o osx -a arm test.lua test.o
+-- $ file test.o
+-- empty.o: DOS executable (block device driver)
+
+local ffi = require('ffi')


      

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


    

    Moved to another place.

    

      

      

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


      

      
<snipped>



      

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


      

      
Typo: s/are/is/

    

    Fixed.

    

      



      

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


      

      
<snipped>



      

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


      

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

    

    Fixed.

    

      



      

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


      

      
Nit: The comment line looks underfilled.

    

    Refilled it.

        
    

      



      

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


      

      
<snipped>



      

        
+local SUM_CPUTYPE = {


      

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


    

    Added a comment

    

      

      

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


      

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



      

        
+  arm = 3 + 9,
+}
+


      

      
<snipped>



      

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


      

      
<snipped>



      

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


      

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

Matter of taste.
Feel free to ignore.


    

    ignored

    

      

      

        
+
+test:done(true)
-- 
2.34.1



      

      

    

  
  


--------------9H09pU1eZzzHj9f0UYZufVja--