Re: [Tarantool-patches] [V2] luajit: pass properly compile options to LuaJIT for RelWithDebInfo

[Igor Munkin <imun@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_<UPPER-MODE> 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