From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Igor Munkin <imun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake Date: Thu, 11 Feb 2021 22:23:36 +0300 [thread overview] Message-ID: <20210211192336.GC9361@root> (raw) In-Reply-To: <7cd42be38d86c832ecd4ba0f3edd7ae83aead7ad.1612291495.git.imun@tarantool.org> 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. 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/ > GNU Make to CMake. These changes provides CMake build system only for Typo: s/provides/provide/ > the following OS: GNU/Linux, OSX, FreeBSD. For other platrforms use the Typo: s/platrforms/platforms/ > 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. > > 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 <imun@tarantool.org> > --- 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? > .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 <doc/install.html> (change Makefile -> Makefile.original)? Looks more related to the previous patch. > > diff --git a/.gitignore b/.gitignore > index 1a07bf7..a21ee1c 100644 > --- a/.gitignore > +++ b/.gitignore <snipped> > 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. > +# 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/ > +# covered with the tests in our CI, there is no need to support Typo: s/covered with/covered by/ > +# others for now and the original build system can be used. > +# > + > +if(NOT( > + CMAKE_SYSTEM_NAME STREQUAL "Linux" OR > + CMAKE_SYSTEM_NAME STREQUAL "Darwin" OR > + CMAKE_SYSTEM_NAME STREQUAL "FreeBSD" > +)) > + message(FATAL_ERROR > + "Please use the old build system:\n\tmake -f Makefile.original <options>" > + ) > +endif() > + > +# --- Fine-tuning cmake environment -------------------------------------------- > + > +set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake") > +set(CMAKE_EXPORT_COMPILE_COMMANDS ON) > + > +include(LuaJITUtils) > +include(SetVersion) > + > +# --- Variables to be exported to child scopes --------------------------------- > + > +SetVersion( > + LUAJIT_VERSION > + LUAJIT_VERSION_MAJOR > + LUAJIT_VERSION_MINOR > + LUAJIT_VERSION_PATCH > + LUAJIT_VERSION_TWEAK > + LUAJIT_PRERELEASE > +) > + > +set(LUAJIT_SOURCE_DIR "${PROJECT_SOURCE_DIR}/src") > +set(LUAJIT_BINARY_DIR "${PROJECT_BINARY_DIR}/src") > + > +# Names of the CLI binaries. > +set(LUAJIT_CLI_NAME "luajit") > +set(LUAJIT_LIB_NAME "luajit") > + > +# Specialized install paths. > +set(LUAJIT_DATAROOTDIR > + share/luajit-${LUAJIT_VERSION_MAJOR}.${LUAJIT_VERSION_MINOR}.${LUAJIT_VERSION_PATCH}${LUAJIT_PRERELEASE} > +) > +set(LUAJIT_INCLUDEDIR > + include/luajit-${LUAJIT_VERSION_MAJOR}.${LUAJIT_VERSION_MINOR} > +) > + > +set(BUILDMODE_VALUES mixed static dynamic) See nothing bad to transfer comments described each option here. > +list(GET BUILDMODE_VALUES 0 BUILDMODE_DEFAULT) > +set(BUILDMODE ${BUILDMODE_DEFAULT} CACHE STRING > + "Build mode. Choose one of the following: ${BUILDMODE_VALUES}." > +) > +set_property(CACHE BUILDMODE PROPERTY STRINGS ${BUILDMODE_VALUES}) > + > +# Check that BUILDMODE value is correct. > +# FIXME: In CMake 3.5 we'll be able to use IN_LIST here. > +list(FIND BUILDMODE_VALUES ${BUILDMODE} BUILDMODE_INDEX) > +if(BUILDMODE_INDEX EQUAL -1) > + message(FATAL_ERROR "BUILDMODE must be one of the following: ${BUILDMODE_VALUES}.") > +endif() > + > +# --- 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? 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 > +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? > + > +# Since the assembler part does NOT maintain a frame pointer, it's > +# pointless to slow down the C part by not omitting it. Debugging, > +# tracebacks and unwinding are not affected -- the assembler part > +# has frame unwind information and GCC emits it where needed (x64) > +# or with -g. > +AppendFlags(CMAKE_C_FLAGS -fomit-frame-pointer -fno-stack-protector) > + > +# Re-defined to benefit from expanding macros in gdb. Typo: s/Re-defined/Redefined/ > +set(CMAKE_C_FLAGS_DEBUG "-g -ggdb3") > +set(CMAKE_C_FLAGS_RELWITHDEBINFO "-O2 -DNDEBUG -g -ggdb3") Side note: Yes!!! Finally, we don't need to rewrite all macroses by our own in console when debugging. This is a good one! > +# Re-defined since default cmake release optimization level is O3. Typo: s/Re-defined/Redefined/ > +set(CMAKE_C_FLAGS_RELEASE "-O2 -DNDEBUG") > + > +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. > +if(LUAJIT_ENABLE_WARNINGS) > + AppendFlags(CMAKE_C_FLAGS > + -Wextra > + -Wdeclaration-after-statement > + -Wpointer-arith > + -Wredundant-decls > + -Wshadow > + ) > +endif() > + > +# Auxilary flags for main targets (libraries, binaries). Typo: s/Auxilary/Auxiliary/ > +AppendFlags(TARGET_C_FLAGS > + -D_FILE_OFFSET_BITS=64 > + -D_LARGEFILE_SOURCE > + -U_FORTIFY_SOURCE > +) > + > +# 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`. > + > +# Features from Lua 5.2 that are unlikely to break existing code > +# are enabled by default. Some other features that *might* break > +# some existing code (e.g. __pairs or os.execute() return values) > +# can be enabled here. > +# XXX: this does not provide full compatibility with Lua 5.2 at > +# this time. > +option(LUAJIT_LUA52COMPAT "Compatibility with Lua 5.2" OFF) > +if(LUAJIT_LUA52COMPAT) This has been already fixed on branch, thanks! > + AppendFlags(TARGET_C_FLAGS -DLUAJIT_ENABLE_LUA52COMPAT) > +endif() > + > +# Disable the JIT compiler, i.e. turn LuaJIT into a pure > +# interpreter. > +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) Ditto. > + > +# 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? > +# override the default 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 > + "Switch to single-number or dual-number mode." > +) > +# XXX: explicitly added empty string allows to disable this flag > +# in GUI. > +set_property(CACHE LUAJIT_NUMMODE PROPERTY STRINGS > + ${LUAJIT_NUMMODE_DEFAULT} ${LUAJIT_NUMMODE_VALUES} > +) <snipped> > +# Switch to harder (and slower) hash function when a collision > +# chain in the string hash table exceeds certain length. Typo: s/certain length/a certain length/ > +option(LUAJIT_SMART_STRINGS "Harder string hashing function" ON) > +if(LUAJIT_SMART_STRINGS) > + AppendFlags(TARGET_C_FLAGS -DLUAJIT_SMART_STRINGS=1) > +endif() > + 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. > +# XXX: Note that most of the options below are NOT suitable for > +# benchmarking or release mode! <snipped> > +# 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 :( > + > +# 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) > + AppendFlags(TARGET_C_FLAGS -DLUA_USE_ASSERT) > +endif() Ditto. > + > +# 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 | $ 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:<td class="excplatform">ARM <tt>-DLUAJIT_UNWIND_EXTERNAL</tt></td> | 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? > + > +# --- Main source tree --------------------------------------------------------- <snipped> > 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: <execute_process> 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. 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. > + 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. > + 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/CMakeFiles/buildvm.dir/build.make>: | 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. 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. Nit: As far as we can't organize this CPU arches in alphabetical order I propose reverse-alphabetical ;): | X86 X64 PPC MIPS64 MIPS ARM64 ARM Feel free to ignore (it's more a joke). > + 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. 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 `continue()` command. > + 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 <lj_arch.h> 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 > +endfunction() <snipped> > diff --git a/cmake/MakeSourceList.cmake b/cmake/MakeSourceList.cmake > new file mode 100644 > index 0000000..fa455bb > --- /dev/null > +++ b/cmake/MakeSourceList.cmake > @@ -0,0 +1,47 @@ <snipped> > + include(CMakeParseArguments) # if we update to CMake >= 3.5, can remove this line Nit: Please move comment to the previous line with the following changes: s/if/If/; s/this/the next/; s/line/line./. Feel free to ignore. > + cmake_parse_arguments(${prefix} <snipped> > 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 <Makefile.original> to define LUAJIT_TARGET directly? I think that should be mentioned in the commit message too. Feel free to ignore. > +LuaJITArch(LUAJIT_ARCH "${TESTARCH}") > +AppendFlags(HOST_C_FLAGS -DLUAJIT_TARGET=LUAJIT_ARCH_${LUAJIT_ARCH}) > + > +# XXX: LUAJIT_ARCH equals to DYNASM_ARCH for the most case but Typo: /case/cases,/ > +# there are few exceptions to the rule. <snipped> > diff --git a/cmake/SetTargetFlags.cmake b/cmake/SetTargetFlags.cmake > new file mode 100644 > index 0000000..260fc6b > --- /dev/null > +++ b/cmake/SetTargetFlags.cmake > @@ -0,0 +1,42 @@ > +# This module exposes following variables to the project: > +# * BUILDVM_MODE > +# * TARGET_C_FLAGS > +# * TARGET_VM_FLAGS > +# * TARGET_BIN_FLAGS > +# * TARGET_SHARED_FLAGS > +# * TARGET_LIBS > + <snipped> > +if(CMAKE_SYSTEM_NAME STREQUAL "Darwin") > + if(LUAJIT_ARCH STREQUAL "x64") > + AppendFlags(TARGET_BIN_FLAGS -pagezero_size 10000 -image_base 100000000) > + AppendFlags(TARGET_SHARED_FLAGS -image_base 7fff04c4a000) > + endif() > + AppendFlags(TARGET_SHARED_FLAGS -single_module -undefined dynamic_lookup) Are -dynamiclib and -fPIC flags forced set by CMake? Also, I don't find -install_name -compatibility_version -current_version flags. Are they insignificant? 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? > +else() # Linux and FreeBSD. > + AppendFlags(TARGET_BIN_FLAGS -Wl,-E) > + list(APPEND TARGET_LIBS dl) > +endif() > + > +# Auxilary flags for the VM core. Typo: s/Auxilary/Auxiliary/ > +# 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. > diff --git a/cmake/SetVersion.cmake b/cmake/SetVersion.cmake > new file mode 100646 > index 0000000..3f0247c > --- /dev/null > +++ b/cmake/SetVersion.cmake > @@ -0,0 +1,45 @@ > +# Find, check and set LuaJIT's version from a VCS tag. > +# Major portions taken verbatim or adapted from the uJIT. > +# Copyright (C) 2015-2019 IPONWEB Ltd. > + > +function(SetVersion version majver minver patchver tweakver prerel) > + find_package(Git QUIET REQUIRED) > + if(EXISTS ${CMAKE_SOURCE_DIR}/.git AND Git_FOUND) > + # Read version from the project's VCS and store the result > + # into version. > + execute_process( > + COMMAND ${GIT_EXECUTABLE} describe > + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} > + OUTPUT_VARIABLE vcs_tag) > + string(STRIP "${vcs_tag}" vcs_tag) > + message(STATUS "[SetVersion] Reading version from VCS: ${vcs_tag}") > + else() > + # Use default version since no git is found in the system or > + # VCS directory is not found in repo root directory. Typo: s/in repo root/in the repo root/ > + set(vcs_tag "v2.1.0-beta3-0-g0000000") > + message(STATUS "[SetVersion] No VCS found, use default version: ${vcs_tag}") > + endif() <snipped> > diff --git a/etc/CMakeLists.txt b/etc/CMakeLists.txt > new file mode 100644 > index 0000000..4a4c3cd > --- /dev/null > +++ b/etc/CMakeLists.txt > @@ -0,0 +1,32 @@ <snipped> > 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 @@ <snipped> > +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 <lj_utils_leb128.c> and <lj_wbuf.c>, when the memory profiler is disabled. > +) > + > +make_source_list(SOURCES_PROFILER > + SOURCES > + lj_memprof.c > + lj_profile.c > +) > + > +# Lua standard library + extensions by LuaJIT. > +make_source_list(SOURCES_LUA_LIB > + # XXX: Please do not changes the order of the libraries Typo: s/changes/change/ > + # (required by buildvm). > + SOURCES > + lib_base.c > + lib_math.c > + lib_bit.c > + lib_string.c > + lib_table.c > + lib_io.c > + lib_os.c > + lib_package.c > + lib_debug.c > + lib_jit.c > + lib_ffi.c > + lib_misc.c > +) > + > +# JIT compiler, core part. > +make_source_list(SOURCES_JIT_CORE > + SOURCES > + lj_asm.c > + lj_ffrecord.c > + lj_ir.c > + lj_mcode.c > + lj_record.c > + lj_snap.c > + lj_trace.c > +) > + > +# JIT compiler, machine-independent optimisations. Typo: s/optimisations/optimizations/ > +make_source_list(SOURCES_JIT_OPT > + SOURCES > + lj_opt_dce.c > + lj_opt_fold.c > + lj_opt_loop.c > + lj_opt_mem.c > + lj_opt_narrow.c > + lj_opt_sink.c > + lj_opt_split.c > +) <snipped> > +# Everything except FFI and JIT. > +make_source_list(SOURCES_CORE_NO_JIT_FFI > + SOURCES > + ${SOURCES_RUNTIME} > + ${SOURCES_LUA_LIB} > + ${SOURCES_FRONTEND} > + ${SOURCES_PROFILER} > + ${SOURCES_UTILS} > +) > + > +set(SOURCES_CORE ${SOURCES_CORE_NO_JIT_FFI}) > + > +if(LUAJIT_HAS_JIT) Here `LUAJIT_HAS_JIT` is "NOT LUAJIT_DISABLE_JIT" (or just "LUAJIT_DISABLE_JIT"), so this branch is always taken. > + 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) Ditto. > + list(APPEND SOURCES_CORE ${SOURCES_FFI}) > + if(NOT LUAJIT_HAS_JIT) > + # Needed for lj_mcode_sync. > + list(APPEND SOURCES_CORE ${CMAKE_CURRENT_SOURCE_DIR}/lj_mcode.c) > + endif() > +endif() > + <snipped> > +add_custom_target(libluajit DEPENDS ${LIBLUAJIT_DEPS}) > +add_custom_target(luajit ALL DEPENDS libluajit ${LUAJIT_DEPS}) > + Just highlight typo from the branch version. | # Unfortunately, CMake provides no guarantees for install commands | # used for the targets excluded from <all> and obligues user to Typo: s/obligues/obliges/ | # 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): | -- 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? > + 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 > +) > + <snipped> > + > +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. > + 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). > 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? > +# toolchain. > + > +# XXX: DynASM flags are set considering the target arch Typo: s/arch/arch./. > +include(SetDynASMFlags) > + > +# --- Build minilua ------------------------------------------------------------ > + <snipped> > 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/ > + # 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) > + <snipped> > + 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. > + # 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. > + # the installation path in <configure_file> 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. > +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? > -- > 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
next prev parent reply other threads:[~2021-02-11 19:24 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-02 20:57 [Tarantool-patches] [PATCH luajit 0/5] Self-sufficient LuaJIT testing environment Igor Munkin via Tarantool-patches 2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 1/5] build: preserve the original build system Igor Munkin via Tarantool-patches 2021-02-04 22:53 ` Timur Safin via Tarantool-patches 2021-02-08 15:56 ` Igor Munkin via Tarantool-patches 2021-02-09 11:38 ` Sergey Kaplun via Tarantool-patches 2021-02-09 12:47 ` Igor Munkin via Tarantool-patches 2021-02-09 14:45 ` Sergey Kaplun via Tarantool-patches 2021-02-09 15:28 ` Igor Munkin via Tarantool-patches 2021-02-10 9:35 ` Sergey Kaplun via Tarantool-patches 2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake Igor Munkin via Tarantool-patches 2021-02-04 22:53 ` Timur Safin via Tarantool-patches 2021-02-08 15:56 ` Igor Munkin via Tarantool-patches 2021-02-09 13:55 ` Timur Safin via Tarantool-patches 2021-02-09 15:09 ` Igor Munkin via Tarantool-patches 2021-02-11 19:23 ` Sergey Kaplun via Tarantool-patches [this message] 2021-02-16 15:28 ` Igor Munkin via Tarantool-patches 2021-02-18 9:56 ` Sergey Kaplun via Tarantool-patches 2021-02-20 19:18 ` Igor Munkin via Tarantool-patches 2021-02-27 10:48 ` Sergey Kaplun via Tarantool-patches 2021-02-28 18:18 ` Igor Munkin via Tarantool-patches 2021-02-13 3:47 ` Sergey Kaplun via Tarantool-patches 2021-02-16 15:32 ` Igor Munkin via Tarantool-patches 2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 3/5] test: run LuaJIT tests via CMake Igor Munkin via Tarantool-patches 2021-02-08 15:05 ` Timur Safin via Tarantool-patches 2021-02-08 16:29 ` Igor Munkin via Tarantool-patches 2021-02-09 8:16 ` Timur Safin via Tarantool-patches 2021-02-09 8:43 ` Igor Munkin via Tarantool-patches 2021-02-09 13:59 ` Timur Safin via Tarantool-patches 2021-02-09 15:10 ` Igor Munkin via Tarantool-patches 2021-02-14 18:48 ` Sergey Kaplun via Tarantool-patches 2021-02-19 19:04 ` Igor Munkin via Tarantool-patches 2021-02-27 13:50 ` Sergey Kaplun via Tarantool-patches 2021-02-28 18:18 ` Igor Munkin via Tarantool-patches 2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 4/5] test: fix warnings found with luacheck in misclib* Igor Munkin via Tarantool-patches [not found] ` <012f01d6fe1a$a2aa6890$e7ff39b0$@tarantool.org> [not found] ` <2c495492-50f4-acfd-ad66-2cb44abb5fa1@tarantool.org> 2021-02-08 15:40 ` Sergey Bronnikov via Tarantool-patches 2021-02-08 15:58 ` Igor Munkin via Tarantool-patches 2021-02-08 15:57 ` Igor Munkin via Tarantool-patches 2021-02-14 19:16 ` Sergey Kaplun via Tarantool-patches 2021-02-16 15:29 ` Igor Munkin via Tarantool-patches 2021-02-16 16:36 ` Sergey Kaplun via Tarantool-patches 2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 5/5] test: run luacheck static analysis via CMake Igor Munkin via Tarantool-patches 2021-02-04 22:52 ` Timur Safin via Tarantool-patches 2021-02-08 15:57 ` Igor Munkin via Tarantool-patches 2021-02-14 19:32 ` Sergey Kaplun via Tarantool-patches 2021-02-19 19:14 ` Igor Munkin via Tarantool-patches 2021-02-28 22:04 ` [Tarantool-patches] [PATCH luajit 0/5] Self-sufficient LuaJIT testing environment Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210211192336.GC9361@root \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox