Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia <arkholga@tarantool.org>]

Hi!

Thank you guys for the deep review and your patience.

I have fixed issues that you have underlined.

Added messages with options in case DENABLE_LTO=ON in order to see all 
flags that we enabling.

Checked with file/readelf and updated branch. So the patch(luajit part) 
looks like:

   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.
   +    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()
        set (luajit_strip ${CMAKE_STRIP})
~


16.06.2020 21:31, Alexander Turenko пишет:
> My review is not sufficient anymore, since I wrote some parts of the
> code. Sergey, Timur, can you participate here?
>
>> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+ due
>> errors and slow work.
> It seems it is not actual anymore (at least for ld). If you want to
> highlight minimal requirements, I think it would be better to mention
> f9e28ce4602aff3f9bb4e743b0d6167b0f8df88d commit.
>
> Typo: Enablibg.
>
>> +    if (${ENABLE_LTO})
> No need to use ${var} inside if condition. See the surrounding code.
>
>> +`......message(STATUS "Enablig LTO for luajit")
>> +        set (luajit_ld ${CMAKE_C_LINK_OPTIONS_IPO})
> The variable is reassigned right below (it is why the build does not
> fail).
>
> It seems, you should find a way to verify that your changes actually
> enable LTO for luajit. Either by looking into actual compiler / linker
> options, or by looking into compiled code differences (say, find a cheap
> function in one compilation unit and verify that it is inlined into
> another one).
>
> It uses CMAKE_C_LINK_OPTIONS_IPO internal variable directly and ignores
> LDFLAGS_LTO that is set by lto.cmake. I suggest to use LDFLAGS_LTO.
>
> I guess you want to set luajit_ldflags, not luajit_ld.
>
> Use spaces for indent in CMake and Lua files. I guess all modern text
> editors may be properly configured to have different indent symbol /
> level for different file types.
>
> NB: I suggest to rebase your patchsets on top of master on each review
> iteration.
>
> ----
>
> I know, it is really hard to read the same code again and again when
> everything looks finished. But it is inevitable part of any development
> process due to the human nature: we do mistakes and should build
> processes that allow us to catch them.
>
> I suggest to introduce a personal self-review checklist that will grow
> with review comments from teammates.