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 6D60170358; Tue, 16 Feb 2021 18:28:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6D60170358 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1613489321; bh=tW3nzqXBW4KoOFOk1P4beidsq1t3mQc8veA5aOeGz1o=; 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=x0kXNArrG84q2E+dEQpcdEaewB04Jdh2bKNwGsgsFoc5UzbIiFfB2jeYNSVO5aRHY kUeq0hsh8i53E+qhbtnn+jlqIaQdL65O46RZCRn0xtPXYwjlHF+J+GLUQnH/Io+ig/ O/f73LihenoO5VFmJCaBPKWhDzEAxwLT2YQ/bguM= Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 0EEB670358 for ; Tue, 16 Feb 2021 18:28:40 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0EEB670358 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lC2HB-00018N-GZ; Tue, 16 Feb 2021 18:28:34 +0300 Date: Tue, 16 Feb 2021 18:28:30 +0300 To: Sergey Kaplun Message-ID: <20210216152830.GL5448@tarantool.org> References: <7cd42be38d86c832ecd4ba0f3edd7ae83aead7ad.1612291495.git.imun@tarantool.org> <20210211192336.GC9361@root> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20210211192336.GC9361@root> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD91883A1EE8D2E9932B762446F14B265E4328C398AC2855206182A05F538085040CBBB2C01BAB29578F55D3FCC385862D39C5A5A6A47C85895E1D165DE0DE995E8 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE76DD23B0452F84E3CEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375F0BD5CF353A411D8638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FCE1456C810600668BC3CF459501C2C66AB463CD60234F07AF389733CBF5DBD5E913377AFFFEAFD269A417C69337E82CC2CC7F00164DA146DAFE8445B8C89999729449624AB7ADAF37F6B57BC7E64490611E7FA7ABCAF51C92A417C69337E82CC2CC7F00164DA146DA6F5DAA56C3B73B23C77107234E2CFBA567F23339F89546C55F5C1EE8F4F765FCB835E6E385EA5AF075ECD9A6C639B01BBD4B6F7A4D31EC0BC0CAF46E325F83A522CA9DD8327EE4930A3850AC1BE2E7351D6A3D1828C120DEC4224003CC836476C0CAF46E325F83A50BF2EBBBDD9D6B0F05F538519369F3743B503F486389A921A5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5438A5D78CEA3C80B54ABC741094EFAF205C475FB60BB073FD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D345DB600F8E858000FB1F44C47F158F97E820BC3C8E7B96B9D9C9ECE65913ABBB6A0744D33D8E8DE151D7E09C32AA3244C249DC008E221C2327017E9D9C078A5EA63871F383B54D9B3927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojiBTwj6noE5fDtORJxPD/kw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822BC18E482CF73B6CB3233D7103329BA2DA7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B 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, Thanks for your review! On 11.02.21, Sergey Kaplun wrote: > Hi, Igor! > > Thanks for the patch! > > Appreciate your hard work! Thanks for the verbose comments! > The building process is faster now! > I'm sorry: I can't see your GUI innovations, unfortunately. :( > > I've looked into branch version. > Please consider my comments below. I've fixed all typos you've mentioned, so I left only those I haven't or have changed in a different way. > > On 02.02.21, Igor Munkin wrote: > > In scope of this patch the LuaJIT build system is partially ported from > > Typo: s/In scope/In the scope/ I strongly doubt. Neither you, nor me: Within. > > > old build system (see the recipe in the previous commit). > > This paragraph describes only supported OS. There is no description of > supported build system options. > IINM there is no cross-compiling support, stripping debug info, amalgam > build for now, it should be mentioned in the commit message too. Here is the new wording for this section: | Within this patch the LuaJIT build system is partially ported from GNU | Make to CMake. These changes provide CMake build system for all | supported host architectures but only for the following OS: GNU/Linux, | OSX, FreeBSD. For other platrforms and specific builds (such as 'amalg', | stripped binary and shared library, cross-compiling support) use the old | build system (see the recipe in the previous commit). > > > > > Several components of the new build system such as automatic version > > detection, source files list generation and some recipes for > > CMakeLists.txt are taken verbatim or adapted from LuaVela repository. > > > > Part of tarantool/tarantool#4862 > > > > Signed-off-by: Igor Munkin > > --- > > 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. > > > .gitignore | 12 +- > > CMakeLists.txt | 261 +++++++++++++++++++++++++ > > cmake/LuaJITUtils.cmake | 31 +++ > > cmake/MakeSourceList.cmake | 47 +++++ > > cmake/SetDynASMFlags.cmake | 130 ++++++++++++ > > cmake/SetTargetFlags.cmake | 42 ++++ > > cmake/SetVersion.cmake | 45 +++++ > > etc/CMakeLists.txt | 32 +++ > > src/CMakeLists.txt | 391 +++++++++++++++++++++++++++++++++++++ > > src/host/CMakeLists.txt | 61 ++++++ > > tools/CMakeLists.txt | 77 ++++++++ > > 11 files changed, 1128 insertions(+), 1 deletion(-) > > create mode 100644 CMakeLists.txt > > create mode 100644 cmake/LuaJITUtils.cmake > > create mode 100644 cmake/MakeSourceList.cmake > > create mode 100644 cmake/SetDynASMFlags.cmake > > create mode 100644 cmake/SetTargetFlags.cmake > > create mode 100644 cmake/SetVersion.cmake > > create mode 100644 etc/CMakeLists.txt > > create mode 100644 src/CMakeLists.txt > > create mode 100644 src/host/CMakeLists.txt > > create mode 100644 tools/CMakeLists.txt > > Side note: should we update (change Makefile -> > Makefile.original)? Looks more related to the previous patch. OK, fixed, squashed with the previous commit. Here is the diff: ================================================================================ diff --git a/doc/install.html b/doc/install.html index c491c60..b2e4753 100644 --- a/doc/install.html +++ b/doc/install.html @@ -164,9 +164,9 @@ hold all user-configurable settings:

  • src/luaconf.h sets some configuration variables.
    • -
  • Makefile has settings for installing LuaJIT (POSIX +
  • Makefile.original has settings for installing LuaJIT (POSIX only).
    • -
  • src/Makefile has settings for compiling LuaJIT +
  • src/Makefile.original has settings for compiling LuaJIT under POSIX, MinGW or Cygwin.
  • src/msvcbuild.bat has settings for compiling LuaJIT with MSVC or WinSDK.
    • @@ -228,7 +228,7 @@ variable is not set, then it's forced to 10.4.

    Installing LuaJIT

    -The top-level Makefile installs LuaJIT by default under +The top-level Makefile.original installs LuaJIT by default under /usr/local, i.e. the executable ends up in /usr/local/bin and so on. You need root privileges to write to this path. So, assuming sudo is installed on your system, @@ -366,7 +366,7 @@ target OS differ, or you'll get assembler or linker errors:

    • E.g. if you're compiling on a Windows or OSX host for embedded Linux or Android, you need to add TARGET_SYS=Linux to the examples below.
      • -
    • For a minimal target OS, you may need to disable the built-in allocator in src/Makefile and use TARGET_SYS=Other.
      • +
    • For a minimal target OS, you may need to disable the built-in allocator in src/Makefile.original and use TARGET_SYS=Other.
    • Don't forget to specify the same TARGET_SYS for the install step, too.

    @@ -647,7 +647,7 @@ the paths needed to locate the shared library. to a shadow tree instead of the root tree of the build system.

  • MULTILIB sets the architecture-specific library path component for multilib systems. The default is lib.
    • -
  • Have a look at the top-level Makefile and src/Makefile +
  • Have a look at the top-level Makefile.original and src/Makefile.original for additional variables to tweak. The following variables may be overridden, but it's not recommended, except for special needs like cross-builds: ================================================================================ NB: I haven't break the lines intentionally to make the diff minimal. > > > diff --git a/CMakeLists.txt b/CMakeLists.txt > > new file mode 100644 > > index 0000000..0dba5d8 > > --- /dev/null > > +++ b/CMakeLists.txt > > @@ -0,0 +1,261 @@ > > +# LuaJIT -- interpreter and JIT compiler for Lua language. > > +# This is the main entry point for building, testing and > > +# packaging the project. > > +# Major portions taken verbatim or adapted from the uJIT. > > +# Copyright (C) 2015-2019 IPONWEB Ltd. > > + > > +# --- Initial setup ------------------------------------------------------------ > > + > > +cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR) > > +project(LuaJIT C) > > + > > +# > > Nit: Comment style is inconsistent. Feel free to ignore. Fixed, squashed, force-pushed to the branch. Diff is below: ================================================================================ diff --git a/CMakeLists.txt b/CMakeLists.txt index af9f8e4..e5be172 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -11,12 +11,10 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR) project(LuaJIT C) -# # XXX: Originally CMake machinery is introduced to make LuaJIT # testing self-sufficient. Since there are only few systems # covered with the tests in our CI, there is no need to support # others for now and the original build system can be used. -# if(NOT( CMAKE_SYSTEM_NAME STREQUAL "Linux" OR ================================================================================ > > > +# XXX: Originally CMake machinery is introduced to make LuaJIT > > +# testing self-sufficient. Since there are only few systems > > Typo: s/only few systems/only a few systems/ No, I want to emphasize the amount, so the article changes the meaning. > > > + > > +set(BUILDMODE_VALUES mixed static dynamic) > > See nothing bad to transfer comments described each option here. Added, squashed, force-pushed to the branch. Diff is below: ================================================================================ diff --git a/CMakeLists.txt b/CMakeLists.txt index e5be172..786dbe0 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -59,7 +59,14 @@ set(LUAJIT_INCLUDEDIR include/luajit-${LUAJIT_VERSION_MAJOR}.${LUAJIT_VERSION_MINOR} ) +# Mixed mode creates a static + dynamic library and a statically +# linked luajit. Static mode creates a static library and a +# statically linked luajit. Dynamic mode creates a dynamic library +# and a dynamically linked luajit. +# XXX: dynamically linked executable will only run when the +# library is installed! set(BUILDMODE_VALUES mixed static dynamic) +# The default build mode is mixed mode on POSIX. list(GET BUILDMODE_VALUES 0 BUILDMODE_DEFAULT) set(BUILDMODE ${BUILDMODE_DEFAULT} CACHE STRING "Build mode. Choose one of the following: ${BUILDMODE_VALUES}." ================================================================================ > > > +list(GET BUILDMODE_VALUES 0 BUILDMODE_DEFAULT) > > +# --- Compilation flags setup -------------------------------------------------- > > + > > +if(NOT CMAKE_INSTALL_PREFIX STREQUAL "/usr/local") > > + AppendFlags(TARGET_C_FLAGS -DLUA_ROOT='"${CMAKE_INSTALL_PREFIX}"') > > In Makefile.original I see the following check inside this branch: > > | ifneq (/usr,$(PREFIX)) > | TARGET_DYNXLDOPTS= -Wl,-rpath,$(TARGET_LIBPATH) > | endif > > Is it related to unsupported build systems? AFAICS, it is. Anyway, if you're having troubles, feel free to share. > > I've found some warnings on macOS in the configuration stage, that looks > related: > > | CMake Warning (dev): > | Policy CMP0042 is not set: MACOSX_RPATH is enabled by default. Run "cmake > | --help-policy CMP0042" for policy details. Use the cmake_policy command to > | set the policy and suppress this warning. > | > | MACOSX_RPATH is not specified for the following targets: > | > | libluajit_shared > | > | This warning is for project developers. Use -Wno-dev to suppress it. > > Version of macOS: > | $ uname -a > | Darwin MacBook-Pro.local 19.6.0 Darwin Kernel Version 19.6.0: Mon Aug 31 22:12:52 PDT 2020; root:xnu-6153.141.2~1/RELEASE_X86_64 x86_64 > I've seen this warning, and have tried almost all combinations of the MACOS_RPATH, CMP0042, and target options -- nothing has helped. BTW, this warning breaks nothing for me (on my old Mac). If this bothers you, please report your issues. If you have a solution, please share it. > > +endif() > > + > > +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? > > > + > > +AppendFlags(CMAKE_C_FLAGS -Wall) > > +option(LUAJIT_ENABLE_WARNINGS "Build LuaJIT with warnings enabled" OFF) > > Nit: This confused me a little. All warnings already enabled. This flag > enables extra warnings. I may be change like the following: > s/LUAJIT_ENABLE_WARNINGS/LUAJIT_ENABLE_EXTRA_WARNINGS/ > s/warnings/extra warnings/ > or > s/LUAJIT_ENABLE_WARNINGS/LUAJIT_ENABLE_MORE_WARNINGS/ > s/warnings/more warnings/ > Feel free to ignore. Honestly, I don't want a huge option name. Let's return to the subj in scope of the options revision that we discussed with Timur nearby. Ignoring for now. > > > +# Permanently disable the FFI extension to reduce the size of the > > +# LuaJIT executable. But please consider that the FFI library is > > +# compiled-in, but NOT loaded by default. It only allocates any > > +# memory, if you actually make use of it. > > +option(LUAJIT_DISABLE_FFI "FFI support" OFF) > > +if(LUAJIT_DISABLE_FFI) > > + AppendFlags(TARGET_C_FLAGS -DLUAJIT_DISABLE_FFI) > > +endif() > > +set(LUAJIT_HAS_FFI NOT LUAJIT_DISABLE_FFI) > > Don't get your point here. `set()` does not support any expressions > as I've known from [1][2]. This just always defines `LUAJIT_HAS_FFI`. OMG, CMake is the greatest shit in the world. I just dropped these auxiliary variables, squashed with the previous commit. Diff is below: ================================================================================ diff --git a/CMakeLists.txt b/CMakeLists.txt index a1536ed..13208d1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -130,7 +130,6 @@ option(LUAJIT_DISABLE_FFI "FFI support" OFF) if(LUAJIT_DISABLE_FFI) AppendFlags(TARGET_C_FLAGS -DLUAJIT_DISABLE_FFI) endif() -set(LUAJIT_HAS_FFI NOT LUAJIT_DISABLE_FFI) # Features from Lua 5.2 that are unlikely to break existing code # are enabled by default. Some other features that *might* break @@ -149,7 +148,6 @@ option(LUAJIT_DISABLE_JIT "JIT support" OFF) if(LUAJIT_DISABLE_JIT) AppendFlags(TARGET_C_FLAGS -DLUAJIT_DISABLE_JIT) endif() -set(LUAJIT_HAS_JIT NOT LUAJIT_DISABLE_JIT) # Some architectures (e.g. PPC) can use either single-number (1) # or dual-number (2) mode. Please see LJ_ARCH_NUMMODE in lj_arch.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index df9b85d..e70b786 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -140,17 +140,20 @@ make_source_list(SOURCES_CORE_NO_JIT_FFI set(SOURCES_CORE ${SOURCES_CORE_NO_JIT_FFI}) -if(LUAJIT_HAS_JIT) +# Build JIT sources if JIT support is enabled. +if(NOT LUAJIT_DISABLE_JIT) list(APPEND SOURCES_CORE ${SOURCES_JIT}) if(LUAJIT_ENABLE_GDBJIT) list(APPEND SOURCES_CORE ${CMAKE_CURRENT_SOURCE_DIR}/lj_gdbjit.c) endif() endif() -if(LUAJIT_HAS_FFI) +# Build FFI sources if FFI support is enabled. +if(NOT LUAJIT_DISABLE_FFI) list(APPEND SOURCES_CORE ${SOURCES_FFI}) - if(NOT LUAJIT_HAS_JIT) - # Needed for lj_mcode_sync. + # Build lj_mcode.c if JIT support is disabled since + # is used in src/lj_ccallback.c. + if(LUAJIT_DISABLE_JIT) list(APPEND SOURCES_CORE ${CMAKE_CURRENT_SOURCE_DIR}/lj_mcode.c) endif() endif() ================================================================================ > > > > + > > +# Some architectures (e.g. PPC) can use either single-number (1) > > +# or dual-number (2) mode. Uncomment one of these lines to > > Nit: See nothing to uncomment :). May be change it? *Кхъ*. Removed :) ================================================================================ diff --git a/CMakeLists.txt b/CMakeLists.txt index ca03fda..cea0a53 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -152,9 +152,8 @@ endif() set(LUAJIT_HAS_JIT NOT LUAJIT_DISABLE_JIT) # Some architectures (e.g. PPC) can use either single-number (1) -# or dual-number (2) mode. Uncomment one of these lines to -# override the default mode. Please see LJ_ARCH_NUMMODE in -# lj_arch.h for details. +# or dual-number (2) mode. Please see LJ_ARCH_NUMMODE in lj_arch.h +# for details. set(LUAJIT_NUMMODE_VALUES 1 2) set(LUAJIT_NUMMODE_DEFAULT "") set(LUAJIT_NUMMODE ${LUAJIT_NUMMODE_DEFAULT} CACHE STRING ================================================================================ > > > Nit: It would be nice to see "Debugging support" section here like for > other sections like "Main source tree" or so on. > Feel free to ignore. These are in "Compilation flags setup" section. Ignoring. > > > > +# Turn on assertions for the Lua/C API to debug problems with > > +# lua_* calls. This is rather slow -- use only while developing C > > +# libraries/embeddings. > > +option(LUAJIT_USE_APICHECK "Assertions for the Lua/C API" OFF) > > +if(LUAJIT_USE_APICHECK) > > + AppendFlags(TARGET_C_FLAGS -DLUA_USE_APICHECK) > > +endif() > > Hmm, as for me looks more user friendly not to change option names here. > Just leave `LUA_USE_APICHECK` here. > I've already singed wings on it, when run Lua/LuaJIT/lua-Harness > tests! Yeees, I'd not looked at Cmake warnings carefully :( Renamed these flags, but the new naming is more consistent, so we'll revert it back in scope of build options refactoring. Diff is below: ================================================================================ diff --git a/CMakeLists.txt b/CMakeLists.txt index 25d86c0..673cf36 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -230,16 +230,16 @@ endif() # Turn on assertions for the Lua/C API to debug problems with # lua_* calls. This is rather slow -- use only while developing C # libraries/embeddings. -option(LUAJIT_USE_APICHECK "Assertions for the Lua/C API" OFF) -if(LUAJIT_USE_APICHECK) +option(LUA_USE_APICHECK "Assertions for the Lua/C API" OFF) +if(LUA_USE_APICHECK) AppendFlags(TARGET_C_FLAGS -DLUA_USE_APICHECK) endif() # Turn on assertions for the whole LuaJIT VM. This significantly # slows down everything. Use only if you suspect a problem with # LuaJIT itself. -option(LUAJIT_USE_ASSERT "Assertions for the whole LuaJIT VM" OFF) -if(LUAJIT_USE_ASSERT) +option(LUA_USE_ASSERT "Assertions for the whole LuaJIT VM" OFF) +if(LUA_USE_ASSERT) AppendFlags(TARGET_C_FLAGS -DLUA_USE_ASSERT) endif() ================================================================================ > > > > + > > +# TODO: Implement a configuration option to enable ASAN. > > +# There are two entries of LUAJIT_USE_ASAN define: > > +# $ grep -rnF 'LUAJIT_USE_ASAN' . > > +# ./src/lj_str.c:15:#if LUAJIT_USE_ASAN > > +# ./src/host/buildvm.c:36:#if LUAJIT_USE_ASAN > > +# At the same time this flag is not provided by LuaJIT original > > +# build system (i.e. src/Makefile.original) so there are no > > +# related compiler and linker flags passed. This should be done > > +# the right way later. > > Good catch! I decide to check what else is missing. Here are some other > options (sorry, for that dump, just want to save it anywhere): > | $ grep -rnF 'LUAJIT_CTYPE_CHECK_ANCHOR' > | src/lj_ctype.c:139:#ifdef LUAJIT_CTYPE_CHECK_ANCHOR > | src/lj_ctype.c:159:#ifdef LUAJIT_CTYPE_CHECK_ANCHOR > > | $ grep -rnF 'LUAJIT_DEBUG_RA' > | src/lj_asm.c:192:/* #define LUAJIT_DEBUG_RA */ > | src/lj_asm.c:194:#ifdef LUAJIT_DEBUG_RA I tried only this one, while debugging the RENAME issue. It would be nice to export it. > > | $ grep -rnF 'LUAJIT_DISABLE_DEBUGINFO' > | src/lj_parse.c:1397:#ifndef LUAJIT_DISABLE_DEBUGINFO > | src/lj_parse.c:2700:#ifdef LUAJIT_DISABLE_DEBUGINFO > | Binary file src/.lj_parse.c.swp matches > | src/lj_memprof.c:102: ** -DLUAJIT_DISABLE_DEBUGINFO flag. > > | $ grep -rnF 'LUAJIT_DISABLE_PROFILE' > | src/lj_arch.h:501:#if defined(LUAJIT_DISABLE_PROFILE) > > | $ grep -rnF 'LUAJIT_DISABLE_VMEVENT' > | src/lj_trace.c:796:#ifndef LUAJIT_DISABLE_VMEVENT > | src/lj_vmevent.h:33:#ifdef LUAJIT_DISABLE_VMEVENT > | src/lib_jit.c:118:#ifdef LUAJIT_DISABLE_VMEVENT > > | $ grep -rnF 'LUAJIT_ENABLE_CHECKHOOK' > | src/lj_record.c:2621:#ifdef LUAJIT_ENABLE_CHECKHOOK > > | $ grep -rnF 'LUAJIT_ENABLE_TABLE_BUMP' > | src/lj_record.c:259:#ifdef LUAJIT_ENABLE_TABLE_BUMP > | src/lj_record.c:1179:#ifdef LUAJIT_ENABLE_TABLE_BUMP > | src/lj_record.c:1477:#ifdef LUAJIT_ENABLE_TABLE_BUMP > | src/lj_record.c:1523:#ifdef LUAJIT_ENABLE_TABLE_BUMP > | src/lj_record.c:1874:#ifdef LUAJIT_ENABLE_TABLE_BUMP > | src/lj_record.c:2321:#ifdef LUAJIT_ENABLE_TABLE_BUMP > | src/lj_record.c:2544:#ifdef LUAJIT_ENABLE_TABLE_BUMP > | src/lj_jit.h:455:#ifdef LUAJIT_ENABLE_TABLE_BUMP > > | $ grep -rnF 'LUAJIT_NO_UNALIGNED' > | src/lj_def.h:287:#if defined(_M_PPC) && defined(LUAJIT_NO_UNALIGNED) > > | $ grep -rnF 'LUAJIT_USE_PERFTOOLS' > | src/lj_trace.c:85:#ifdef LUAJIT_USE_PERFTOOLS > | src/lj_trace.c:163:#ifdef LUAJIT_USE_PERFTOOLS > > | $ grep -rnF 'LUAJIT_UNWIND_EXTERNAL' > | doc/extensions.html:402:ARM -DLUAJIT_UNWIND_EXTERNAL > | src/lj_err.c:53:** unwinding with -DLUAJIT_UNWIND_EXTERNAL. *All* C code must be compiled > | src/lj_err.c:64:#if defined(__GNUC__) && (LJ_TARGET_X64 || defined(LUAJIT_UNWIND_EXTERNAL)) && !LJ_NO_UNWIND > | src/lj_err.c:521: ** enabled LUAJIT_UNWIND_EXTERNAL and forgot to recompile *every* > > They not all related to debugging (AFAICT only `LUAJIT_USE_PERFTOOLS`, > `LUAJIT_CTYPE_CHECK_ANCHOR` and `LUAJIT_DEBUG_RA`), more to configure. > May be it is better to create separate issue(s) for all these options? Feel free to do it :) > > > > diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake > > new file mode 100644 > > index 0000000..faaef6b > > --- /dev/null > > +++ b/cmake/LuaJITUtils.cmake > > @@ -0,0 +1,31 @@ > > +function(LuaJITTestArch outvar strflags) > > + # XXX: simply splits the COMMAND argument by > > + # spaces with no further parsing. At the same time GCC is bad in > > + # argument handling, so let's help it a bit. > > + separate_arguments(TEST_C_FLAGS UNIX_COMMAND ${strflags}) > > Nit: TESTARCH_C_FLAGS looks more verbose to me. Just TEST seems to be > referring to testing. > Feel free to ignore. Renamed, squashed with the previous commit. Here is the diff: ================================================================================ diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake index faaef6b..1769675 100644 --- a/cmake/LuaJITUtils.cmake +++ b/cmake/LuaJITUtils.cmake @@ -2,9 +2,9 @@ function(LuaJITTestArch outvar strflags) # XXX: simply splits the COMMAND argument by # spaces with no further parsing. At the same time GCC is bad in # argument handling, so let's help it a bit. - separate_arguments(TEST_C_FLAGS UNIX_COMMAND ${strflags}) + separate_arguments(TESTARCH_C_FLAGS UNIX_COMMAND ${strflags}) execute_process( - COMMAND ${CMAKE_C_COMPILER} ${TEST_C_FLAGS} -E lj_arch.h -dM + COMMAND ${CMAKE_C_COMPILER} ${TESTARCH_C_FLAGS} -E lj_arch.h -dM WORKING_DIRECTORY ${LUAJIT_SOURCE_DIR} OUTPUT_VARIABLE TESTARCH ) ================================================================================ > > 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. > > + execute_process( > > + COMMAND ${CMAKE_C_COMPILER} ${TEST_C_FLAGS} -E lj_arch.h -dM > > May be it will better to provide not default compiler here? > It will be easier to adjust cross compiling with this. I don't get this, but as we discussed before cross compiling activity is out of the scope of this issue. > > > + WORKING_DIRECTORY ${LUAJIT_SOURCE_DIR} > > + OUTPUT_VARIABLE TESTARCH > > + ) > > + set(${outvar} ${TESTARCH} PARENT_SCOPE) > > +endfunction() > > + > > +function(LuaJITArch outvar testarch) > > + foreach(TRYARCH X64 X86 ARM ARM64 PPC MIPS64 MIPS) > > I occur the following error, when build inside Docker emulating > aarch64 (`FROM arm64v8/centos`): > > | Error: pointer size mismatch in cross-build. > | Try: make HOST_CC="gcc -m32" CROSS=... > | > | make[2]: *** [src/CMakeFiles/vm_static.dir/build.make:62: src/lj_vm.S] Error 1 > | make[1]: *** [CMakeFiles/Makefile2:193: src/CMakeFiles/vm_static.dir/all] Error 2 > | make[1]: *** Waiting for unfinished jobs.... > | Error: pointer size mismatch in cross-build. > | Try: make HOST_CC="gcc -m32" CROSS=... > | > | make[2]: *** [src/CMakeFiles/vm_shared.dir/build.make:62: src/lj_vm.S] Error 1 > | make[1]: *** [CMakeFiles/Makefile2:394: src/CMakeFiles/vm_shared.dir/all] Error 2 > > All OK with the old Makefile: > > | $ make -f Makefile.original -j > | ==== Building LuaJIT 2.1.0-beta3 ==== > | ... > | ==== Successfully built LuaJIT 2.1.0-beta3 ==== > > 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) + foreach(TRYARCH X64 X86 ARM64 ARM PPC MIPS64 MIPS) string(FIND "${testarch}" "LJ_TARGET_${TRYARCH}" FOUND) if(FOUND EQUAL -1) continue() ================================================================================ > > Nit: As far as we can't organize this CPU arches in alphabetical order > I propose reverse-alphabetical ;): God, please, no... > > | X86 X64 PPC MIPS64 MIPS ARM64 ARM > > Feel free to ignore (it's more a joke). Not funny, dude... > > > + string(FIND "${testarch}" "LJ_TARGET_${TRYARCH}" FOUND) > > + if(FOUND EQUAL -1) > > + continue() > > `continue()` command was introduced in 3.2 CMake version > according to [1]. So even 3.1 version is not enough. Shit, that can't be true... But thanks for checking it! > > This is a result for verision 3.0.2. > | CMake Error at cmake/LuaJITUtils.cmake:18 (continue): > | Unknown CMake command "continue". > | Call Stack (most recent call first): > | cmake/SetTargetFlags.cmake:16 (LuaJITArch) > | src/CMakeLists.txt:164 (include) > | > | > | -- Configuring incomplete, errors occurred! > > Please, add `cmake_minimum_required()` command to the top, or do not use Hell no, I'll just rewrite this part and leave *non-toxic* comment about it. When CMake will be updated in Tarantool next time, we'll refactor this part. But not today. > `continue()` command. Reimplemented, squashed, force-pushed to the branch. Diff is below: ================================================================================ diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake index 0a662f7..2c4a27e 100644 --- a/cmake/LuaJITUtils.cmake +++ b/cmake/LuaJITUtils.cmake @@ -14,12 +14,18 @@ endfunction() function(LuaJITArch outvar testarch) foreach(TRYARCH X64 X86 ARM64 ARM PPC MIPS64 MIPS) string(FIND "${testarch}" "LJ_TARGET_${TRYARCH}" FOUND) - if(FOUND EQUAL -1) - continue() + # FIXME: is introduced in CMake version 3.2, but + # the minimum required version now is 3.1. This is not such a + # vital feature, so it's not used now. However, when CMake + # version is bumped next time, it's better to rewrite this + # part using for "early return". + # For more info see CMake Release notes for 3.2 version. + # https://cmake.org/cmake/help/latest/release/3.2.html#commands + if(NOT FOUND EQUAL -1) + string(TOLOWER ${TRYARCH} LUAJIT_ARCH) + set(${outvar} ${LUAJIT_ARCH} PARENT_SCOPE) + return() endif() - string(TOLOWER ${TRYARCH} LUAJIT_ARCH) - set(${outvar} ${LUAJIT_ARCH} PARENT_SCOPE) - return() endforeach() message(FATAL_ERROR "[LuaJITArch] Unsupported target architecture") endfunction() ================================================================================ > > > + endif() > > + string(TOLOWER ${TRYARCH} LUAJIT_ARCH) > > + set(${outvar} ${LUAJIT_ARCH} PARENT_SCOPE) > > + return() > > + endforeach() > > + message(FATAL_ERROR "[LuaJITArch] Unsupported target architecture") > > It would be nice to check return status from preprocessing > command. On ppc64 inside Docker (`FROM ppc64le/debian`) configuration > ends successfully, arch detected as "ppc64": > > | lj_arch.h:436:2: error: #error "No support for PowerPC 64 bit mode (yet)" > | #error "No support for PowerPC 64 bit mode (yet)" > | ^~~~~ > | lj_arch.h:436:2: error: #error "No support for PowerPC 64 bit mode (yet)" > | #error "No support for PowerPC 64 bit mode (yet)" > | ^~~~~ > | -- Configuring done > | WARNING: Target "luajit_static" has EXCLUDE_FROM_ALL set and will not be built by default but an install rule has been provided for it. CMake does not define behavior for this case. > | WARNING: Target "libluajit_static" has EXCLUDE_FROM_ALL set and will not be built by default but an install rule has been provided for it. CMake does not define behavior for this case. > | WARNING: Target "libluajit_shared" has EXCLUDE_FROM_ALL set and will not be built by default but an install rule has been provided for it. CMake does not define behavior for this case. > | WARNING: Target "libluajit_shared" has EXCLUDE_FROM_ALL set and will not be built by default but an install rule has been provided for it. CMake does not define behavior for this case. > | -- Generating done > | -- Build files have been written to: /luajit Nice! Added, squashed, force-pushed to the branch. Diff is below: ================================================================================ diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake index 2c4a27e..be0a8b7 100644 --- a/cmake/LuaJITUtils.cmake +++ b/cmake/LuaJITUtils.cmake @@ -7,8 +7,15 @@ function(LuaJITTestArch outvar strflags) COMMAND ${CMAKE_C_COMPILER} ${TESTARCH_C_FLAGS} -E lj_arch.h -dM WORKING_DIRECTORY ${LUAJIT_SOURCE_DIR} OUTPUT_VARIABLE TESTARCH + RESULT_VARIABLE TESTARCH_RC ) - set(${outvar} ${TESTARCH} PARENT_SCOPE) + if(TESTARCH_RC EQUAL 0) + set(${outvar} ${TESTARCH} PARENT_SCOPE) + else() + # XXX: Yield a special value (an empty string) to respect the + # failed preprocessor routine and then fail arch detection. + set(${outvar} "" PARENT_SCOPE) + endif() endfunction() function(LuaJITArch outvar testarch) ================================================================================ > > > +endfunction() > > > > diff --git a/cmake/SetDynASMFlags.cmake b/cmake/SetDynASMFlags.cmake > > new file mode 100644 > > index 0000000..9a920c3 > > --- /dev/null > > +++ b/cmake/SetDynASMFlags.cmake > > @@ -0,0 +1,130 @@ > > +# This module exposes following variables to the project: > > Side note: Nice approach, I like it! > > > +# * HOST_C_FLAGS > > +# * DYNASM_ARCH > > +# * DYNASM_FLAGS > > + > > +# XXX: buildvm includes core headers and thus has to be built > > +# with the same flags and defines as the LuaJIT core itself. > > +set(HOST_C_FLAGS) > > +set(DYNASM_ARCH) > > +set(DYNASM_FLAGS) > > + > > +LuaJITTestArch(TESTARCH "${TARGET_C_FLAGS} ${HOST_CFLAGS}") > > Nit: Looks like it is necessary to use to define > LUAJIT_TARGET directly? I think that should be mentioned in the commit > message too. Sorry, don't get this one. > Feel free to ignore. > > > > diff --git a/cmake/SetTargetFlags.cmake b/cmake/SetTargetFlags.cmake > > new file mode 100644 > > index 0000000..260fc6b > > --- /dev/null > > +++ b/cmake/SetTargetFlags.cmake > > 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. > > I got warnings from linker in the build stage on macOS: > > | $ uname -a > | Darwin MacBook-Pro.local 19.6.0 Darwin Kernel Version 19.6.0: Mon Aug 31 22:12:52 PDT 2020; root:xnu-6153.141.2~1/RELEASE_X86_64 x86_64 > > | ld: warning: -seg1addr not 16384 byte aligned, rounding up > | /Library/Developer/CommandLineTools/usr/bin/ranlib: file: libluajit.a(lj_opt_split.c.o) has no symbols > | [ 98%] Built target libluajit_shared > | /Library/Developer/CommandLineTools/usr/bin/ranlib: file: libluajit.a(lj_opt_split.c.o) has no symbols > > 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. > > > > +# XXX: ASAN-related build flags are stored in CMAKE_C_FLAGS. > > +set(TARGET_VM_FLAGS "${CMAKE_C_FLAGS} ${TARGET_C_FLAGS}") > > + > > +unset(TESTARCH) > > Looks like you forgot to unset LUAJIT_ARCH. Or to add it in the header > note. The former, thanks! > > > > diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt > > new file mode 100644 > > index 0000000..8ada1a4 > > --- /dev/null > > +++ b/src/CMakeLists.txt > > @@ -0,0 +1,391 @@ > > > > > +make_source_list(SOURCES_UTILS > > + SOURCES > > + lj_alloc.c > > + lj_char.c > > + lj_utils_leb128.c > > + lj_vmmath.c > > + lj_wbuf.c > > Looks like there is no need to compile and > , when the memory profiler is disabled. There is no need, but strictly saying both units are not strictly related to memprof, but rather providing a standalone functionality. So, I leave it unchanged. > > > | # handle this manually on his side. Hence check whether the > | # targets used below are presented for the chosen build mode. > | # See more info in CMake docs below: > | # https://cmake.org/cmake/help/v3.1/prop_tgt/EXCLUDE_FROM_ALL.html > > 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. > > | -- Found assembler: /usr/bin/cc > | -- Configuring done > | WARNING: Target "luajit_static" has EXCLUDE_FROM_ALL set and will not be built by default but an install rule has been provided for it. CMake does not define behavior for this case. > | WARNING: Target "libluajit_static" has EXCLUDE_FROM_ALL set and will not be built by default but an install rule has been provided for it. CMake does not define behavior for this case. > | WARNING: Target "libluajit_shared" has EXCLUDE_FROM_ALL set and will not be built by default but an install rule has been provided for it. CMake does not define behavior for this case. > | WARNING: Target "libluajit_shared" has EXCLUDE_FROM_ALL set and will not be built by default but an install rule has been provided for it. CMake does not define behavior for this case. > > Can we suppress them? > > > +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? > > > + RUNTIME > > + DESTINATION bin > > + COMPONENT luajit > > +) > > +install(TARGETS ${LIBLUAJIT_STATIC_DEPS} > > + ARCHIVE > > + DESTINATION lib > > + COMPONENT luajit > > +) > > +install(TARGETS ${LIBLUAJIT_SHARED_DEPS} > > + LIBRARY > > + DESTINATION lib > > + COMPONENT luajit > > +) > > + > > > > > + > > +install(FILES > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/bc.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/bcsave.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/dump.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/p.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/v.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/zone.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/dis_x86.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/dis_x64.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/dis_arm.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/dis_arm64.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/dis_arm64be.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/dis_ppc.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/dis_mips.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/dis_mipsel.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/dis_mips64.lua > > + ${CMAKE_CURRENT_SOURCE_DIR}/jit/dis_mips64el.lua > > + ${CMAKE_CURRENT_BINARY_DIR}/jit/vmdef.lua > > Nit: Is order important? Why are they not listed in alphabetical order? > Feel free to ignore. Reordered, squashed, force-pushed to the branch. > > > + DESTINATION ${LUAJIT_DATAROOTDIR}/jit > > + PERMISSIONS > > + OWNER_READ OWNER_WRITE > > + GROUP_READ > > + WORLD_READ > > + COMPONENT luajit > > +) > > As I read from [4] CMake does not provide a default way to uninstall > installed files. But fortunately there is an example how doing it > in [4] directly. I think it is good to implement this part for > backward compatibility (especially, we have all prerequisites). OK, but I adjusted it a bit: ================================================================================ diff --git a/.gitignore b/.gitignore index a21ee1c..2103a30 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,7 @@ CMakeCache.txt CMakeFiles Makefile cmake_install.cmake +cmake_uninstall.cmake compile_commands.json install_manifest.txt luajit-parse-memprof diff --git a/CMakeLists.txt b/CMakeLists.txt index 16a9d5b..6bba9f4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -261,3 +261,16 @@ add_subdirectory(etc) # --- Tools -------------------------------------------------------------------- add_subdirectory(tools) + +# --- Misc rules --------------------------------------------------------------- + +# XXX: Implement target using the following recipe: +# https://gitlab.kitware.com/cmake/community/-/wikis/FAQ#can-i-do-make-uninstall-with-cmake +if(NOT TARGET uninstall) + configure_file(${CMAKE_MODULE_PATH}/cmake_uninstall.cmake.in + cmake_uninstall.cmake @ONLY ESCAPE_QUOTES) + + add_custom_target(uninstall + COMMAND ${CMAKE_COMMAND} -P ${CMAKE_CURRENT_BINARY_DIR}/cmake_uninstall.cmake + ) +endif() diff --git a/cmake/cmake_uninstall.cmake.in b/cmake/cmake_uninstall.cmake.in new file mode 100644 index 0000000..fc3b885 --- /dev/null +++ b/cmake/cmake_uninstall.cmake.in @@ -0,0 +1,24 @@ +if(NOT EXISTS "@CMAKE_BINARY_DIR@/install_manifest.txt") + message(FATAL_ERROR "Cannot find install manifest: @CMAKE_BINARY_DIR@/install_manifest.txt") +endif() + +# XXX: This loop removes only entries from install_manifest.txt, +# but do nothing for the directories created while installation. +# Honestly, the recipe is awful, but is better than nothing. +file(READ "@CMAKE_BINARY_DIR@/install_manifest.txt" files) +string(REGEX REPLACE "\n" ";" files "${files}") +foreach(file ${files}) + message(STATUS "Uninstalling $ENV{DESTDIR}${file}") + if(IS_SYMLINK "$ENV{DESTDIR}${file}" OR EXISTS "$ENV{DESTDIR}${file}") + execute_process( + COMMAND @CMAKE_COMMAND@ -E remove "$ENV{DESTDIR}${file}" + RESULT_VARIABLE REMOVE_RC + OUTPUT_QUIET + ) + if(NOT "${REMOVE_RC}" STREQUAL 0) + message(FATAL_ERROR "Problem when removing $ENV{DESTDIR}${file}") + endif() + else(IS_SYMLINK "$ENV{DESTDIR}${file}" OR EXISTS "$ENV{DESTDIR}${file}") + message(STATUS "File $ENV{DESTDIR}${file} does not exist.") + endif() +endforeach() ================================================================================ > > > diff --git a/src/host/CMakeLists.txt b/src/host/CMakeLists.txt > > new file mode 100644 > > index 0000000..fe2de5c > > --- /dev/null > > +++ b/src/host/CMakeLists.txt > > @@ -0,0 +1,61 @@ > > +# Building the toolchain for LuaJIT VM preprocessing. > > + > > +cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR) > > + > > +# FIXME: Both minilua and buildvm need to be build with the HOST_* > > Sorry, I don't get it. What is the HOST_* toolchain? HOST_CC, HOST_CFLAGS, HOST_LDFLAGS, HOST_LIBS, HOST_SYS -- see for more details. > > > +# toolchain. > > + > > +# XXX: DynASM flags are set considering the target arch > > > > diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt > > new file mode 100644 > > index 0000000..f9ffc5e > > --- /dev/null > > +++ b/tools/CMakeLists.txt > > @@ -0,0 +1,77 @@ > > +# Building tools for developing with uJIT. > > +# Major portions taken verbatim or adapted from the uJIT. > > +# Copyright (C) 2015-2019 IPONWEB Ltd. > > + > > +# See the rationale in the root CMakeLists.txt > > +cmake_minimum_required(VERSION 3.1 FATAL_ERROR) > > + > > +set(LUAJIT_TOOLS_DEPS) > > + > > +if(LUAJIT_DISABLE_MEMPROF) > > + message(STATUS "LuaJIT memory profiler support is disabled") > > +else() > > + # XXX: Can use genex here since the value need to be evaluated > > + # on the configuration phase. Fortunately, we know the exact > > Typo: s/on/in/ Niether of these. 'at' or 'while' are fine (I chose the former). > > > + # path where LuaJIT binary is located. > > + set(LUAJIT_TOOLS_BIN ${LUAJIT_BINARY_DIR}/${LUAJIT_CLI_NAME}) > > + set(LUAJIT_TOOLS_DIR ${CMAKE_CURRENT_SOURCE_DIR}) > > + configure_file(luajit-parse-memprof.in luajit-parse-memprof @ONLY ESCAPE_QUOTES) > > + > > > > > + install(FILES > > + ${CMAKE_CURRENT_SOURCE_DIR}/memprof.lua > > + DESTINATION ${LUAJIT_DATAROOTDIR} > > + PERMISSIONS > > + OWNER_READ OWNER_WRITE > > + GROUP_READ > > + WORLD_READ > > + COMPONENT tools-parse-memprof > > + ) > > + install(CODE > > Is it reasonable to install memrpof tooling for not supported arches? > Yes, it's already working in that way, but maybe it is redundant. I see no problem: this part works fine on all hosts, so you can take the binary dump and parse it anywhere. > > > + # XXX: Since the auxiliary script need to be configured in > > + # other way it need to be reconfigured it prior to its > > + # installation. Unfortunately, we need to manually specify > > Nit: I propose to reformulate and split the first sentence like the > following: > > | XXX: The auxiliary script needs to be configured for use in > | repository directly. In another way it needs to be reconfigured > | prior to installation. > > Feel free to ignore. See the changes below. > > > + # the installation path in command. > > + # Hope this script will be gone as a result of the issue below > > + # https://github.com/tarantool/tarantool/issues/5688. > > + " > > + set(LUAJIT_TOOLS_BIN ${CMAKE_INSTALL_PREFIX}/bin/${LUAJIT_CLI_NAME}) > > + set(LUAJIT_TOOLS_DIR ${CMAKE_INSTALL_PREFIX}/${LUAJIT_DATAROOTDIR}) > > + configure_file(${CMAKE_CURRENT_SOURCE_DIR}/luajit-parse-memprof.in > > + ${CMAKE_INSTALL_PREFIX}/bin/luajit-parse-memprof @ONLY ESCAPE_QUOTES) > > + message(STATUS \"Installing: ${CMAKE_INSTALL_PREFIX}/bin/luajit-parse-memprof\") > > + " > > + COMPONENT tools-parse-memprof > > + ) > > The result file is not executable. 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: ================================================================================ diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt index 60535cd..cb49bdc 100644 --- a/tools/CMakeLists.txt +++ b/tools/CMakeLists.txt @@ -16,6 +16,13 @@ else() # path where LuaJIT binary is located. set(LUAJIT_TOOLS_BIN ${LUAJIT_BINARY_DIR}/${LUAJIT_CLI_NAME}) set(LUAJIT_TOOLS_DIR ${CMAKE_CURRENT_SOURCE_DIR}) + # XXX: Unfortunately, there is no convenient way to set + # particular permissions to the output file via CMake. + # Furthermore, I even failed to copy the given file to the same + # path to change its permissions. After looking at the docs, I + # realized that the valid solution would be too monstrous for + # such a simple task. As a result I've made the template itself + # executable, so the issue is resolved. configure_file(luajit-parse-memprof.in luajit-parse-memprof @ONLY ESCAPE_QUOTES) add_custom_target(tools-parse-memprof EXCLUDE_FROM_ALL DEPENDS @@ -58,18 +65,24 @@ else() COMPONENT tools-parse-memprof ) install(CODE - # XXX: Since the auxiliary script need to be configured in - # other way it need to be reconfigured it prior to its - # installation. Unfortunately, we need to manually specify - # the installation path in command. - # Hope this script will be gone as a result of the issue below + # XXX: The auxiliary script needs to be configured for to be + # used in repository directly. It also need to be reconfigured + # prior to its installation. The temporary + # output is stored to the project build directory and removed + # later after being installed. This script will have gone as a + # result of the issue: # https://github.com/tarantool/tarantool/issues/5688. " + set(LJPM_INSTALL_PATH ${CMAKE_INSTALL_PREFIX}/bin/luajit-parse-memprof) set(LUAJIT_TOOLS_BIN ${CMAKE_INSTALL_PREFIX}/bin/${LUAJIT_CLI_NAME}) set(LUAJIT_TOOLS_DIR ${CMAKE_INSTALL_PREFIX}/${LUAJIT_DATAROOTDIR}) configure_file(${CMAKE_CURRENT_SOURCE_DIR}/luajit-parse-memprof.in - ${CMAKE_INSTALL_PREFIX}/bin/luajit-parse-memprof @ONLY ESCAPE_QUOTES) - message(STATUS \"Installing: ${CMAKE_INSTALL_PREFIX}/bin/luajit-parse-memprof\") + ${PROJECT_BINARY_DIR}/luajit-parse-memprof @ONLY ESCAPE_QUOTES) + file(INSTALL ${PROJECT_BINARY_DIR}/luajit-parse-memprof + DESTINATION ${CMAKE_INSTALL_PREFIX}/bin + USE_SOURCE_PERMISSIONS + ) + file(REMOVE ${PROJECT_BINARY_DIR}/luajit-parse-memprof) " COMPONENT tools-parse-memprof ) diff --git a/tools/luajit-parse-memprof.in b/tools/luajit-parse-memprof.in old mode 100644 new mode 100755 ================================================================================ > > > +endif() > > + > > +add_custom_target(LuaJIT-tools DEPENDS ${LUAJIT_TOOLS_DEPS}) > > 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. > > > -- > > 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