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 4C7616EC58; Sat, 20 Feb 2021 22:18:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4C7616EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1613848721; bh=6HQ2W1zsXghBUN1GOfZM0nOE2hi3qC2Me4fH8zH5PBw=; 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=CkaiG6nPpX66/+MMJFvgjATCzL0vUiBdycnVNOxZeE4JF5ES2JQWxAsVtmgx/44NA qfaJo+RNaJkOfxOnpBLTIudGki0lOqINBGQrIbFoSeggYwcmosRV3q6gs+ECZGxhTj h5sejqDINQvuV88atthbmM9XEeSypzyba/4xOu6M= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 356A36EC58 for ; Sat, 20 Feb 2021 22:18:40 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 356A36EC58 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lDXm2-0000Q5-WA; Sat, 20 Feb 2021 22:18:39 +0300 Date: Sat, 20 Feb 2021 22:18:35 +0300 To: Sergey Kaplun Message-ID: <20210220191835.GB9042@tarantool.org> References: <7cd42be38d86c832ecd4ba0f3edd7ae83aead7ad.1612291495.git.imun@tarantool.org> <20210211192336.GC9361@root> <20210216152830.GL5448@tarantool.org> <20210218095643.GA6842@root> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210218095643.GA6842@root> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD975C3EC174F5669221340953D8FEAAFCA26BB79E5C093AA91182A05F538085040B1ACF3020BC54F7CDDF99347D823E2E1631C8D8DC909A1485B4B2153517ED7C4 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7196003627DEC9496EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006379C6BD13E0D9523298638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FCDAC524052B5D19FB35E4D9C6EEF1DBA5FCA739BFC4E66FFA389733CBF5DBD5E913377AFFFEAFD269A417C69337E82CC2CC7F00164DA146DAFE8445B8C89999729449624AB7ADAF37F6B57BC7E64490611E7FA7ABCAF51C92176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481F9449624AB7ADAF3735872C767BF85DA29E625A9149C048EE0A3850AC1BE2E73557739F23D657EF2BC8A9BA7A39EFB7666BA297DBC24807EA089D37D7C0E48F6C8AA50765F7900637571EAFDAACF43475EFF80C71ABB335746BA297DBC24807EA27F269C8F02392CD20465B3A5AADEC6827F269C8F02392CD5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A52BC311FA095C39B9D4D8C898AE8E9F2407210A72E3EA98E0D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D348B409C2D257583DFB78B5033742ED36ECF63810A85FA40141716C7F016E16A0526C00156B97DB66C1D7E09C32AA3244C74C667BCA293D372C1E7D39EC3276FFBA90944CA99CF22E3927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj2CfMKaWP9xulY1hVDiyOgA== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822935A8C75872DC3CBDD001FE46E3F6880A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergey, On 18.02.21, Sergey Kaplun wrote: > Igor, > > Thanks for the explanations! > > > > Side note: This is not related to the patch directly but what about > > > Tarantool COPYRIGHT notice in the LuaJIT code and inside new cmake files > > > in particular? > > > > We definitely need to make such activity, but I have no idea how to do > > it a right way. Let's postpone it and discuss later, if you don't mind. > > Yes, of course. May be I create the ticket? Let's discuss it in our LuaJIT group at first and then create a ticket with the resolution and plan. > > > > > > diff --git a/CMakeLists.txt b/CMakeLists.txt > > > > new file mode 100644 > > > > index 0000000..0dba5d8 > > > > --- /dev/null > > > > +++ b/CMakeLists.txt > > > > +if(CMAKE_LIBRARY_ARCHITECTURE) > > > > + AppendFlags(TARGET_C_FLAGS -DLUA_MULTILIB='"lib/${CMAKE_LIBRARY_ARCHITECTURE}"') > > > > +endif() > > > > > > What about `LUA_LMULTILIB`? > > > > > > Side note: should we provide `DESTDIR` or/and `MULTILIB` control variable > > > like it does in Makefile.original? > > > > DESTDIR is supported out of the box in CMake. MULTILIB is "autodetected" > > via CMake[1] (I hope, but never tried). Regardging LMULTILIB, I have no > > idea what its purpose is. Do you? > > To configure default CPATH, IINM, see luaconf.h for details. My fault, ask you in a different way: how do you suppose to use it? > > > > > | option(LUAJIT_USE_VALGRIND "Valgrind support" OFF) > | if(LUAJIT_USE_VALGRIND) > | AppendFlags(TARGET_C_FLAGS -DLUAJIT_USE_VALGRIND) > | endif() > > Nit: I see the compilation error (predictable) when this option is ON, > but valgrind is not installed. May be add some checks at configuration > phase? > > Feel free to ignore. The original build system also checks nothing. You can create a ticket to implement it in CMake (or rather in both build systems). Ignoring for now. > > > > > diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake > > > > new file mode 100644 > > > > index 0000000..faaef6b > > > > --- /dev/null > > > > +++ b/cmake/LuaJITUtils.cmake > > > Nit: Also, it would be nice to see comments here, why do we use this > > > approach instead use of CMAKE_HOST_SYSTEM_PROCESSOR variable (is set by > > > `uname -p`). > > > Feel free to ignore. > > > > I have no clear explanation of these and I definitely don't want to > > write a bullshit in the comments, so I will either add TODO here if you > > insist, or ignore this one. > > Add TODO, please. Just to mention this. Added TODO, squashed, force-pushed to the branch. Diff is below: ================================================================================ diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake index 8ee4c26..2400fa5 100644 --- a/cmake/LuaJITUtils.cmake +++ b/cmake/LuaJITUtils.cmake @@ -3,6 +3,8 @@ function(LuaJITTestArch outvar strflags) # spaces with no further parsing. At the same time GCC is bad in # argument handling, so let's help it a bit. separate_arguments(TESTARCH_C_FLAGS UNIX_COMMAND ${strflags}) + # TODO: It would be nice to drop a few words, why do we use this + # approach instead of CMAKE_HOST_SYSTEM_PROCESSOR variable. execute_process( COMMAND ${CMAKE_C_COMPILER} ${TESTARCH_C_FLAGS} -E lj_arch.h -dM WORKING_DIRECTORY ${LUAJIT_SOURCE_DIR} ================================================================================ > > > > > > > Content of : > > > | src/host/buildvm_arch.h: dynasm/*.lua > > > | @$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --blue --bold --progress-dir=/luajit/CMakeFiles --progress-num=$(CMAKE_PROGRESS_1) "Generating buildvm_arch.h" > > > | cd /luajit/src/host && /luajit/src/host/minilua /luajit/dynasm/dynasm.lua -D ENDIAN_LE -D P64 -D JIT -D FFI -D DUALNUM -D FPU -D HFABI -D VER=80 -o buildvm_arch.h /luajit/src/vm_arm.dasc > > > > > > vm_arm.dasc instead vm_arm64.dasc here. > > > > Oops. > > > > > > > > All because LJ_TARGET_ARM matched before LJ_TARGET_ARM64 in the line > > > below. And it works for MIPS, because of different order :) > > > Swap arches and leave comments about an order, please. > > > > Neat, many thanks for testing and investigation! Fixed, squashed, > > force-pushed to the branch. Diff is below: > > > > ================================================================================ > > > > diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake > > index 1769675..0a662f7 100644 > > --- a/cmake/LuaJITUtils.cmake > > +++ b/cmake/LuaJITUtils.cmake > > @@ -12,7 +12,7 @@ function(LuaJITTestArch outvar strflags) > > endfunction() > > > > function(LuaJITArch outvar testarch) > > - foreach(TRYARCH X64 X86 ARM ARM64 PPC MIPS64 MIPS) > > Please add a comment. Just to notice. Added the comment, squashed, force-pushed to the branch. Diff is below: ================================================================================ diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake index 2400fa5..f52ce10 100644 --- a/cmake/LuaJITUtils.cmake +++ b/cmake/LuaJITUtils.cmake @@ -21,6 +21,7 @@ function(LuaJITTestArch outvar strflags) endfunction() function(LuaJITArch outvar testarch) + # XXX: Please do not change the order of the architectures. foreach(TRYARCH X64 X86 ARM64 ARM PPC MIPS64 MIPS) string(FIND "${testarch}" "LJ_TARGET_${TRYARCH}" FOUND) # FIXME: is introduced in CMake version 3.2, but ================================================================================ > > > > > diff --git a/cmake/SetTargetFlags.cmake b/cmake/SetTargetFlags.cmake > > > > new file mode 100644 > > > > index 0000000..260fc6b > > > > --- /dev/null > > > > +++ b/cmake/SetTargetFlags.cmake > > > > I add some comments to the following chunk. > > | +if(CMAKE_SYSTEM_NAME STREQUAL "Darwin") > | + if(LUAJIT_ARCH STREQUAL "x64") > > Side note: I have a linking error with the original Makefile > without `MACOSX_DEPLOYMENT_TARGET` specification: > || LINK luajit > || Undefined symbols for architecture x86_64: > || "__Unwind_DeleteException", referenced from: > || _lj_err_unwind_dwarf in libluajit.a(lj_err.o) > || "__Unwind_GetCFA", referenced from: > || _lj_err_unwind_dwarf in libluajit.a(lj_err.o) > || "__Unwind_RaiseException", referenced from: > || _lj_err_throw in libluajit.a(lj_err.o) > || "__Unwind_SetGR", referenced from: > || _lj_err_unwind_dwarf in libluajit.a(lj_err.o) > || "__Unwind_SetIP", referenced from: > || _lj_err_unwind_dwarf in libluajit.a(lj_err.o) > || ld: symbol(s) not found for architecture x86_64 > > But nothing with CMake... > Nonetheless, I suggest to safe the old Makefile behaviour to specify > the minimum OS version (see [1]): There is no error in CMake. This behaviour is dropped in LuaJIT in scope of this patch[1]. Why do we need to add this to CMake? > > Feel free to ignore. Ignoring. > > | + AppendFlags(TARGET_BIN_FLAGS -pagezero_size 10000 -image_base 100000000) > > Here are description from Mike for this flags, please add this comment. > > || commit f76e5a311ba543ae174acd3b585fb672fde8a3b5 > || Author: Mike Pall > || Date: Thu Mar 4 16:27:42 2010 +0100 > || > || Allocate 32 bit memory on OSX/x64 with mmap() hinting. > || > || Must set -pagezero_size, otherwise the lower 4GB are blocked. Added, thanks. > > | + AppendFlags(TARGET_SHARED_FLAGS -image_base 7fff04c4a000) > > Also I've found that this line produces warnings from linker like: > > || ld: warning: -seg1addr not 16384 byte aligned, rounding up > > So, I suggest the following solution for cmake and for the original > Makefile (just add 8Kb): There is the same value in the build system of the vanilla LuaJIT[2], so I leave everything intact. BTW, I see no warning on my Mac. > > > > > > > > > Are -dynamiclib and -fPIC flags forced set by CMake? > > > Also, I don't find -install_name -compatibility_version -current_version > > > flags. Are they insignificant? > > > > Could you please share the corresponding part of "make VERBOSE=1"? These > > variables should be set by CMake. > > Thanks for the tip-off. Nothing is missing, but -compatibility_version > is "2.0.0" instead of "2.1". Is that OK? I see. There are the following parameters: * VERSION[3] -- responsible for '-current_version' on Mac * SOVERSION[4] -- responsible for '-compatibility_version' on Mac I faced no problems with the current values of these variables, but if you have any, please let me know. I believe the fix will be a small if-else block with a huge comment that CMake is a shit. > > > > Is it related to this? > > > > No, you'll see this if you run with no suppressing its output. The > > same issue is also presented for lj_gdbjit.c when GDBJIT support is > > disabled. > > Thanks, please see the patch to suppress superfluous randlib warnings about > "*.a" having no symbols on MacOSX. Mike just ignoring them by > `2>/dev/null` :) > =================================================================== > diff --git a/cmake/SetTargetFlags.cmake b/cmake/SetTargetFlags.cmake > index c8bcded..1fcdeea 100644 > --- a/cmake/SetTargetFlags.cmake > +++ b/cmake/SetTargetFlags.cmake > @@ -30,6 +30,8 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin") > AppendFlags(TARGET_SHARED_FLAGS -image_base 7fff04c4a000) > endif() > AppendFlags(TARGET_SHARED_FLAGS -single_module -undefined dynamic_lookup) > + set(CMAKE_C_ARCHIVE_CREATE " rcusS ") > + set(CMAKE_C_ARCHIVE_FINISH " -no_warning_for_no_symbols -c ") > else() # Linux and FreeBSD. > AppendFlags(TARGET_BIN_FLAGS -Wl,-E) > list(APPEND TARGET_LIBS dl) > =================================================================== > > But may be this is not the best plase for this (it is not about flags). > > Feel free to change and reword all these comments (here and above) on > your own. I don't know why you are so bothered with these warnings and want to hack command invocation. If this bothers you a lot, then simply *try to fix the problem* instead of *masking it*. The fix is quite small and easy: ================================================================================ diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 54f2941..809aac6 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -12,6 +12,11 @@ include_directories(${CMAKE_CURRENT_SOURCE_DIR}) include(MakeSourceList) +# --- Set OS and arch specific flags ------------------------------------------- + +include(SetTargetFlags) +include(SetDynASMFlags) + # --- Define source tree ------------------------------------------------------- # Core runtime. @@ -106,7 +111,6 @@ make_source_list(SOURCES_JIT_OPT lj_opt_mem.c lj_opt_narrow.c lj_opt_sink.c - lj_opt_split.c ) make_source_list(SOURCES_FFI @@ -146,6 +150,19 @@ if(NOT LUAJIT_DISABLE_JIT) if(LUAJIT_ENABLE_GDBJIT) list(APPEND SOURCES_CORE ${CMAKE_CURRENT_SOURCE_DIR}/lj_gdbjit.c) endif() + # The support provided within src/lj_opt_split.c is required + # only for soft-float targets or for 32 bit CPUs which lack + # native 64 bit integer operations (the FFI is currently the + # only emitter for 64 bit integer instructions). + # Detect the target by the set DYNASM_FLAGS. + list(FIND DYNASM_FLAGS FPU HASFPU) + list(FIND DYNASM_FLAGS P64 HASP64) + # XXX: The CMake condition below should be the same as the + # following #ifdef condition (LJ_HASJIT is already 'true'): + # #if LJ_HASJIT && (LJ_SOFTFP || (LJ_32 && LJ_HASFFI)) + if(HASFPU LESS 0 OR HASP64 LESS 0 AND NOT LUAJIT_DISABLE_FFI) + list(APPEND SOURCES_CORE ${CMAKE_CURRENT_SOURCE_DIR}/lj_opt_split.c) + endif() endif() # Build FFI sources if FFI support is enabled. @@ -163,10 +180,6 @@ make_source_list(CLI_SOURCES luajit.c ) -# --- Set OS and arch specific flags ------------------------------------------- - -include(SetTargetFlags) - # --- Prepare files generated by buildvm --------------------------------------- add_subdirectory(host) diff --git a/src/host/CMakeLists.txt b/src/host/CMakeLists.txt index c00587f..e01db87 100644 --- a/src/host/CMakeLists.txt +++ b/src/host/CMakeLists.txt @@ -6,9 +6,6 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR) # FIXME: Both minilua and buildvm need to be build with the HOST_* # toolchain. -# XXX: DynASM flags are set considering the target arch. -include(SetDynASMFlags) - # --- Build minilua ------------------------------------------------------------ # If left blank, minilua is built and used. You can supply an installed ================================================================================ Does this change worth it? I don't know. At least it *solves* the warning, not *masks* it. > > > > > diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt > > > > new file mode 100644 > > > > index 0000000..8ada1a4 > > > > --- /dev/null > > > > +++ b/src/CMakeLists.txt > > > This warnings are annoying (cmake version is 3.13.4): > > > > Did you try the latest version of the patchset? All these > > rules are moved under condition after Timur comment. > > Yes, just check it again inside Docker with arm64. Well, then I have no idea what is wrong with it. In the last version the install rule is not created if there is no corresponding target. > How can I provide more debug info? > > > > > > > Can we suppress them? I don't know if we can. Try just to close your eyes :) > > > > > > > +install(TARGETS ${LUAJIT_DEPS} > > > > > > The development releases of LuaJIT deliberately are not installed like > > > "luajit", but "luajit-2.1.0-beta3". Do we want to change this behaviour? > > > > What do you mean by that? Could you please provide an example? > > Sure. OK, now I get it. No, I don't want to mess with this shit and I have no idea why Mike wants to. Anyway, if one needs such complex installation it can use the original build system. > > > > > Shit. BTW, the one generated above in bindir is neither. You can't > > imagine how I reproach myself about approving this file addition. It's > > like a vermiform appendix that we can't cut off right now. I've made the > > template executable though this is nonsense but I failed to find another > > normal solution. Anyway, I left the comment regarding it, squashed with > > the previous commit. Here is the diff for both comments above: > > There is no use to cry over spilt milk. Without this we probably would > hear nothing from our dear customers about this feature. But customers do not use this runner, we don't even provide it in the packages. So nobody (except might be you) uses it now. > I reckon on doing this soon after integration of testing suite and > memory profiler bugfixes. Nice, thanks! > > > > > > > Side note: why don't you provide custom `tools` target for memprof like > > > in the previous patch? > > > > For what? In the previous patch there was an additional work to do, that > > is a dependency for the main targets, so I grouped it into a separate > > target. > > OK, hope this problems will vanish with a -[tm] flag. > > This is a comment for the previous patch. Just place it here to avoid of > spam messages. > > When I try `make -f Makefile.original -j`, I see the following warning: > > | make: Circular src/luajit <- tools dependency dropped. Yes, there is a primitive circular dependency, that Make can solve by itself. I see no problem in this warning and totally don't want to spoil the Makefile more. > > Also, I found that amalg build is broken. I propose the following patch: Thanks, I've fixed amalg build with your change. Diff is below: ================================================================================ diff --git a/src/Makefile.original b/src/Makefile.original index 502504c..031f077 100644 --- a/src/Makefile.original +++ b/src/Makefile.original @@ -606,14 +606,14 @@ default all: $(TARGET_T) amalg: @grep "^[+|]" ljamalg.c - $(MAKE) all "LJCORE_O=ljamalg.o" + $(MAKE) -f Makefile.original all "LJCORE_O=ljamalg.o" clean: $(HOST_RM) $(ALL_RM) libbc: ./$(LUAJIT_T) host/genlibbc.lua -o host/buildvm_libbc.h $(LJLIB_C) - $(MAKE) all + $(MAKE) -f Makefile.original all depend: @for file in $(ALL_HDRGEN); do \ ================================================================================ > > > > > > > > > > > -- > > > > 2.25.0 > > > > > > > > > > [1]: https://stackoverflow.com/questions/41773161/negate-boolean-variable-in-cmake > > > [2]: https://cmake.org/pipermail/cmake/2015-July/061116.html > > > [3]: https://cmake.org/cmake/help/latest/command/continue.html > > > [4]: https://gitlab.kitware.com/cmake/community/-/wikis/FAQ#can-i-do-make-uninstall-with-cmake > > > > > > -- > > > Best regards, > > > Sergey Kaplun > > > > [1]: https://cmake.org/cmake/help/v3.1/variable/CMAKE_LIBRARY_ARCHITECTURE.html > > > > -- > > Best regards, > > IM > > [1]: https://cmake.org/cmake/help/v3.14/envvar/MACOSX_DEPLOYMENT_TARGET.html > > -- > Best regards, > Sergey Kaplun [1]: https://github.com/LuaJIT/LuaJIT/commit/8961a92 [2]: https://github.com/LuaJIT/LuaJIT/blob/v2.1/src/Makefile#L327 [3]: https://cmake.org/cmake/help/latest/prop_tgt/VERSION.html#mach-o-versions [4]: https://cmake.org/cmake/help/latest/prop_tgt/SOVERSION.html#mach-o-versions -- Best regards, IM