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

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

[Timur Safin]

It was discovered the harder way (while debugging) that LuaJIT
sources do not generate line info debug information while we are
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,
because it would contain -g -ggdb for RelWithDebInfo, iff
selected compiler support those options.

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}})
     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
+    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

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

[Alexander Turenko]

> -    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}})

Debug:

-- CMAKE_C_FLAGS:  -fexceptions -funwind-tables -fno-omit-frame-pointer -fno-stack-protector -fno-common -fopenmp -msse2
-- CMAKE_C_FLAGS_DEBUG: -g -ggdb -O0

RelWithDebInfo:

-- CMAKE_C_FLAGS:  -fexceptions -funwind-tables -fno-omit-frame-pointer -fno-stack-protector -fno-common -fopenmp -msse2
-- CMAKE_C_FLAGS_RELWITHDEBINFO: -O2 -g -DNDEBUG -ggdb -O2

So all those -fno-omit-frame-pointer was applied to luajit and we should
proceed with each such option very carefully. For example, cite from
cmake/compiler.cmake:

 | #
 | # Set flags for all include files: those maintained by us and
 | # coming from third parties.
 | # Since we began using luajit, which uses gcc stack unwind
 | # internally, we also need to make sure all code is compiled
 | # with unwind info.
 | #
 | 
 | add_compile_flags("C;CXX" "-fexceptions" "-funwind-tables")
 | 
 | # We must set -fno-omit-frame-pointer here, since we rely
 | # on frame pointer when getting a backtrace, and it must
 | # be used consistently across all object files.
 | # The same reasoning applies to -fno-stack-protector switch.
 | 
 | if (ENABLE_BACKTRACE)
 |     add_compile_flags("C;CXX"
 |         "-fno-omit-frame-pointer"
 |         "-fno-stack-protector")
 | endif()

I would be okay to just enable '-g -ggdb' without changing base flags
from CMAKE_C_FLAGS to CMAKE_C_FLAGS_{DEBUG,RELWITHDEBINFO}, but see
below.

>      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

So there was a reason. I don't know it, but we should know if we want to
change it. Can it be related to performance?

I would ask Nick Zavaritsky if you really want to know. I searched over
commits and discussions that may be relevant, but didn't found anything.

I asked about this in [1].

[1]: https://github.com/tarantool/tarantool/issues/4827#issuecomment-665059210

WBR, Alexander Turenko.

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

[Igor Munkin]

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