From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 C1479445320 for ; Thu, 6 Aug 2020 17:52:15 +0300 (MSK) Date: Thu, 6 Aug 2020 17:41:53 +0300 From: Igor Munkin Message-ID: <20200806144153.GW18920@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [V2] luajit: pass properly compile options to LuaJIT for RelWithDebInfo List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Timur Safin Cc: tarantool-patches@dev.tarantool.org Timur, Thanks for the patch! Please, consider my comments below. On 24.07.20, Timur Safin wrote: > It was discovered the harder way (while debugging) that LuaJIT > sources do not generate line info debug information while we are Typo (still on the remote branch): s/whil/while/. Typo (still on the remote branch): s/we in/we are in/. > in any of optimized modes (e.g. RelWithDebInfo). > > Simplistically we need to append `-g -ggdb` to optimization > flags in luajit_cflags, but that would be fragile in the longer > run. We need just to pass CMAKE_C_FLAGS_ instead, Well, you claim that the simple solution is fragile, but I don't see the reason. Could you please clarify your concerns? > because it would contain -g -ggdb for RelWithDebInfo, iff Typo: s/iff/if/. > selected compiler support those options. Typo: s/support/supports/. > > Closes #4827 > --- > cmake/luajit.cmake | 17 ++++++----------- > 1 file changed, 6 insertions(+), 11 deletions(-) > > diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake > index 555bc8371..76155c0c6 100644 > --- a/cmake/luajit.cmake > +++ b/cmake/luajit.cmake > @@ -139,7 +139,9 @@ macro(luajit_build) > # will most certainly wreak havok. > # > # This stuff is extremely fragile, proceed with caution. > - set (luajit_cflags ${CMAKE_C_FLAGS}) > + string (TOUPPER ${CMAKE_BUILD_TYPE} upper_build_type) > + set (c_flags_init "CMAKE_C_FLAGS_${upper_build_type}") > + set (luajit_cflags ${${c_flags_init}}) Why do you set the new flags, instead of appending them to ${CMAKE_C_FLAGS}? If there is a particular reason, please drop a few words regarding it. Besides, I checked LuaJIT build flags: * prior to your patch | /usr/bin/cc -O2 -Wall -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -DNDEBUG | -DNVALGRIND -DNVALGRIND=1 -DLUAJIT_SMART_STRINGS=1 -D_FILE_OFFSET_BITS=64 | -D_LARGEFILE_SOURCE -U_FORTIFY_SOURCE -fno-stack-protector -fexceptions | -funwind-tables -fno-omit-frame-pointer -fno-stack-protector -fno-common | -fopenmp -msse2 -Wno-parentheses-equality -Wno-tautological-compare | -Wno-misleading-indent ation -Wno-varargs -Wno-implicit-fallthrough -c | -o lj_record.o lj_record.c * and with your changes | /usr/bin/cc -Wall -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -DNDEBUG | -DNVALGRIND -DNVALGRIND=1 -DLUAJIT_SMART_STRINGS=1 -D_FILE_OFFSET_BITS=64 | -D_LARGEFILE_SOURCE -U_FORTIFY_SOURCE -fno-stack-protector -O2 -g -DNDEBUG | -ggdb -O2 -Wno-parentheses-equality -Wno-tautological-compare | -Wno-misleading-indentation -Wno-varargs -Wno-implicit-fallthrough -c | -o lj_record.o lj_record.c I have the same concerns Sasha mentioned in his review regarding -fexceptions and -funwind-tables flags. There are several issues in LuaJIT queue but it seems all them are related to the "host" program build (i.e. Tarantool), not LuaJIT itself. However, -fno-omit-frame-pointer is set for internal Tarantool purposes (despite Mike's comments in LuaJIT Makefile). So, I guess you can't freely drop them. > set (luajit_ldflags ${CMAKE_EXE_LINKER_FLAGS}) > separate_arguments(luajit_cflags) > separate_arguments(luajit_ldflags) > @@ -168,19 +170,12 @@ macro(luajit_build) > "-pagezero_size 10000 -image_base 100000000") > endif() > > - # We are consciously ommiting debug info in RelWithDebInfo mode Here I also agree with Sasha here: there was a reason to omit debuginfo for RelWithDebInfo builds, but you mentioned a word neither in commit message nor in a comment nearby. > + set (upper_build_type) > + set (c_flags_init) > + > if (${CMAKE_BUILD_TYPE} STREQUAL "Debug") > - set (luajit_ccopt -O0) > - if (CC_HAS_GGDB) > - set (luajit_ccdebug -g -ggdb) > - else () > - set (luajit_ccdebug -g) > - endif () > add_definitions(-DLUA_USE_APICHECK=1) > add_definitions(-DLUA_USE_ASSERT=1) > - else () > - set (luajit_ccopt -O2) > - set (luajit_ccdbebug "") > endif() > if (${CMAKE_SYSTEM_NAME} STREQUAL Darwin) > # Pass sysroot - prepended in front of system header/lib dirs, > -- > 2.20.1 > -- Best regards, IM