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 6715DF00E45; Thu, 14 Nov 2024 17:05:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6715DF00E45 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1731593143; bh=U0B5Z3TXRGPw6LieiwIlP1C3GBH+eaV5zGz2KWQmHsU=; 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=Uv2X7jU8ILd/6JfS7wJ5M+/wUPi5fRj9ELwWD9pNMJ7SVVMptKpKPeMpocgG3RqZz dzQFLPO2XALvtyInyz2Kq8y920Vgxrusj2d68Jnj9Ysh/CQdq88gPxj7OORG0xXjqn U6wY/3d8zoRn/XlbyHZ2+eUahBmxlfE+OCQsfV10= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [95.163.41.83]) (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 22D5542080E for ; Thu, 14 Nov 2024 17:05:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 22D5542080E Received: by exim-smtp-5d7bbb79d6-pqrv9 with esmtpa (envelope-from ) id 1tBaTc-0000000087x-4Akj; Thu, 14 Nov 2024 17:05:41 +0300 Date: Thu, 14 Nov 2024 17:04:53 +0300 To: mandesero@gmail.com Message-ID: References: <41109126-36d0-4e5f-8d2c-25d3fc5ed6b7@tarantool.org> <20241111110342.7519-1-mandesero@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20241111110342.7519-1-mandesero@gmail.com> X-Mailru-Src: smtp X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9DF473FCC18249DD45E7C19F8404478A07DE803FB1270642400894C459B0CD1B9026075281BA919B2C7A4B249DE6549FD5601496AC5603EF88212515783ED3FE7750FC9643A608EC1 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7E2331B2371EFE129EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637279A5203CF71F5518638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D874EF5681A027965066AF38A00688AE6B73CE0207D735B53BCC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637F3E38EE449E3E2AE389733CBF5DBD5E9B5C8C57E37DE458B9E9CE733340B9D5F3BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735B58781B77DE60D36C4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5C3D160D3431EEF115002B1117B3ED69682C1EE7890478F16108A05421C070DB8823CB91A9FED034534781492E4B8EEADDFC043C56F70D752BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF77DD89D51EBB7742D3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF27F35BD5A52F29884F87B675D74FDD48FAA8A164CD7AD7452730B40407BF65BE2B6ADA43BC89BCA9D91475F11186E200C1B56D8B4B07A870CD72F4DFF6D4FD3BB7C207060CE5E1D1C226CC413062362A913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojUjlJs7fo7Wm5naEx1GC/lg== X-DA7885C5: 688A3FC3E0B2D17DF255D290C0D534F9FC0E71B4E5BD412B7ED8858ED40D3C25155012DF223F23E15B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F739381B31377CF4CA219CE9338F80FA8865DE62FB0B1951F21DC98F611FDB1D7B532E49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v3 luajit 1/2] cmake: run tests with Valgrind 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, Maksim! Thanks for the patch! Please consider my comments below. On 11.11.24, mandesero@gmail.com wrote: > From: Maksim Tiushev > > This patch enables running tests with Valgrind. Custom Valgrind testing > options can be configured by setting the `VALGRIND_OPTIONS` variable. > Please note that this environment variable must be set before building, > and any updates to it will require a project rebuild. By default, the > suppression file is set to `src/lj.supp`. There is a `VALGRIND_OPTS` variable [1] that we can set -- it makes the usage of valgrind more flexible -- we can define any necessary flags in the command line (not at the building stage). > > Also this patch disables the following tests when running with Valgrind Typo: s/Also/Also,/ > due to failures: > > Disabled due #10803: Typo: s<#10803> > - test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua Nit: Commit line width is more than 72 symbols. > - test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua > - test/tarantool-tests/lj-726-profile-flush-close.test.lua > - test/tarantool-tests/misclib-sysprof-lapi.test.lua > > Timed out due to running under Valgrind: > - test/tarantool-tests/gh-7745-oom-on-trace.test.lua > - test/tarantool-tests/lj-1034-tabov-error-frame.test.lua > > Part of #3705 Typo: s<#3705> > --- > CMakeLists.txt | 5 +++++ > test/CMakeLists.txt | 16 ++++++++++++++++ > test/tarantool-tests/CMakeLists.txt | 3 ++- > .../gh-7745-oom-on-trace.test.lua | 1 + > .../lj-1034-tabov-error-frame.test.lua | 1 + > .../lj-512-profiler-hook-finalizers.test.lua | 5 ++++- > .../lj-726-profile-flush-close.test.lua | 5 ++++- > ...7264-add-proto-trace-sysprof-default.test.lua | 2 ++ > .../profilers/misclib-sysprof-lapi.test.lua | 2 ++ > 9 files changed, 37 insertions(+), 3 deletions(-) > > diff --git a/CMakeLists.txt b/CMakeLists.txt > index aa2103b3..854b3613 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index 0db2dd8b..3cc0ddbc 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -69,6 +69,22 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS > ) > > set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]") > + > +if(LUAJIT_USE_VALGRIND) > + if (NOT LUAJIT_USE_SYSMALLOC) > + message(WARNING > + "LUAJIT_USE_SYSMALLOC option is mandatory for Valgrind's memcheck tool" > + " on x64 and the only way to get useful results from it for all other" > + " architectures.") > + endif() > + > + find_program(VALGRIND valgrind) > + set(LUAJIT_TEST_VALGRIND_OPTS > + "--suppressions=${LUAJIT_SOURCE_DIR}/lj.supp $ENV{VALGRIND_OPTIONS}") I suppose that we can drop $ENV{VALGRIND_OPTIONS}, since there is a `VALGRIND_OPTS` variable [1] that we can set -- it makes the usage of valgrind more flexible -- we can define any necessary flags in the command line (not at the building stage). Therefore, `LUAJIT_TEST_VALGRIND_OPTS` may be renamed to `LUAJIT_TEST_VALGRIND_SUPP`. > + set(LUAJIT_TEST_COMMAND > + "${VALGRIND} ${LUAJIT_TEST_VALGRIND_OPTS} ${LUAJIT_TEST_COMMAND}") > +endif() > + > separate_arguments(LUAJIT_TEST_COMMAND) > > set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake") > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt > index 8bdb2cf3..39562f35 100644 > --- a/test/tarantool-tests/CMakeLists.txt > +++ b/test/tarantool-tests/CMakeLists.txt > @@ -110,7 +110,8 @@ foreach(test_path ${tests}) > # LUA_CPATH and LD_LIBRARY_PATH variables and also > # dependencies list with libraries are set in scope of > # BuildTestLib macro. > - ENVIRONMENT "LUA_PATH=${LUA_PATH};${LUA_TEST_ENV_MORE}" > + ENVIRONMENT "LUA_PATH=${LUA_PATH};${LUA_TEST_ENV_MORE};\ > +LJ_USE_VALGRIND=${LUAJIT_USE_VALGRIND}" It is better to set this variable to the `LUA_TEST_ENV_MORE` list. Also, I suggest rename it to the `LUAJIT_TEST_USE_VALGRIND`. > LABELS ${TEST_SUITE_NAME} > DEPENDS tarantool-tests-deps > ) > diff --git a/test/tarantool-tests/gh-7745-oom-on-trace.test.lua b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua > index fe320251..b4ab8872 100644 > --- a/test/tarantool-tests/gh-7745-oom-on-trace.test.lua > +++ b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua > @@ -6,6 +6,7 @@ local test = tap.test('OOM on trace'):skipcond({ > (jit.os == 'OSX'), > ['Disabled on MacOS due to #8652'] = jit.os == 'OSX', > ['Test requires JIT enabled'] = not jit.status(), > + ['Disabled with Valgrind (Timeout)'] = os.getenv("LJ_USE_VALGRIND") == 'ON', Minor: there is no need to check the custom value. If the variable isn't set `os.getenv()` returns `nil`. > }) > > test:plan(1) > diff --git a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua > index 0e23fdb2..5e75973b 100644 > --- a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua > +++ b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua > @@ -4,6 +4,7 @@ local test = tap.test('lj-1034-tabov-error-frame'):skipcond({ > ['Test requires JIT enabled'] = not jit.status(), > ['Test requires GC64 mode enabled'] = not ffi.abi('gc64'), > ['Disabled on MacOS due to #8652'] = jit.os == 'OSX', > + ['Disabled with Valgrind (Timeout)'] = os.getenv("LJ_USE_VALGRIND") == 'ON', Minor: there is no need to check the custom value. If the variable isn't set `os.getenv()` returns `nil`. > }) > > -- XXX: The test for the problem uses the table of GC > diff --git a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua > index f02bd05f..8f32b9ef 100644 > --- a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua > +++ b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua > @@ -1,7 +1,10 @@ > local tap = require('tap') > local profile = require('jit.profile') > > -local test = tap.test('lj-512-profiler-hook-finalizers') > +local test = tap.test('lj-512-profiler-hook-finalizers'):skipcond({ > + -- See also https://github.com/tarantool/tarantool/issues/10803 Nit: Missed dot at the end of the sentence. > + ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON', Minor: there is no need to check the custom value. If the variable isn't set `os.getenv()` returns `nil`. > +}) > test:plan(1) > > -- Sampling interval in ms. > diff --git a/test/tarantool-tests/lj-726-profile-flush-close.test.lua b/test/tarantool-tests/lj-726-profile-flush-close.test.lua > index 36cca43d..27eb3751 100644 > --- a/test/tarantool-tests/lj-726-profile-flush-close.test.lua > +++ b/test/tarantool-tests/lj-726-profile-flush-close.test.lua > @@ -1,6 +1,9 @@ > local tap = require('tap') > > -local test = tap.test('lj-726-profile-flush-close') > +local test = tap.test('lj-726-profile-flush-close'):skipcond({ > + -- See also https://github.com/tarantool/tarantool/issues/10803 Nit: Missed dot at the end of the sentence. > + ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON', Minor: there is no need to check the custom value. If the variable isn't set `os.getenv()` returns `nil`. > +}) > test:plan(1) > > local TEST_FILE = 'lj-726-profile-flush-close.profile' > diff --git a/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua > index 176c1a15..e6c853a1 100644 > --- a/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua > +++ b/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua > @@ -6,6 +6,8 @@ local test = tap.test('gh-7264-add-proto-trace-sysprof-default'):skipcond({ > ['Sysprof is implemented for Linux only'] = jit.os ~= 'Linux', > -- See also https://github.com/LuaJIT/LuaJIT/issues/606. > ['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'), > + -- See also https://github.com/tarantool/tarantool/issues/10803 Nit: Missed dot at the end of the sentence. > + ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON', Minor: there is no need to check the custom value. If the variable isn't set `os.getenv()` returns `nil`. > }) > > test:plan(2) > diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua > index 26c277cd..2210d668 100644 > --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua > +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua > @@ -5,6 +5,8 @@ local test = tap.test("misc-sysprof-lapi"):skipcond({ > ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux", > -- See also https://github.com/LuaJIT/LuaJIT/issues/606. > ["Disabled due to LuaJIT/LuaJIT#606"] = os.getenv("LUAJIT_TABLE_BUMP"), > Nit: Missed dot at the end of the sentence. + -- See also https://github.com/tarantool/tarantool/issues/10803 > + ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON', Minor: there is no need to check the custom value. If the variable isn't set `os.getenv()` returns `nil`. > }) > > test:plan(19) > -- > 2.34.1 > [1]: https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts -- Best regards, Sergey Kaplun