From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp10.mail.ru (smtp10.mail.ru [94.100.181.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id EBCA441C5DB for ; Mon, 29 Jun 2020 23:16:14 +0300 (MSK) References: <20200312100549.31608-1-arkholga@tarantool.org> <20200616010232.3j2zvwo7z6lmnody@tkn_work_nb> <20200616183120.3dbhchwynjpv7fdp@tkn_work_nb> From: Olga Arkhangelskaia Message-ID: <0f3b79f4-944a-c579-7cce-851f7dff1c68@tarantool.org> Date: Mon, 29 Jun 2020 23:16:13 +0300 MIME-Version: 1.0 In-Reply-To: <20200616183120.3dbhchwynjpv7fdp@tkn_work_nb> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.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.