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 C5E3FA189AF; Tue, 9 Apr 2024 15:51:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C5E3FA189AF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1712667095; bh=vpmpw3rBrRHMdnHzRhnIyS1XfdlgTkLK0JgtZjCJeUE=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=ORq71njX+nvRL9T30epBglhqJaQlD3ENW/nsypbCwd/GWe4UUE98p5q/MeEudFGGY D5sXhMobLILNWi1yEY81s3urgQM3abjzSjWFBH3uX/VwEFumCgHvBIFR2gdjZpYg6e w+KvpL/Qat4BPOvaPxpF5mwEzTCHqWY4PB8maShw= Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [95.163.41.86]) (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 6FBCBA189AF for ; Tue, 9 Apr 2024 15:51:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6FBCBA189AF Received: by smtp48.i.mail.ru with esmtpa (envelope-from ) id 1ruAwm-0000000DLlf-31v6; Tue, 09 Apr 2024 15:51:33 +0300 Date: Tue, 9 Apr 2024 15:47:31 +0300 To: Sergey Bronnikov Message-ID: References: <9a71bf0765acd6ab019de4ae9f491a6c7bcb463d.1710416150.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D327C87852EB66D3E9BB869645F453850E58E3D3BAE501A2182A05F53808504023255EB4AC5D78B6AC8EDD30083ED68E701828D12DFD9F88E70CC22FBD1917D222ADB976EB683C0A X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE76DA79C5AFF329FDBEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063721F819F5952827038638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8B8D19C516ECDFEB25D9F6DFC8493A83F065E6030A2DE1F2ECC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637F924B32C592EA89F389733CBF5DBD5E9B5C8C57E37DE458B9E9CE733340B9D5F3BBE47FD9DD3FB595F5C1EE8F4F765FC8C7ADC89C2F0B2A5E2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B851EDB9C5A93305EEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-87b9d050: 1 X-C1DE0DAB: 0D63561A33F958A5058E7EB6B236EC825002B1117B3ED696F43D5942BAD06BCC7E0012C66AE17B00823CB91A9FED034534781492E4B8EEAD5E90D3DD2A5B7EAFBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFEFEFA132F2909C0C470A9F226D81AF1858F13A576A5EEC873C860530C9787724DDCE95BB4BF576A643A9A4803F78FBB9E881FC80A40180E9BB0A515CCCAB4C1D8196EB6A44593D39C226CC413062362A913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojRxVvDy3pXepJaNKglGQl/g== X-DA7885C5: 3661674E8B84B30BF255D290C0D534F97BAE5AB361C435804CEC4C85D3C1E41253DF3D213E0294825B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE3380B31FBFD66A5DE86F0B82A6FA44A40BFC4378FCC8914146E49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/2] 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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) > >> > > 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. > > --- a/.github/workflows/exotic-builds-testing.yml > +++ b/.github/workflows/exotic-builds-testing.yml > >> 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 > >> 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 > >> +-- 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". > > >> +-- > >> +-- 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? > >> +-- > >> +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? (*) > >> +]] > >> + > >> + 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 > > >> + 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 (**). > > > > > >> +-- Numbers below are defined in . > > Typo: s/Numbers/The numbers/ > > There is no space for "" on the line > > after adding "The"as you requested. And line become underfileld if > > "" 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. | -- | -- has the definitions of the | -- numbers below. The original XNU source code may be found in | -- [1]. | -- | -- 1. https://opensource.apple.com/source/xnu/xnu-4570.41.2/osfmk/mach/machine.h.auto.html > > > > >> +-- Original source is XNU source code, [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 -- [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 . >   -- The original source is the XNU source code, >   -- [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. > >> +-- > >> +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. > >> 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