<HTML><BODY>Igor,<br><br>Thanks for the review, I've made all the changes as you suggested.<br><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Среда, 15 января 2020, 2:26 +03:00 от Igor Munkin <imun@tarantool.org>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15790443702091472626_BODY">Sasha,<br><br>
Thanks for the patch! I left some notes below, please consider them.<br><br>
On 21.11.19, Alexander V. Tikhonov wrote:<br><br>
> gitlab-ci: ASAN testing memory leaks add<br>
IMHO the commit subject should be adjusted as the following:<br>
| gitlab-ci: add memory leaks detection via LSAN<br><br>
> Added memory leaks detection to ASAN testing. Added files for exceptions:<br>
> - address sanitizer on compilation: asan/ignore_asan.txt<br>
> - memory leak sanitizer on run-time: asan/ignore_lsan.txt<br>
> Cmake updated with asan/ignore_asan.txt file added and test run rules<br>
> changed with asan/ignore_lsan.txt file. Added 'engine' and replication'<br>
> suites into the testing and blocked 'box/on_shutdown.test.lua' test<br>
> that breaks the testing, all of excepted tests will be enabled durring<br>
> issue #4360.<br>
> <br>
> Close #2058<br><br>
I propose to reword the commit message as the following:<br>
| The change enables memory leaks detection to existing ASAN testing<br>
| routine and introduces suppression files with the corresponding<br>
| exception list:<br>
| * address sanitizer for compile-time: asan/asan.supp<br>
| * memory leak sanitizer for run-time: asan/lsan.supp<br>
|<br>
| Furthermore, added engine and replication suites for ASAN testing<br>
| routine.<br>
|<br>
| Additionally to the tests blacklisted within #4359,<br>
| 'box/on_shutdown.test.lua' is also disabled since it fails the<br>
| introduced leak check. All blacklisted tests have to be enabled<br>
| within #4360.<br>
|<br>
| Closes #2058<br><br>
Feel free to adjust it on your own way.</div></div></div></div></blockquote>Rewrote as you suggested.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15790443702091472626_BODY"><br><br>
> ---<br>
> .travis.mk | 13 +++--<br>
> asan/ignore_asan.txt | 17 ++++++<br>
> asan/ignore_lsan.txt | 105 ++++++++++++++++++++++++++++++++++<br>
> cmake/profile.cmake | 4 +-<br>
> test/box/on_shutdown.skipcond | 7 +++<br>
> 5 files changed, 138 insertions(+), 8 deletions(-)<br>
> create mode 100644 asan/ignore_asan.txt<br>
> create mode 100644 asan/ignore_lsan.txt<br>
> create mode 100644 test/box/on_shutdown.skipcond<br>
> <br>
> diff --git a/.travis.mk b/.travis.mk<br>
> index 42969ff56..c6bc82d41 100644<br>
> --- a/.travis.mk<br>
> +++ b/.travis.mk<br>
> @@ -111,15 +111,16 @@ coverage_debian: deps_debian test_coverage_debian_no_deps<br>
> # ASAN<br>
> <br>
> build_asan_debian:<br>
> - CC=clang-8 CXX=clang++-8 cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}<br>
> - CC=clang-8 CXX=clang++-8 cmake . -DENABLE_ASAN=ON ${CMAKE_EXTRA_PARAMS}<br><br>
Side note: have no idea why it had been divided into separate commands.</div></div></div></div></blockquote><p>Previously it had some strange issues that were not reproduced for now,<br>so the command changed as it should be from the very start.</p><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15790443702091472626_BODY"><br><br>
> + CC=clang-8 CXX=clang++-8 cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON -DENABLE_ASAN=ON ${CMAKE_EXTRA_PARAMS}<br>
> make -j<br>
> <br>
> test_asan_debian_no_deps: build_asan_debian<br>
> - # temporary excluded engine/ and replication/ suites with some tests from other suites by issue #4360<br>
> - cd test && ASAN=ON ASAN_OPTIONS=detect_leaks=0 ./test-run.py --force $(TEST_RUN_EXTRA_PARAMS) \<br>
> - app/ app-tap/ box/ box-py/ box-tap/ engine_long/ long_run-py/ luajit-tap/ \<br>
> - replication-py/ small/ sql/ sql-tap/ swim/ unit/ vinyl/ wal_off/ xlog/ xlog-py/<br>
> + # temporary excluded some tests by issue #4360<br>
> + # ASAN environment flag shows the excluded tests to skip the testing<br><br>
As discussed offline, please drop a comment related to the difference in<br>
applying suppressions for LSAN and ASAN.</div></div></div></div></blockquote>Added comments about ASAN/LSAN excluding tests files at build/run times.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15790443702091472626_BODY"><br><br>
> + cd test && ASAN=ON \<br>
> + LSAN_OPTIONS=suppressions=${PWD}/asan/ignore_lsan.txt \<br>
> + ASAN_OPTIONS=heap_profile=0:unmap_shadow_on_exit=1:detect_invalid_pointer_pairs=1:symbolize=1:detect_leaks=1:dump_instruction_bytes=1:print_suppressions=0 \<br>
> + ./test-run.py --force $(TEST_RUN_EXTRA_PARAMS)<br>
> <br>
> test_asan_debian: deps_debian deps_buster_clang_8 test_asan_debian_no_deps<br>
> <br><br>
I propose to name suppresion lists in more traditional for such tools<br>
way:<br>
* s/ignore_asan.txt/asan.supp/<br>
* s/ignore_lsan.txt/lsan.supp/</div></div></div></div></blockquote>Renamed.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15790443702091472626_BODY"><br><br>
> diff --git a/asan/ignore_asan.txt b/asan/ignore_asan.txt<br>
> new file mode 100644<br>
> index 000000000..2a20114f4<br>
> --- /dev/null<br>
> +++ b/asan/ignore_asan.txt<br>
> @@ -0,0 +1,17 @@<br>
> +# File format:<br>
> +#fun:*<br>
> +#src:*<br>
> +<br>
> +# !test: app-tap/json.test.lua<br>
> +# source: third_party/lua-cjson/lua_cjson.c<br>
> +fun:json_decode<br>
> +<br>
> +# test: unit/base64.test.lua<br>
> +# source: third_party/base64.c<br>
> +fun:base64_decode_block<br>
> +# source: test/unit/base64.c<br>
> +fun:base64_test<br>
> +<br>
> +# !test: unit/msgpack.test<br>
> +# source: src/lib/msgpuck/test/msgpuck.c<br>
> +fun:test_mp_print<br>
> diff --git a/asan/ignore_lsan.txt b/asan/ignore_lsan.txt<br>
> new file mode 100644<br>
> index 000000000..a1237fb86<br>
> --- /dev/null<br>
> +++ b/asan/ignore_lsan.txt<br>
> @@ -0,0 +1,105 @@<br>
> +# File format:<br>
> +#leak:*<br>
> +<br>
> +# test: app/crypto.test.lua<br>
> +# source: /usr/lib/x86_64-linux-gnu/libcrypto.so<br>
> +leak:CRYPTO_zalloc<br>
> +<br>
> +# test: app-tap/http_client.test.lua<br>
> +# source: src/tarantool<br>
> +leak:Curl_setstropt<br>
> +leak:create_conn<br>
> +leak:Curl_conncache_add_conn<br>
> +leak:alloc_addbyter<br>
> +leak:Curl_getaddrinfo_ex<br>
> +leak:Curl_cache_addr<br>
> +leak:Curl_hash_init<br>
> +leak:Curl_hash_add<br>
> +leak:Curl_he2ai<br>
> +leak:Curl_open<br>
> +leak:Curl_resolver_init<br>
> +<br>
> +# test: app-tap/iconv.test.lua<br>
> +# source: /usr/lib/x86_64-linux-gnu/gconv/UTF-16.so<br>
> +leak:gconv_init<br>
> +<br>
> +# test: box*/<br>
> +# source: third_party/luajit<br>
> +leak:lj_BC_FUNCC<br>
> +<br>
> +# test: box/access.test.lua<br>
> +# test: box/access_bin.test.lua<br>
> +# test: box/access_misc.test.lua<br>
> +# source: src/box/error.cc<br>
> +leak:AccessDeniedError::AccessDeniedError<br>
> +<br>
> +# test: box/bitset.test.lua<br>
> +# source: src/lib/bitset/iterator.c<br>
> +leak:tt_bitset_iterator_init<br>
> +<br>
> +# test: box-py/args.test.py<br>
> +# source: /lib/x86_64-linux-gnu/libc.so*<br>
> +leak:libc.so*<br>
> +<br>
> +# test: box-tap/schema-mt.test.lua<br>
> +# source: src/lib/core/coio_task.c<br>
> +leak:coio_on_start<br>
> +# source: src/lib/salad/mhash.h<br>
> +leak:mh_i32ptr_new<br>
> +<br>
> +# test: replication/misc.test.lua<br>
> +# source: src/box/vy_log.c<br>
> +leak:vy_recovery_new_f<br>
> +# source: src/lib/salad/mhash.h<br>
> +leak:mh_i64ptr_new<br>
> +<br>
> +# test: sql-tap/gh2250-trigger-chain-limit.test.lua<br>
> +# source: src/lib/core/exception.cc<br>
> +leak:Exception::operator new<br>
> +<br>
> +# test: sql-tap/trigger9.test.lua<br>
> +# source: src/lib/core/fiber.c<br>
> +leak:cord_start<br>
> +<br>
> +# test: sql-tap/tkt-7bbfb7d442.test.lua<br>
> +# test: sql-tap/view.test.lua<br>
> +# test: sql-tap/with1.test.lua<br>
> +# test: sql-tap/with2.test.lua<br>
> +# source: src/box/sql/malloc.c<br>
> +leak:sql_sized_malloc<br>
> +<br>
> +# test: swim/errinj.test.lua<br>
> +# test: swim/swim.test.lua<br>
> +# source: src/lib/swim/swim.c<br>
> +leak:swim_member_new<br>
> +leak:swim_update_member_payload<br>
> +<br>
> +# !test: unit/bps_tree.test.lua<br>
> +# source: src/lib/salad/bps_tree.h<br>
> +leak:bps_tree_test_create_leaf<br>
> +leak:bps_tree_test_process_insert_leaf<br>
> +<br>
> +# !test: unit/heap.test.lua<br>
> +# source: test/unit/heap.c<br>
> +leak:test_random_delete_workload<br>
> +leak:test_delete_last_node<br>
> +<br>
> +# !test: unit/heap_iterator.test.lua<br>
> +# source: src/lib/salad/heap.h<br>
> +leak:test_heap_reserve<br>
> +<br>
> +# !test: unit/swim.test.lua<br>
> +# source: src/lib/swim/swim_io.c<br>
> +leak:swim_scheduler_set_codec<br>
> +<br>
> +# test: vinyl/errinj.test.lua<br>
> +# source: src/lib/core/fiber.h<br>
> +leak:fiber_cxx_invoke<br>
> +<br>
> +# test: vinyl/errinj_ddl.test.lua<br>
> +# source: src/box/vy_stmt.c<br>
> +leak:vy_stmt_alloc<br>
> +<br>
> +# test: vinyl/recover.test.lua<br>
> +# source: src/lib/core/fiber.c<br>
> +leak:cord_costart_thread_func<br>
> diff --git a/cmake/profile.cmake b/cmake/profile.cmake<br>
> index 0ba31fa2c..22e2fb2c9 100644<br>
> --- a/cmake/profile.cmake<br>
> +++ b/cmake/profile.cmake<br>
> @@ -53,7 +53,7 @@ if (ENABLE_ASAN)<br>
> "\n")<br>
> endif()<br>
> <br>
> - set(CMAKE_REQUIRED_FLAGS "-fsanitize=address")<br>
> + set(CMAKE_REQUIRED_FLAGS "-fsanitize=address -fsanitize-blacklist=${CMAKE_SOURCE_DIR}/asan/ignore_asan.txt")<br>
> check_c_source_compiles("int main(void) {<br>
> #include <sanitizer/asan_interface.h><br>
> void *x;<br>
> @@ -76,5 +76,5 @@ if (ENABLE_ASAN)<br>
> message(FATAL_ERROR "Cannot enable AddressSanitizer")<br>
> endif()<br>
> <br>
> - add_compile_flags("C;CXX" -fsanitize=address)<br>
> + add_compile_flags("C;CXX" -fsanitize=address -fsanitize-blacklist=${CMAKE_SOURCE_DIR}/asan/ignore_asan.txt)<br>
> endif()<br>
> diff --git a/test/box/on_shutdown.skipcond b/test/box/on_shutdown.skipcond<br>
> new file mode 100644<br>
> index 000000000..e46fd1088<br>
> --- /dev/null<br>
> +++ b/test/box/on_shutdown.skipcond<br>
> @@ -0,0 +1,7 @@<br>
> +import os<br>
> +<br>
> +# Disabled at ASAN build due to issue #4360.<br>
> +if os.getenv("ASAN") == 'ON':<br>
> + self.skip = 1<br>
> +<br>
> +# vim: set ft=python:<br>
> -- <br>
> 2.17.1<br>
> <br><br>
As discussed offline, please send the follow-up patches for all other<br>
branches if needed.</div></div></div></div></blockquote>Ok.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15790443702091472626_BODY"><br><br>
-- <br>
Best regards,<br>
IM<br></div></div></div></div></blockquote>
<br>
<br>-- <br>Alexander Tikhonov<br></BODY></HTML>