Re: [Tarantool-patches] [PATCH luajit 1/6] Cleanup and enable external unwinding for more platforms.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maksim!

Thanks for the patch!
LGTM, just a few nits and several small side notes below.

On 26.09.22, Maksim Kokryashkin wrote:
> (cherry picked from commit e131936133c58de4426c595db2341caf5a1665b5)
> 
> This commit enables external unwiding on all platforms, that

Typo: s/unwiding/unwinding/

> create unwind tables by default. Corresponding check is added
> to the build routine.
> 
> Also, GC64 mode is now enabled on MacOS by default.

Maybe we should notice, that non-GC64 mode is now forbidden for MacOs?

> 
> Maxim Kokryashkin:
> * added the description for the patch
> 
> Needed for tarantool/tarantool#6096
> Needed for tarantool/tarantool#7230
> ---
>  CMakeLists.txt             |   3 +
>  cmake/SetTargetFlags.cmake |  22 ++-
>  doc/extensions.html        |  22 +--
>  src/Makefile.original      |  11 +-
>  src/lj_arch.h              |  27 ++--
>  src/lj_err.c               | 321 +++++++++++++++++++------------------
>  6 files changed, 213 insertions(+), 193 deletions(-)
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 8b49f9d7..c4f3ef62 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -176,6 +176,9 @@ endif()
>  
>  # Enable GC64 mode for x64.
>  option(LUAJIT_ENABLE_GC64 "GC64 mode for x64" OFF)
> +if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> +  set(LUAJIT_ENABLE_GC64 ON)
> +endif()

Side note: So, we shouldn't forget to remove it additional set in the
Tarantool build system.

Also, I see no fortified enabled in the LuaJIT original commit. Is this
because this commit is later than enabling GC64 mode by default?

>  if(LUAJIT_ENABLE_GC64)
>    AppendFlags(TARGET_C_FLAGS -DLUAJIT_ENABLE_GC64)
>  endif()
> diff --git a/cmake/SetTargetFlags.cmake b/cmake/SetTargetFlags.cmake
> index b544d2ac..36896aff 100644
> --- a/cmake/SetTargetFlags.cmake
> +++ b/cmake/SetTargetFlags.cmake
> @@ -15,6 +15,22 @@ endif()
>  LuaJITTestArch(TESTARCH "${TARGET_C_FLAGS}")
>  LuaJITArch(LUAJIT_ARCH "${TESTARCH}")
>  
> +string(FIND ${TARGET_C_FLAGS} "LJ_NO_UNWIND 1" UNWIND_POS)
> +if(UNWIND_POS EQUAL -1)
> +  execute_process(
> +    COMMAND bash -c "exec 2>/dev/null; echo 'extern void b(void);int a(void){b();return 0;}' | ${CMAKE_C_COMPILER} -c -x c - -o tmpunwind.o && grep -qa -e eh_frame -e __unwind_info tmpunwind.o && echo E; rm -f tmpunwind.o"

Side note: I'm OK with the same check as Mike did.
Does `check_c_compiler_flag(-funwind-tables TESTUNWIND)` check the same
thing, but in a more readable way?

> +    WORKING_DIRECTORY ${LUAJIT_SOURCE_DIR}
> +    OUTPUT_VARIABLE TESTUNWIND
> +    RESULT_VARIABLE TESTUNWIND_RC
> +  )
> +  if(TESTUNWIND_RC EQUAL 0)
> +    string(FIND "${TESTUNWIND}" "E" UNW_TEST_POS)
> +    if(NOT UNW_TEST_POS EQUAL -1)
> +      AppendFlags(TARGET_C_FLAGS -DLUAJIT_UNWIND_EXTERNAL)
> +    endif()
> +  endif()
> +endif()
> +
>  # Target-specific compiler options.
>  #
>  # x86/x64 only: For GCC 4.2 or higher and if you don't intend to
> @@ -25,12 +41,6 @@ if(LUAJIT_ARCH STREQUAL "x86")
>  endif()
>  

<snipped>

> diff --git a/doc/extensions.html b/doc/extensions.html
> index e0f136e2..777e9928 100644
> --- a/doc/extensions.html
> +++ b/doc/extensions.html

<snipped>

> diff --git a/src/Makefile.original b/src/Makefile.original
> index dd1c6a76..c9609700 100644
> --- a/src/Makefile.original
> +++ b/src/Makefile.original

<snipped>

> diff --git a/src/lj_arch.h b/src/lj_arch.h
> index 40129d9e..730be5bf 100644
> --- a/src/lj_arch.h
> +++ b/src/lj_arch.h

<snipped>

> diff --git a/src/lj_err.c b/src/lj_err.c
> index c310daf6..298e5434 100644
> --- a/src/lj_err.c
> +++ b/src/lj_err.c

<snipped>

> 

-- 
Best regards,
Sergey Kaplun