Re: [Tarantool-patches] [PATCH luajit] sysprof: replace `backtrace` with libunwind

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

Hi, Maxim!

Sorry for such late review!

The patch is generally LGTM, except a bunch of nits and questions
below.

On 12.06.22, Maxim Kokryashkin wrote:
> `backtrace` fails to unwind the host stack in LuaJIT, since there is

Typo: s/is/are/

> no frame pointers during the vm calls. Sometimes, those failures cause
> crashes. This commit replaces it with the libunwind-based unwinder,
> which makes use of additional runtime info to provide robust unwinding
> without any frame pointers.
> 
> Also, this commit enables C API tests, which used to crash with
> `backtrace`.
> 
> Part of tarantool/tarantool#781
> ---
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/sysprof-libunwind
> PR: https://github.com/tarantool/tarantool/pull/7198
> 
>  CMakeLists.txt                                | 38 ++++++--
>  cmake/FindLibUnwind.cmake                     | 86 +++++++++++++++++++
>  cmake/GetLibUnwindVersion.cmake               | 12 +++
>  src/CMakeLists.txt                            |  4 +
>  src/lj_sysprof.c                              | 51 ++++++++---
>  test/luajit-test-init.lua                     | 41 +++++++++
>  test/tarantool-tests/CMakeLists.txt           |  6 ++
>  .../misclib-sysprof-capi.test.lua             | 12 +--
>  .../misclib-sysprof-lapi.test.lua             |  5 +-
>  9 files changed, 225 insertions(+), 30 deletions(-)
>  create mode 100644 cmake/FindLibUnwind.cmake
>  create mode 100644 cmake/GetLibUnwindVersion.cmake
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 659ee879..d62d5b34 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -89,12 +89,7 @@ if(CMAKE_LIBRARY_ARCHITECTURE)
>    AppendFlags(TARGET_C_FLAGS -DLUA_MULTILIB='"lib/${CMAKE_LIBRARY_ARCHITECTURE}"')
>  endif()
>  
> -# Since the assembler part does NOT maintain a frame pointer, it's
> -# pointless to slow down the C part by not omitting it. Debugging,
> -# tracebacks and unwinding are not affected -- the assembler part
> -# has frame unwind information and GCC emits it where needed (x64)
> -# or with -g.

Minor: We can mention here, that `-fomit-frame-pointer` is set depending
of SYSPROF and LIBUNWIND support.

> -AppendFlags(CMAKE_C_FLAGS -fomit-frame-pointer -fno-stack-protector)
> +AppendFlags(CMAKE_C_FLAGS -fno-stack-protector)

Just interesting: how much it drops the performance for C part.

>  
>  # Redefined to benefit from expanding macros in gdb.
>  set(CMAKE_C_FLAGS_DEBUG "-g -ggdb3")
> @@ -188,6 +183,37 @@ endif()
>  option(LUAJIT_DISABLE_SYSPROF "LuaJIT platform and Lua profiler support" OFF)
>  if(LUAJIT_DISABLE_SYSPROF)
>    AppendFlags(TARGET_C_FLAGS -DLUAJIT_DISABLE_SYSPROF)
> +  AppendFlags(CMAKE_C_FLAGS -fomit-frame-pointer)
> +else()
> +  if(CMAKE_SYSTEM_NAME STREQUAL "Linux" AND

Maybe it is better to move this logic to <cmake/LibUnwind.cmake> or
something named like that. <CMakeLists.txt> is only about to set
options and flags. Not such logic, IMO.

> +    (
> +      CMAKE_SYSTEM_PROCESSOR STREQUAL "i386" OR
> +      CMAKE_SYSTEM_PROCESSOR STREQUAL "i686" OR
> +      CMAKE_SYSTEM_PROCESSOR STREQUAL "x86_64"
> +    )
> +  )
> +    # XXX: Libunwind can be provided externally.
> +    if(NOT LIBUNWIND_LIBRARIES)
> +      find_package(LibUnwind MODULE QUIET)
> +    endif()
> +
> +    if(NOT LIBUNWIND_FOUND AND NOT LIBUNWIND_LIBRARIES)
> +      set(LUAJIT_DISABLE_SYSPROF ON)
> +      AppendFlags(TARGET_C_FLAGS -DLUAJIT_DISABLE_SYSPROF)
> +      AppendFlags(CMAKE_C_FLAGS -fomit-frame-pointer)

I suppose, that removed comment above (about frame pointer) can be moved
here.

> +      message(STATUS "Libunwind was not found, sysprof is disabled")
> +
> +      # XXX: CMake sets those variables globally, so using the `unset` here

Nit: comment length is more than 66 symbols.

> +      # doesn't really clear them out of the parent scope. As stated in the
> +      # `unset` documentation, to force a variable reference of the form ${VAR}
> +      # to return an empty string, you need to use `set(<variable> "")`.
> +      set(LIBUNWIND_INCLUDE_DIR "" PARENT_SCOPE)
> +      set(LIBUNWIND_LIBRARIES "" PARENT_SCOPE)
> +      set(LIBUNWIND_FOUND FALSE PARENT_SCOPE)
> +    else()
> +      AppendFlags(CMAKE_C_FLAGS -fno-omit-frame-pointer -fasynchronous-unwind-tables)
> +    endif()
> +  endif()
>  endif()
>  
>  # Switch to harder (and slower) hash function when a collision
> diff --git a/cmake/FindLibUnwind.cmake b/cmake/FindLibUnwind.cmake
> new file mode 100644
> index 00000000..70221c12
> --- /dev/null
> +++ b/cmake/FindLibUnwind.cmake

<snipped>

> +if(BUILD_STATIC AND NOT APPLE)
> +    set(LIBUNWIND_LIBRARY_NAME libunwind.a)
> +else()
> +    # Only a dynamic version of libunwind is available on macOS: also, we

Nit: comment length is more than 66 symbols.

> +    # should link against the umbrella framework `System` — otherwise `ld` will

Non ascii charecter. ----------------------------------------^
What is "umberlla framework `System`"?

> +    # complain that it cannot link directly with libunwind.tbd.

What is libunwind.tbd?

> +    set(LIBUNWIND_LIBRARY_NAME System unwind)
> +endif()
> +find_library(LIBUNWIND_LIBRARY NAMES ${LIBUNWIND_LIBRARY_NAME}
> +             PATHS ${PC_LIBUNWIND_LIBRARY_DIRS})

<snipped>

> +if(BUILD_STATIC)
> +    # libunwind could have been built with liblzma dependency:

Do we need it? And why?

> +    # https://github.com/libunwind/libunwind/blob/4feb1152d1c4aaafbb2d504dbe34c6db5b6fe9f2/configure.ac#L302-L317
> +    pkg_check_modules(PC_LIBLZMA QUIET liblzma)
> +    find_library(LIBLZMA_LIBRARY liblzma.a ${PC_LIBLZMA_LIBRARY_DIRS})
> +    if(NOT LIBLZMA_LIBRARY STREQUAL "LIBLZMA_LIBRARY-NOTFOUND")
> +        message(STATUS "liblzma found")
> +        set(LIBUNWIND_LIBRARIES ${LIBUNWIND_LIBRARIES} ${LIBLZMA_LIBRARY})
> +    endif()
> +    # Ditto,
> +    # https://github.com/libunwind/libunwind/blob/4feb1152d1c4aaafbb2d504dbe34c6db5b6fe9f2/configure.ac#L319-L334

Ditto.

> +    set(LIBUNWIND_LIBRARIES ${LIBUNWIND_LIBRARIES} ZLIB::ZLIB)
> +endif()

<snipped>

> diff --git a/cmake/GetLibUnwindVersion.cmake b/cmake/GetLibUnwindVersion.cmake
> new file mode 100644
> index 00000000..af833478
> --- /dev/null
> +++ b/cmake/GetLibUnwindVersion.cmake

<snipped>

> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 1a3f106a..9e004a44 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt

<snipped>

> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
> index a4a70146..20604b90 100644
> --- a/src/lj_sysprof.c
> +++ b/src/lj_sysprof.c
> @@ -26,7 +26,9 @@
>  
>  #include <pthread.h>
>  #include <errno.h>
> -#include <execinfo.h>
> +

It is good to comment documentation here:
| We only need local unwinding, then a special implementation
| can be selected which may run much faster than the generic
| implementation which supports both kinds of unwinding, local and
| remote.

> +#define UNW_LOCAL_ONLY
> +#include <libunwind.h>
>  
>  /*
>  ** Number of profiler frames we need to omit during stack
> @@ -85,6 +87,34 @@ static struct sysprof sysprof = {0};
>  
>  /* --- Stream ------------------------------------------------------------- */
>  
> +static ssize_t collect_stack(void **buffer, int size)
> +{

<snipped>

> +  for (; frame_no < size; ++frame_no) {
> +    unw_word_t ip;
> +    rc = unw_get_reg(&unw_cur, UNW_REG_IP, &ip);
> +    if (rc != 0)
> +      return -1;
> +
> +    buffer[frame_no] = (void *)ip;
> +    rc = unw_step(&unw_cur);
> +    if (rc <= 0)
> +    break;

Something wrong with indent here.

> +  }
> +  return frame_no;
> +}
> +
>  static const uint8_t ljp_header[] = {'l', 'j', 'p', LJP_FORMAT_VERSION,
>                                        0x0, 0x0, 0x0};
>  
> @@ -197,10 +227,11 @@ static void default_backtrace_host(void *(writer)(int frame_no, void *addr))

<snipped>

> diff --git a/test/luajit-test-init.lua b/test/luajit-test-init.lua
> index acd3d60e..37606e49 100644
> --- a/test/luajit-test-init.lua
> +++ b/test/luajit-test-init.lua
> @@ -18,3 +18,44 @@ end
>  function _dofile(filename)
>    return dofile(path_to_sources..filename)
>  end
> +
> +-- XXX: Some tests need more complicated skipconds that can be implemented

Nit: comment length is more than 66 symbols.

> +-- in Lua, so this function checks if the test was marked as excluded via CLI
> +-- arg.
> +

<snipped>

> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index e4d7134a..7bb07251 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt

<snipped>

> diff --git a/test/tarantool-tests/misclib-sysprof-capi.test.lua b/test/tarantool-tests/misclib-sysprof-capi.test.lua
> index afb99cf2..a5c5b77d 100644
> --- a/test/tarantool-tests/misclib-sysprof-capi.test.lua
> +++ b/test/tarantool-tests/misclib-sysprof-capi.test.lua
> @@ -1,10 +1,11 @@
>  -- Sysprof is implemented for x86 and x64 architectures only.
>  local ffi = require("ffi")
> +-- luacheck: globals is_excluded

May be we should introduce is_excluded() mechanism, in the separate
commit?

Feel free to ignore.

>  require("utils").skipcond(
>    jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux"
> -    or ffi.abi("gc64"),
> +    or ffi.abi("gc64") or is_excluded(),

Should we delete all others conditions, as far as sysprof is disabled
for other arches?

>    jit.arch.." architecture or "..jit.os..
> -  " OS is NIY for sysprof"
> +  " OS is NIY for sysprof, or the test suite was excluded"
>  )
>  
>  local testsysprof = require("testsysprof")
> @@ -15,15 +16,11 @@ local jit = require('jit')

<skipped>

> diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> index 2f53d771..2e1fa2c9 100644
> --- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> @@ -1,10 +1,11 @@
>  -- Sysprof is implemented for x86 and x64 architectures only.
>  local ffi = require("ffi")
> +-- luacheck: globals is_excluded
>  require("utils").skipcond(
>    jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux"
> -    or ffi.abi("gc64"),
> +    or ffi.abi("gc64") or is_excluded(),

Ditto.

>    jit.arch.." architecture or "..jit.os..
> -  " OS is NIY for sysprof"
> +  " OS is NIY for sysprof, or the test suite was excluded"
>  )
>  
>  local tap = require("tap")
> -- 
> 2.36.1
> 

-- 
Best regards,
Sergey Kaplun