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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

`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.

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.

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.
 
 # 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"
+    )
+  )
+    # 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")
+
+      # 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)
+    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
@@ -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)
+    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)
+        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)
+    # 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)
+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)
diff --git a/cmake/GetLibUnwindVersion.cmake b/cmake/GetLibUnwindVersion.cmake
new file mode 100644
index 00000000..af833478
--- /dev/null
+++ b/cmake/GetLibUnwindVersion.cmake
@@ -0,0 +1,12 @@
+function(GetLibUnwindVersion _LIBUNWIND_VERSION)
+    set(_LIBUNWIND_VERSION_HEADER "${LIBUNWIND_INCLUDE_DIR}/libunwind-common.h")
+    if(LIBUNWIND_LIBRARY AND EXISTS ${_LIBUNWIND_VERSION_HEADER})
+        file(READ ${_LIBUNWIND_VERSION_HEADER}
+             _LIBUNWIND_VERSION_HEADER_CONTENTS)
+        string(REGEX MATCH
+               "#define UNW_VERSION_MAJOR[ \t]+([0-9]+)\n#define UNW_VERSION_MINOR[ \t]+([0-9]+)"
+               _VERSION_REGEX "${_LIBUNWIND_VERSION_HEADER_CONTENTS}")
+        set(${_LIBUNWIND_VERSION} "${CMAKE_MATCH_1}.${CMAKE_MATCH_2}"
+            PARENT_SCOPE)
+    endif()
+endfunction()
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index dffc0a4d..50768236 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -283,6 +283,10 @@ add_dependencies(core_shared buildvm_output)
 
 list(APPEND TARGET_LIBS m)
 
+if(NOT LUAJIT_DISABLE_SYSPROF)
+  list(APPEND TARGET_LIBS ${LIBUNWIND_LIBRARIES})
+endif()
+
 set(LIB_OBJECTS_STATIC
   $<TARGET_OBJECTS:vm_static>
   $<TARGET_OBJECTS:core_static>
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
+
 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
@@ -26,7 +26,15 @@
 
 #include <pthread.h>
 #include <errno.h>
-#include <execinfo.h>
+
+/*
+** 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 +93,34 @@ static struct sysprof sysprof = {0};
 
 /* --- Stream ------------------------------------------------------------- */
 
+static ssize_t collect_stack(void **buffer, int size)
+{
+  int frame_no = 0;
+  unw_context_t unw_ctx;
+  unw_cursor_t unw_cur;
+
+  int rc = unw_getcontext(&unw_ctx);
+  if (rc != 0)
+    return -1;
+
+  rc = unw_init_local(&unw_cur, &unw_ctx);
+  if (rc != 0)
+    return -1;
+
+  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;
+  }
+  return frame_no;
+}
+
 static const uint8_t ljp_header[] = {'l', 'j', 'p', LJP_FORMAT_VERSION,
                                       0x0, 0x0, 0x0};
 
@@ -197,10 +233,11 @@ static void default_backtrace_host(void *(writer)(int frame_no, void *addr))
   int max_depth = sp->opt.mode == LUAM_SYSPROF_LEAF
                   ? SYSPROF_HANDLER_STACK_DEPTH + 1
                   : SYSPROF_BACKTRACE_FRAME_MAX;
-  const int depth = backtrace(backtrace_buf, max_depth);
+  const int depth = collect_stack(backtrace_buf, max_depth);
   int level;
 
   lua_assert(depth <= max_depth);
+  lua_assert(depth != -1);
   for (level = SYSPROF_HANDLER_STACK_DEPTH; level < depth; ++level) {
     if (!writer(level - SYSPROF_HANDLER_STACK_DEPTH + 1, backtrace_buf[level]))
       return;
@@ -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;
-    /*
-    ** 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()
+
 if(CMAKE_VERBOSE_MAKEFILE)
   list(APPEND LUA_TEST_FLAGS --verbose)
 endif()
@@ -146,6 +152,7 @@ add_custom_command(TARGET tarantool-tests
       --ext ${LUA_TEST_SUFFIX}
       --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
       ${LUA_TEST_FLAGS}
+      :: --exclude=${LUA_EXCLUDE_TESTS}
   WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
 )
 
diff --git a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
index 15bd0a8b..424a6454 100644
--- a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
+++ b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
@@ -1,7 +1,8 @@
 -- Sysprof is implemented for x86 and x64 architectures only.
-require('utils').skipcond(
+local utils = require('utils')
+utils.skipcond(
   jit.arch ~= 'x86' and jit.arch ~= 'x64' or jit.os ~= 'Linux'
-    or require('ffi').abi('gc64'),
+    or require('ffi').abi('gc64') or utils.is_excluded(arg),
   jit.arch..' architecture or '..jit.os..
   ' OS is NIY for sysprof'
 )
diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
index b5353e85..8e8d7db5 100644
--- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
+++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
@@ -15,7 +15,7 @@ test:plan(2)
 -- error handling"), etc.).
 -- This amount is suited well for GC64 and non-GC64 mode.
 -- luacheck: no unused
-local _, _, _, _, _, _
+local _, _, _, _, _
 
 local handler_is_called = false
 local recursive_f
diff --git a/test/tarantool-tests/misclib-sysprof-capi.test.lua b/test/tarantool-tests/misclib-sysprof-capi.test.lua
index dad0fe4a..0b669503 100644
--- a/test/tarantool-tests/misclib-sysprof-capi.test.lua
+++ b/test/tarantool-tests/misclib-sysprof-capi.test.lua
@@ -1,9 +1,12 @@
 -- Sysprof is implemented for x86 and x64 architectures only.
 local utils = require("utils")
+local ffi = require("ffi")
+-- luacheck: globals is_excluded
 utils.skipcond(
-  jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux",
+  jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux"
+    or ffi.abi("gc64") or utils.is_excluded(arg),
   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")
@@ -14,15 +17,11 @@ local jit = require('jit')
 jit.off()
 
 local test = tap.test("clib-misc-sysprof")
-test:plan(2)
+test:plan(4)
 
 test:ok(testsysprof.base())
 test:ok(testsysprof.validation())
 
--- FIXME: The following two tests are disabled because sometimes
--- `backtrace` dynamically loads a platform-specific unwinder, which is
--- not signal-safe.
---[[
 local function lua_payload(n)
   if n <= 1 then
     return n
@@ -55,5 +54,4 @@ jit.on()
 jit.flush()
 
 test:ok(testsysprof.profile_func(payload))
---]]
 os.exit(test:check() and 0 or 1)
diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
index 4bf10e8d..b1c5a377 100644
--- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
@@ -1,9 +1,12 @@
 -- Sysprof is implemented for x86 and x64 architectures only.
 local utils = require("utils")
+local ffi = require("ffi")
+-- luacheck: globals is_excluded
 utils.skipcond(
-  jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux",
+  jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux"
+    or ffi.abi("gc64") or utils.is_excluded(arg),
   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")
diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
index eb11d40d..bb6f1aa5 100644
--- a/test/tarantool-tests/utils.lua
+++ b/test/tarantool-tests/utils.lua
@@ -135,6 +135,45 @@ function M.profilename(name)
   return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern))
 end
 
+-- XXX: Some tests need more complicated skipconds that can be
+-- implemented in Lua, so this function checks if the test was
+-- marked as excluded via the CLI arg.
+function M.is_excluded(arg)
+  if #arg == 0 then
+    return false
+  end
+
+  local exclusions = nil
+  for i = 1, #arg do
+    local excl_arg = string.match(arg[i], '--exclude=({.+})')
+
+    if excl_arg == nil then
+      break
+    end
+
+    assert(exclusions == nil, '--exclude was already provided')
+
+    local excl_f, err = loadstring('return ' .. excl_arg)
+    assert(excl_f, err)
+
+    local excl = excl_f()
+    assert(type(excl) == 'table', '--exclude option must provide a valid array')
+    exclusions = excl
+  end
+
+  if exclusions == nil then
+    return false
+  end
+
+  local basename = string.match(arg[0], '[%w%-]+%.test%.lua')
+  for _, name in ipairs(exclusions) do
+    if basename == name .. '.test.lua' then
+      return true
+    end
+  end
+  return false
+end
+
 M.const = {
   -- XXX: Max nins is limited by max IRRef, that equals to
   -- REF_DROP - REF_BIAS. Unfortunately, these constants are not
-- 
2.38.1