* [Tarantool-patches] [PATCH luajit] sysprof: replace `backtrace` with libunwind
@ 2022-06-11 22:40 Maxim Kokryashkin via Tarantool-patches
2022-11-29 11:02 ` Sergey Kaplun via Tarantool-patches
0 siblings, 1 reply; 4+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-06-11 22:40 UTC (permalink / raw)
To: tarantool-patches, imun, skaplun
`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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] sysprof: replace `backtrace` with libunwind
2022-06-11 22:40 [Tarantool-patches] [PATCH luajit] sysprof: replace `backtrace` with libunwind Maxim Kokryashkin via Tarantool-patches
@ 2022-11-29 11:02 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 4+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-11-29 11:02 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] sysprof: replace `backtrace` with libunwind
2022-05-18 12:31 ` [Tarantool-patches] [PATCH luajit] sysprof: replace `backtrace` with libunwind Maxim Kokryashkin via Tarantool-patches
@ 2022-05-26 16:33 ` Igor Munkin via Tarantool-patches
0 siblings, 0 replies; 4+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2022-05-26 16:33 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: tarantool-patches
Ignoring this patch, since we decided to resolve this in a separate
series.
--
Best regards,
IM
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Tarantool-patches] [PATCH luajit] sysprof: replace `backtrace` with libunwind
2022-05-18 12:31 [Tarantool-patches] [PATCH luajit] symtab: check the _GNU_SOURCE definition Maxim Kokryashkin via Tarantool-patches
@ 2022-05-18 12:31 ` Maxim Kokryashkin via Tarantool-patches
2022-05-26 16:33 ` Igor Munkin via Tarantool-patches
0 siblings, 1 reply; 4+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-05-18 12:31 UTC (permalink / raw)
To: tarantool-patches, imun, skaplun
`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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-29 11:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-11 22:40 [Tarantool-patches] [PATCH luajit] sysprof: replace `backtrace` with libunwind Maxim Kokryashkin via Tarantool-patches
2022-11-29 11:02 ` Sergey Kaplun via Tarantool-patches
-- strict thread matches above, loose matches on Subject: below --
2022-05-18 12:31 [Tarantool-patches] [PATCH luajit] symtab: check the _GNU_SOURCE definition Maxim Kokryashkin via Tarantool-patches
2022-05-18 12:31 ` [Tarantool-patches] [PATCH luajit] sysprof: replace `backtrace` with libunwind Maxim Kokryashkin via Tarantool-patches
2022-05-26 16:33 ` Igor Munkin via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox