[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