[Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake
Sergey Kaplun
skaplun at tarantool.org
Thu Feb 11 22:23:36 MSK 2021
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 at 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
More information about the Tarantool-patches
mailing list