Now the `_GNU_SOURCE` in `lj_symtab.c` is only defined when it wasn't defined yet. That chnage fixes the macro redifinition warning. Part of tarantool/tarantool#5813 --- Branch: https://github.com/tarantool/luajit/tree/fckxorg/sysprof-rc-full-ci src/lj_symtab.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/lj_symtab.c b/src/lj_symtab.c index 57045765..2b2b205b 100644 --- a/src/lj_symtab.c +++ b/src/lj_symtab.c @@ -7,7 +7,10 @@ #define lj_symtab_c #define LUA_CORE + +#ifndef _GNU_SOURCE #define _GNU_SOURCE +#endif #include "lj_symtab.h" -- 2.36.1
`backtrace` fails to unwind the host stack in LuaJIT, since there is 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-rc-full-ci CMakeLists.txt | 20 +++-- cmake/FindLibUnwind.cmake | 86 +++++++++++++++++++ cmake/GetLibUnwindVersion.cmake | 12 +++ src/CMakeLists.txt | 17 ++++ src/lj_sysprof.c | 51 ++++++++--- .../misclib-sysprof-capi.test.lua | 7 +- 6 files changed, 167 insertions(+), 26 deletions(-) create mode 100644 cmake/FindLibUnwind.cmake create mode 100644 cmake/GetLibUnwindVersion.cmake diff --git a/CMakeLists.txt b/CMakeLists.txt index 659ee879..63db42c2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -89,12 +89,20 @@ 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) + +if(NOT LUAJIT_DISABLE_SYSPROF AND + CMAKE_SYSTEM_NAME STREQUAL "Linux" AND + ( + CMAKE_SYSTEM_PROCESSOR STREQUAL "i386" OR + CMAKE_SYSTEM_PROCESSOR STREQUAL "i686" OR + CMAKE_SYSTEM_PROCESSOR STREQUAL "x86_64" + ) + ) + AppendFlags(CMAKE_C_FLAGS -fno-omit-frame-pointer -fasynchronous-unwind-tables) +else() + AppendFlags(CMAKE_C_FLAGS -fomit-frame-pointer) +endif() # Redefined to benefit from expanding macros in gdb. set(CMAKE_C_FLAGS_DEBUG "-g -ggdb3") diff --git a/cmake/FindLibUnwind.cmake b/cmake/FindLibUnwind.cmake new file mode 100644 index 00000000..70221c12 --- /dev/null +++ b/cmake/FindLibUnwind.cmake @@ -0,0 +1,86 @@ +#[========================================================================[.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 1a3f106a..4f9ad1de 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -283,6 +283,23 @@ add_dependencies(core_shared buildvm_output) list(APPEND TARGET_LIBS m) +if(NOT LUAJIT_DISABLE_SYSPROF AND + CMAKE_SYSTEM_NAME STREQUAL "Linux" AND + ( + CMAKE_SYSTEM_PROCESSOR STREQUAL "i386" OR + CMAKE_SYSTEM_PROCESSOR STREQUAL "i686" OR + CMAKE_SYSTEM_PROCESSOR STREQUAL "x86_64" + ) + ) + find_package(LibUnwind MODULE REQUIRED) + + if(NOT LIBUNWIND_FOUND) + message(FATAL_ERROR "Libunwind is required for build with sysprof") + else() + list(APPEND TARGET_LIBS ${LIBUNWIND_LIBRARIES}) + endif() +endif() + set(LIB_OBJECTS_STATIC $<TARGET_OBJECTS:vm_static> $<TARGET_OBJECTS:core_static> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c index 55cea203..04509f68 100644 --- a/src/lj_sysprof.c +++ b/src/lj_sysprof.c @@ -26,7 +26,9 @@ #include <pthread.h> #include <errno.h> -#include <execinfo.h> + +#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) +{ + 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 +227,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 +441,12 @@ int lj_sysprof_set_backtracer(sp_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/misclib-sysprof-capi.test.lua b/test/tarantool-tests/misclib-sysprof-capi.test.lua index afb99cf2..1117ff73 100644 --- a/test/tarantool-tests/misclib-sysprof-capi.test.lua +++ b/test/tarantool-tests/misclib-sysprof-capi.test.lua @@ -15,15 +15,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 @@ -56,5 +52,4 @@ jit.on() jit.flush() test:ok(testsysprof.profile_func(payload)) ---]] os.exit(test:check() and 0 or 1) -- 2.36.1
Max, Thanks for the patch! LGTM as trivial. On 18.05.22, Maxim Kokryashkin wrote: > Now the `_GNU_SOURCE` in `lj_symtab.c` is only defined when it wasn't > defined yet. That chnage fixes the macro redifinition warning. Typo: s/chnage/change/. > > Part of tarantool/tarantool#5813 > --- > Branch: https://github.com/tarantool/luajit/tree/fckxorg/sysprof-rc-full-ci > > src/lj_symtab.c | 3 +++ > 1 file changed, 3 insertions(+) > <snipped> > -- > 2.36.1 > -- Best regards, IM
Hi, Maxim! Thanks for the patch! LGTM. On 18.05.22, Maxim Kokryashkin wrote: > Now the `_GNU_SOURCE` in `lj_symtab.c` is only defined when it wasn't > defined yet. That chnage fixes the macro redifinition warning. Typo: s/chnage/change/ > > Part of tarantool/tarantool#5813 > --- <snipped> -- Best regards, Sergey Kaplun
Max, I've checked the patchset into tarantool branch in tarantool/luajit and bumped a new version in master. On 18.05.22, Maxim Kokryashkin wrote: > Now the `_GNU_SOURCE` in `lj_symtab.c` is only defined when it wasn't > defined yet. That chnage fixes the macro redifinition warning. > > Part of tarantool/tarantool#5813 > --- > Branch: https://github.com/tarantool/luajit/tree/fckxorg/sysprof-rc-full-ci > > src/lj_symtab.c | 3 +++ > 1 file changed, 3 insertions(+) > <snipped> > -- > 2.36.1 > -- Best regards, IM
Ignoring this patch, since we decided to resolve this in a separate series. -- Best regards, IM
`backtrace` fails to unwind the host stack in LuaJIT, since there is 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. -AppendFlags(CMAKE_C_FLAGS -fomit-frame-pointer -fno-stack-protector) +AppendFlags(CMAKE_C_FLAGS -fno-stack-protector) # 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 + ( + 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) + 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..70221c12 --- /dev/null +++ b/cmake/FindLibUnwind.cmake @@ -0,0 +1,86 @@ +#[========================================================================[.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 1a3f106a..9e004a44 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/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> + +#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) +{ + 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 +227,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 +441,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/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 +-- in Lua, so this function checks if the test was marked as excluded via CLI +-- arg. + +-- luacheck: no global + function is_excluded() + 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 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 @@ -84,6 +84,11 @@ set(LUA_TEST_ENV "LUA_CPATH=\"${LUA_CPATH}\;\;\"" ) +if(LUAJIT_DISABLE_SYSPROF) + string(CONCAT LUA_EXCLUDE_TESTS [[\{\"misclib-sysprof-lapi\",]] + [[\"misclib-sysprof-capi\"\}]]) +endif() + if(CMAKE_VERBOSE_MAKEFILE) list(APPEND LUA_TEST_FLAGS --verbose) endif() @@ -132,6 +137,7 @@ add_custom_command(TARGET tarantool-tests --exec '${LUAJIT_TEST_COMMAND}' --ext ${LUA_TEST_SUFFIX} ${LUA_TEST_FLAGS} + :: --exclude=${LUA_EXCLUDE_TESTS} WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} ) 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 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(), 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') 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 @@ -56,5 +53,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 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(), 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
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