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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Max,

Thanks for the patch! Finally, I'm here :)

On 06.12.22, Maxim Kokryashkin via Tarantool-patches wrote:
> `backtrace` fails to unwind the host stack in LuaJIT, since there 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.

Well, I do not understand quite well this reasoning: if there is a
problem with frame pointers, so why do we need libunwind here? Just add
-fno-omit-frame-pointer and leave as is. Otherwise, do not touch the
compiler flags, since libunwin "provide robust unwinding without any
frame pointers". If none of the above is applicable, please provide
another rationale.

> 
> Also, this commit enables C API tests, which used to crash with
> `backtrace`.
> 
> The `lj-603-err-snap-restore.test.lua` was updated to correspond with
> the new size of the `arg` table.

I believe this part can be dropped when your another patchset is merged.

> 
> Part of tarantool/tarantool#781
> ---
> Changes in v2:
> - Fixed comments as per review by Sergey
> - Fixed build for Makefile.original
> - Moved `is_exluded` to `utils` module
> 
>  CMakeLists.txt                                | 48 ++++++++--
>  cmake/FindLibUnwind.cmake                     | 87 +++++++++++++++++++
>  cmake/GetLibUnwindVersion.cmake               | 12 +++
>  src/CMakeLists.txt                            |  4 +
>  src/Makefile.original                         | 10 +++
>  src/lj_sysprof.c                              | 57 +++++++++---
>  test/tarantool-tests/CMakeLists.txt           |  7 ++
>  ...4-add-proto-trace-sysprof-default.test.lua |  5 +-
>  .../lj-603-err-snap-restore.test.lua          |  2 +-
>  .../misclib-sysprof-capi.test.lua             | 14 ++-
>  .../misclib-sysprof-lapi.test.lua             |  7 +-
>  test/tarantool-tests/utils.lua                | 39 +++++++++
>  12 files changed, 259 insertions(+), 33 deletions(-)
>  create mode 100644 cmake/FindLibUnwind.cmake
>  create mode 100644 cmake/GetLibUnwindVersion.cmake
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index c870cce2..2ac32465 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -91,12 +91,9 @@ 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.
> -AppendFlags(CMAKE_C_FLAGS -fomit-frame-pointer -fno-stack-protector)
> +AppendFlags(CMAKE_C_FLAGS -fno-stack-protector)
> +# The '-fomit-frame-pointer` is set depending on sysprof
> +# and libunwind support.

Minor: I'd rather moved this part above the <AppendFlags>.

>  
>  # Redefined to benefit from expanding macros in gdb.
>  set(CMAKE_C_FLAGS_DEBUG "-g -ggdb3")
> @@ -195,6 +192,45 @@ 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
> +    (
> +      CMAKE_SYSTEM_PROCESSOR STREQUAL "i386" OR
> +      CMAKE_SYSTEM_PROCESSOR STREQUAL "i686" OR
> +      CMAKE_SYSTEM_PROCESSOR STREQUAL "x86_64"
> +    )
> +  )

Well, I doubt this should be here (but I completely understand why you
placed it nearby). My suggestion is to move this to FindLibUnwind.cmake,
or just remove and here is why:
* The only reason for this condition is *not* to search and *not* to
  link the unwinding library. But I see no problem in it in general: if
  you don't consider porting sysprof to other platforms, then simply
  move this condition to FindLibUnwind.cmake; otherwise, just find the
  library and other supplementaries. The linking flags can be tweaked
  there either, but I agree to leave them in the root CMakeLists.txt.
* Juggling with -fomit-frame-pointer can be done way more easily: add it
  unconditionally to C flags and *override* it via -fno-blah-blah where
  it's required (i.e. in FindLibUnwind.cmake when everything is OK or
  when -DLUAJIT_EXTERNAL_SYSPROF_BACKTRACER is set, since we have no
  information whether the aforementioned flag breaks its semantics).

As a result of this suggestion, I see this part much more clear and easy
to maintain in the future.

> +    # 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)
> +
> +      # 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.
> +      AppendFlags(CMAKE_C_FLAGS -fomit-frame-pointer)
> +      message(STATUS "Libunwind was not found, sysprof is disabled")

I think it should be a FATAL_ERROR here: if sysprof has to be enabled,
and libunwind is mandatory for this, you can't just disable sysprof.

> +
> +      # XXX: CMake sets those variables globally, so using the
> +      # `unset` here 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)

Side note: I didn't find this part on the branch. Maybe the new version
worth to be sent?

> +    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..fca0aaee
> --- /dev/null
> +++ b/cmake/FindLibUnwind.cmake

I get the idea, that this module is a carbon copy taken from
tarantool/tarantool repository, but I believe that it should follow
tarantool/luajit guidelines.

> @@ -0,0 +1,87 @@
> +#[========================================================================[.rst:
> +FindLibUnwind
> +--------
> +Finds the libunwind library.
> +
> +Result Variables
> +^^^^^^^^^^^^^^^^
> +``LIBUNWIND_FOUND``
> +  True if the system has the libunwind library.
> +``LIBUNWIND_VERSION``
> +  The version of the libunwind library which was found.
> +``LIBUNWIND_INCLUDE_DIR``
> +  Include directory needed to use libunwind.
> +``LIBUNWIND_LIBRARIES``
> +  Libraries needed to link to libunwind.
> +
> +Cache Variables
> +^^^^^^^^^^^^^^^
> +``LIBUNWIND_INCLUDE_DIR``
> +  The directory containing ``libunwind.h``.
> +``LIBUNWIND_LIBRARIES``
> +  The paths to the libunwind libraries.
> +#]========================================================================]
> +
> +include(FindPackageHandleStandardArgs)
> +include(GetLibUnwindVersion)
> +
> +find_package(PkgConfig QUIET)
> +pkg_check_modules(PC_LIBUNWIND QUIET libunwind)
> +
> +find_path(LIBUNWIND_INCLUDE_DIR libunwind.h ${PC_LIBUNWIND_INCLUDE_DIRS})
> +if(LIBUNWIND_INCLUDE_DIR)
> +    include_directories(${LIBUNWIND_INCLUDE_DIR})
> +endif()
> +
> +if(BUILD_STATIC AND NOT APPLE)

There is no BUILD_STATIC in tarantool/luajit. Suggest to remove it.

> +    set(LIBUNWIND_LIBRARY_NAME libunwind.a)
> +else()
> +    # Only a dynamic version of libunwind is available on macOS:
> +    # also, we should link against the umbrella framework
> +    # `System` - otherwise `ld` will complain that it cannot
> +    # link directly with libunwind.tbd.
> +    set(LIBUNWIND_LIBRARY_NAME System unwind)
> +endif()
> +find_library(LIBUNWIND_LIBRARY NAMES ${LIBUNWIND_LIBRARY_NAME}
> +             PATHS ${PC_LIBUNWIND_LIBRARY_DIRS})
> +
> +if(APPLE)
> +    set(LIBUNWIND_LIBRARIES ${LIBUNWIND_LIBRARY})
> +else()
> +    if(BUILD_STATIC)

Ditto.

> +        set(LIBUNWIND_PLATFORM_LIBRARY_NAME
> +            "libunwind-${CMAKE_SYSTEM_PROCESSOR}.a")
> +    else()
> +        set(LIBUNWIND_PLATFORM_LIBRARY_NAME
> +            "unwind-${CMAKE_SYSTEM_PROCESSOR}")
> +    endif()
> +    find_library(LIBUNWIND_PLATFORM_LIBRARY ${LIBUNWIND_PLATFORM_LIBRARY_NAME}
> +                 ${PC_LIBUNWIND_LIBRARY_DIRS})
> +    set(LIBUNWIND_LIBRARIES ${LIBUNWIND_LIBRARY} ${LIBUNWIND_PLATFORM_LIBRARY})
> +endif()
> +
> +if(BUILD_STATIC)

Ditto.

> +    # libunwind could have been built with liblzma dependency:
> +    # 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
> +    set(LIBUNWIND_LIBRARIES ${LIBUNWIND_LIBRARIES} ZLIB::ZLIB)
> +endif()
> +
> +if(PC_LIBUNWIND_VERSION)
> +    set(LIBUNWIND_VERSION ${PC_LIBUNWIND_VERSION})
> +else()
> +    GetLibUnwindVersion(LIBUNWIND_VERSION)

Maybe it's worth to inline this module right here? I see little sense in
supporting another tiny module just to find the libunwind version (I
even hardly believe we need this variable).

> +endif()
> +
> +find_package_handle_standard_args(LibUnwind
> +      VERSION_VAR LIBUNWIND_VERSION
> +      REQUIRED_VARS LIBUNWIND_INCLUDE_DIR LIBUNWIND_LIBRARIES)
> +
> +mark_as_advanced(LIBUNWIND_INCLUDE_DIR LIBUNWIND_LIBRARIES)

<snipped>

> diff --git a/src/Makefile.original b/src/Makefile.original
> index 593b310d..3c3ae7f2 100644
> --- a/src/Makefile.original
> +++ b/src/Makefile.original
> @@ -289,6 +289,16 @@ ifneq (,$(findstring LJ_TARGET_PS3 1,$(TARGET_TESTARCH)))
>    TARGET_XLIBS+= -lpthread
>  endif
>  
> +ifneq (,$(findstring LJ_HASSYSPROF ,$(TARGET_TESTARCH)))
> +  HAS_LIBUNWIND=$(shell $(TARGET_LD) -lunwind -lunwind-x86_64 -c 2>/dev/null; echo $$?)
> +ifneq (,$(HAS_LIBUNWIND))
> +  TARGET_XCFLAGS+= -DLUAJIT_DISABLE_SYSPROF
> +else
> +  TARGET_XLIBS+= -lunwind -lunwind-x86_64
> +  TARGET_XCFLAGS+= -fno-omit-frame-pointer -fasynchronous-unwind-tables
> +endif
> +endif

Side note: I want our CMake to be *so* clear! But comments are welcome.

> +
>  TARGET_XCFLAGS+= $(CCOPT_$(TARGET_LJARCH))
>  TARGET_ARCH+= $(patsubst %,-DLUAJIT_TARGET=LUAJIT_ARCH_%,$(TARGET_LJARCH))
>  
> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
> index 2e9ed9b3..4ccb03e8 100644
> --- a/src/lj_sysprof.c
> +++ b/src/lj_sysprof.c

<snipped>

> @@ -410,20 +447,12 @@ int lj_sysprof_set_backtracer(luam_Sysprof_backtracer backtracer) {
>  
>    if (sp->state != SPS_IDLE)
>      return PROFILE_ERRUSE;
> -  if (backtracer == NULL) {
> +
> +  if (backtracer == NULL)
>      sp->backtracer = default_backtrace_host;

Minor: Ternary operator looks more compact and clear (at least to me)
here. Feel free to ignore.

> -    /*
> -    ** XXX: `backtrace` is not signal-safe, according to man,
> -    ** because it is lazy loaded on the first call, which triggers
> -    ** allocations. We need to call `backtrace` before starting profiling
> -    ** to avoid lazy loading.
> -    */
> -    void *dummy = NULL;
> -    backtrace(&dummy, 1);
> -  }
> -  else {
> +  else
>      sp->backtracer = backtracer;
> -  }
> +
>    if (!is_unconfigured(sp)) {
>      sp->state = SPS_IDLE;
>    }
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index a428d009..af284300 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -94,6 +94,12 @@ set(LUA_TEST_ENV
>    "LUA_CPATH=\"${LUA_CPATH}\""
>  )
>  
> +if(LUAJIT_DISABLE_SYSPROF)
> +  string(CONCAT LUA_EXCLUDE_TESTS [[\{\"misclib-sysprof-lapi\",]]
> +                                  [[\"gh-7264-add-proto-trace-sysprof-default\",]]
> +                                  [[\"misclib-sysprof-capi\"\}]])
> +endif()

I will wait CTest patchset and group all sysprof-dependent tests in a
separate target, so CMake/CTest machinery is responsible for excluding
them, not test chunks. Otherwise, sliwkom slojnoe prisedanie.

> +
>  if(CMAKE_VERBOSE_MAKEFILE)
>    list(APPEND LUA_TEST_FLAGS --verbose)
>  endif()

<snipped>

> -- 
> 2.38.1
> 

-- 
Best regards,
IM