* Re: [Tarantool-patches] [PATCH luajit v2] sysprof: replace `backtrace` with libunwind
2022-12-06 6:47 [Tarantool-patches] [PATCH luajit v2] sysprof: replace `backtrace` with libunwind Maxim Kokryashkin via Tarantool-patches
@ 2022-12-06 6:58 ` Maxim Kokryashkin via Tarantool-patches
2023-11-20 9:37 ` Igor Munkin via Tarantool-patches
1 sibling, 0 replies; 3+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-12-06 6:58 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 23520 bytes --]
>> -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.
Not that much. I've tested it with the following script:
=================================================================
local function payload()
local function fib(n)
if n <= 1 then
return n
end
return fib(n - 1) + fib(n - 2)
end
for i = 1, 1e3 do fib(32) end
end
local function generate_output(opts)
--local res, err = misc.sysprof.start(opts)
--assert(res, err)
payload()
--res,err = misc.sysprof.stop()
--assert(res, err)
end
local function check_mode(mode, interval)
local res = pcall(
generate_output,
{ mode = mode, interval = interval, path = 'sysprof.bin' }
)
end
check_mode('C', 1)
=================================================================
With FP & sysprof running:
./src/luajit ../test.lua 19,12s user 0,07s system 99% cpu 19,225 total
./src/luajit ../test.lua 20,03s user 0,08s system 99% cpu 20,129 total
./src/luajit ../test.lua 19,82s user 0,09s system 99% cpu 19,927 total
With FP & sysprof disabled:
./src/luajit ../test.lua 18,08s user 0,00s system 99% cpu 18,087 total
./src/luajit ../test.lua 17,89s user 0,00s system 99% cpu 17,904 total
./src/luajit ../test.lua 18,04s user 0,00s system 99% cpu 18,055 total
Without FP & sysprof disabled:
./src/luajit ../test.lua 17,90s user 0,00s system 99% cpu 17,902 total
./src/luajit ../test.lua 19,02s user 0,00s system 99% cpu 19,031 total
./src/luajit ../test.lua 17,80s user 0,00s system 99% cpu 17,810 total
>> @@ -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()
>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.
Moving it to a separate module breaks the PARENT_SCOPE cleanup,
which is vital for builds inside Tarantool.
>
> diff --git a/cmake/FindLibUnwind.cmake b/cmake/FindLibUnwind.cmake
> new file mode 100644
> index 00000000..70221c12
> --- /dev/null
> +++ b/cmake/FindLibUnwind.cmake
> +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
>What is "umberlla framework `System`"?
https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/FrameworkAnatomy.html
>> + # complain that it cannot link directly with libunwind.tbd.
>What is libunwind.tbd?
A TBD file is a text-based file used by Apple. It contains
information about a .DYLIB library, the location of the DYLIB
library, and symbols.
>> +if(BUILD_STATIC)
>> + # libunwind could have been built with liblzma dependency:
>
>Do we need it? And why?
I took that potion of CMake verbatim from the tarantool/tarantool repo,
but I see no issues with that part whatsoever. There is no guarantees
about the format of unwind info in external dynamic libraries and their
symbol tables can be lzma-compressed.
>> 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(),
>
>Should we delete all others conditions, as far as sysprof is disabled
for other arches?
IMO, it is better to leave it as it is for clarity reasons.
Otherwise, those changes are to cause a lot of headache during
the test system reorganization that we are willing to do.
--
Best regards,
Maxim Kokryashkin
>Вторник, 6 декабря 2022, 9:47 +03:00 от Maxim Kokryashkin <max.kokryashkin@gmail.com>:
>
>`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
[-- Attachment #2: Type: text/html, Size: 28766 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Tarantool-patches] [PATCH luajit v2] sysprof: replace `backtrace` with libunwind
2022-12-06 6:47 [Tarantool-patches] [PATCH luajit v2] sysprof: replace `backtrace` with libunwind Maxim Kokryashkin via Tarantool-patches
2022-12-06 6:58 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-11-20 9:37 ` Igor Munkin via Tarantool-patches
1 sibling, 0 replies; 3+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-11-20 9:37 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 3+ messages in thread