[Tarantool-patches] [PATCH] cmake: add LTO support for building luajit
Alexander Turenko
alexander.turenko at tarantool.org
Wed Jul 8 15:23:57 MSK 2020
LGTM.
There are several style comments: feel free to fix them or ignore. No
need to re-review with me.
WBR, Alexander Turenko.
> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
Pasted parts of actual patch comment it.
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 10df633d5..555bc8371 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -230,6 +230,16 @@ macro(luajit_build)
> # above.
> set (luajit_ld ${CMAKE_LINKER})
> set (luajit_ar ${CMAKE_AR} rcus)
> + # Enablibg LTO for luajit if DENABLE_LTO set.
Typo: Enablibg.
Nit: I would move it below `set (luajit_strip ${CMAKE_STRIP})` line and
format it as a separate block (add an empty line before it). Just for
readability.
> + if (ENABLE_LTO)
> + message(STATUS "Enable LTO for luajit")
> + set (luajit_ldflags ${luajit_ldflags} ${LDFLAGS_LTO})
> + message(STATUS "ld: " ${luajit_ldflags})
> + set (luajit_cflags ${luajit_cflags} ${CFLAGS_LTO})
> + message(STATUS "cflags: " ${luajit_cflags})
> + set (luajit_ar ${AR_LTO} rcus)
> + message(STATUS "ar: " ${luajit_ar})
> + endif()
Nit: Those variables are lists and they are printed without spaces
(I don't know exact reason):
| -- Enable LTO for luajit
| -- ld: -Wno-lto-type-mismatch-Wno-lto-type-mismatch
| -- cflags: -Wno-unknown-pragmas-fexceptions-funwind-tables-fno-omit-frame-pointer-fno-stack-protector-fno-common-msse2-Wno-parentheses-equality-Wno-tautological-compare-Wno-varargs-Wno-implicit-fallthrough-flto=thin
| -- ar: "/usr/lib/llvm-8/bin/llvm-ar"rcus
You should include them into quotes, like so:
| -message(STATUS "ld: " ${luajit_ldflags})
| +message(STATUS "ld: ${luajit_ldflags}")
(BTW, it is not ld executable, but ldflags; so I would name it 'ldflags'
in the output.)
Nit: LTO flags are already printed before:
| -- ld version string: GNU ld (GNU Binutils for Debian) 2.31.1
| -- Found ld.bfd version: 2.31
| -- CFLAGS_LTO: -flto=thin
| -- LDFLAGS_LTO: -Wno-lto-type-mismatch
| -- AR_LTO: "/usr/lib/llvm-8/bin/llvm-ar"
| -- Enabling LTO: TRUE
But I don't have strict objection against printing it again.
More information about the Tarantool-patches
mailing list