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 610CE6E6A27; Mon, 20 Nov 2023 12:41:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 610CE6E6A27 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1700473288; bh=psYf5WIfCBJR3O3h9uQX6J3gWl65iNGLNu/IvPiKcx4=; 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=ZUe2ZnUGQ1Gj5BBHdhovIqqlzqThvNCvoR1rFnFz1s4QdxERptlzMZ0KZJJyd/Xwa hUPAckwG8pGkYE+TFx5kCjattwHZNVRopbw2ROY6LjVNs7OjhtBrojAWNKiF/IV7W4 4RwZprBUW5QPShpWcNqttnATckwbqxdopD3fx3Vo= Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [95.163.41.78]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0DAF06E6777 for ; Mon, 20 Nov 2023 12:41:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0DAF06E6777 Received: by smtp37.i.mail.ru with esmtpa (envelope-from ) id 1r50mT-004Zdn-2H; Mon, 20 Nov 2023 12:41:26 +0300 Date: Mon, 20 Nov 2023 09:37:27 +0000 To: Maxim Kokryashkin Message-ID: References: <20221206064712.755124-1-m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20221206064712.755124-1-m.kokryashkin@tarantool.org> X-Clacks-Overhead: GNU Terry Pratchett X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD93F1575C7510F554723A238A7750C30189FE0704E88CB56E400894C459B0CD1B9913F6D8D09A888533AFF60C217C4330F1DF1B0227F4B21E1C94F5D657025D6F1 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73F300A97FDD4E158EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637F1FEB73A813C0D3B8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D85448C6004E9787F668A71A231B800C3D117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD182CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EECCD848CCB6FE560C148812EF9080FC94D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE398E93883770458359735652A29929C6CC4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947C989FD0BDF65E50FB2E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89FB26E97DCB74E6252156CCFE7AF13BCA4B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5C322C82A15ABFD7639F7429E990CDE34E4D9A925463ED545F87CCE6106E1FC07E67D4AC08A07B9B04CB6874B0BCFF0B8CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFA1F94CF013B5C021D16F145D983C419B8130C174240AEFAB289A025027C8C537669CEE92FE12193588AD3775A6B0C8340E96A23C7C9627FBB4ABABCCAED070BAA74DFFEFA5DC0E7F02C26D483E81D6BEECAEF3E2CCC1ED8C383653B6C8D9AE0FD16FCAA6493B703A X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojmRIjd71J0y34ydErxFW40g== X-Mailru-Sender: 2FEBA92C8E508479FE7B9A1DF348D531544DD1AD1B7A5BF81201AD1BDD1A0B76FBA59DB64A70E9EC2326FE6F2A341ACE0FB9F97486540B4CD9E8847AB8CFED4D9ABF8A61C016C2CFB0DAF586E7D11B3E67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v2] 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 . > > # 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( "")`. > + 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) > 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 > @@ -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() > -- > 2.38.1 > -- Best regards, IM