From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id BDDE712FD22; Tue, 29 Nov 2022 14:05:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BDDE712FD22 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1669719943; bh=YBzu0evAeqb+NkkoU73vxTpuHf5cU6SY0ZAH0t8CbVc=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=rizwjmZnuogEX1zdJRWAgtRZmd7DKvUCbWQtvAeCLy6zuA676WT//zw1za1NTRbNz IXVDlXg769iN3H7uiDhgr7gJ2f0I3pfqkFFEwrMtVGrs7XIyRZKNwmS+9HBbUpTmeI HfevhbCj5qfFylFre+76P7nMwiuxUy2CjQ6yJWFQ= Received: from smtp30.i.mail.ru (smtp30.i.mail.ru [95.163.41.71]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 59C1963EB2 for ; Tue, 29 Nov 2022 14:05:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 59C1963EB2 Received: by smtp30.i.mail.ru with esmtpa (envelope-from ) id 1ozyQn-00Ars0-5N; Tue, 29 Nov 2022 14:05:41 +0300 Date: Tue, 29 Nov 2022 14:02:30 +0300 To: Maxim Kokryashkin Message-ID: References: <20220611224054.379999-1-m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220611224054.379999-1-m.kokryashkin@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9288A4F1FFA5F9905C03554E5B8674B9A7E09D1799A395A75182A05F5380850404C228DA9ACA6FE27823B1B0EC6D21B982874EAA70A43A10C8CF56CF9ABD0E6D10F7409BF266CDD5E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE71EAE3A63419E5AEDEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D919B403F35891598638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D80DFDBCD8E334465CC72EE53FA16DBD56117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735200AC5B80A05675ACDF04B652EEC242312D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE902A1BE408319B29C0837EA9F3D197644AD6D5ED66289B52698AB9A7B718F8C46E0066C2D8992A16725E5C173C3A84C3138A1EA7309DD5C2BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7BEE702378D8A34C7731C566533BA786AA5CC5B56E945C8DA X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3457FA942CB4462B4C3F160CFC0492D6653E4012033D353B733C3A4C21961C60E652CAAC2A5498E7AC1D7E09C32AA3244C5702256E6B4C363526D154DE0D8BC76669B6CAE0477E908D927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojLCIl0w9hXVzFNnJVXpyKsA== X-Mailru-Sender: 87D5297B137A96FE2CF18BD2B1767928A85FCC4E5F00120CC3A46B37D37BFB92A69E3B846B6A5E16525762887713E5F1475755348978188EF9D3679FA3DE6E791CC59163FFD68303B0DAF586E7D11B3E67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] sysprof: replace `backtrace` with libunwind X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 or something named like that. 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( "")`. > + 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 > +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}) > +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() > diff --git a/cmake/GetLibUnwindVersion.cmake b/cmake/GetLibUnwindVersion.cmake > new file mode 100644 > index 00000000..af833478 > --- /dev/null > +++ b/cmake/GetLibUnwindVersion.cmake > diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt > index 1a3f106a..9e004a44 100644 > --- a/src/CMakeLists.txt > +++ b/src/CMakeLists.txt > 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 > #include > -#include > + 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 > > /* > ** 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) > +{ > + 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)) > 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. > + > 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 > 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') > 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