Re: [Tarantool-patches] [PATCH v1 2/2] gitlab-ci: ASAN testing memory leaks add

[Igor Munkin <imun@tarantool.org>]

Sasha,

Thanks for the patch! I left some notes below, please consider them.

On 21.11.19, Alexander V. Tikhonov wrote:

> gitlab-ci: ASAN testing memory leaks add
IMHO the commit subject should be adjusted as the following:
| gitlab-ci: add memory leaks detection via LSAN

> Added memory leaks detection to ASAN testing. Added files for exceptions:
>  - address sanitizer on compilation:  asan/ignore_asan.txt
>  - memory leak sanitizer on run-time: asan/ignore_lsan.txt
> Cmake updated with asan/ignore_asan.txt file added and test run rules
> changed with asan/ignore_lsan.txt file. Added 'engine' and replication'
> suites into the testing and blocked 'box/on_shutdown.test.lua' test
> that breaks the testing, all of excepted tests will be enabled durring
> issue #4360.
> 
> Close #2058

I propose to reword the commit message as the following:
| The change enables memory leaks detection to existing ASAN testing
| routine and introduces suppression files with the corresponding
| exception list:
| * address sanitizer for compile-time: asan/asan.supp
| * memory leak sanitizer for run-time: asan/lsan.supp
|
| Furthermore, added engine and replication suites for ASAN testing
| routine.
|
| Additionally to the tests blacklisted within #4359,
| 'box/on_shutdown.test.lua' is also disabled since it fails the
| introduced leak check. All blacklisted tests have to be enabled
| within #4360.
|
| Closes #2058

Feel free to adjust it on your own way.

> ---
>  .travis.mk                    |  13 +++--
>  asan/ignore_asan.txt          |  17 ++++++
>  asan/ignore_lsan.txt          | 105 ++++++++++++++++++++++++++++++++++
>  cmake/profile.cmake           |   4 +-
>  test/box/on_shutdown.skipcond |   7 +++
>  5 files changed, 138 insertions(+), 8 deletions(-)
>  create mode 100644 asan/ignore_asan.txt
>  create mode 100644 asan/ignore_lsan.txt
>  create mode 100644 test/box/on_shutdown.skipcond
> 
> diff --git a/.travis.mk b/.travis.mk
> index 42969ff56..c6bc82d41 100644
> --- a/.travis.mk
> +++ b/.travis.mk
> @@ -111,15 +111,16 @@ coverage_debian: deps_debian test_coverage_debian_no_deps
>  # ASAN
>  
>  build_asan_debian:
> -	CC=clang-8 CXX=clang++-8 cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
> -	CC=clang-8 CXX=clang++-8 cmake . -DENABLE_ASAN=ON ${CMAKE_EXTRA_PARAMS}

Side note: have no idea why it had been divided into separate commands.

> +	CC=clang-8 CXX=clang++-8 cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON -DENABLE_ASAN=ON ${CMAKE_EXTRA_PARAMS}
>  	make -j
>  
>  test_asan_debian_no_deps: build_asan_debian
> -	# temporary excluded engine/ and replication/ suites with some tests from other suites by issue #4360
> -	cd test && ASAN=ON ASAN_OPTIONS=detect_leaks=0 ./test-run.py --force $(TEST_RUN_EXTRA_PARAMS) \
> -		app/ app-tap/ box/ box-py/ box-tap/ engine_long/ long_run-py/ luajit-tap/ \
> -		replication-py/ small/ sql/ sql-tap/ swim/ unit/ vinyl/ wal_off/ xlog/ xlog-py/
> +	# temporary excluded some tests by issue #4360
> +	# ASAN environment flag shows the excluded tests to skip the testing

As discussed offline, please drop a comment related to the difference in
applying suppressions for LSAN and ASAN.

> +	cd test && ASAN=ON \
> +		LSAN_OPTIONS=suppressions=${PWD}/asan/ignore_lsan.txt \
> +		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 \
> +		./test-run.py --force $(TEST_RUN_EXTRA_PARAMS)
>  
>  test_asan_debian: deps_debian deps_buster_clang_8 test_asan_debian_no_deps
>  

I propose to name suppresion lists in more traditional for such tools
way:
* s/ignore_asan.txt/asan.supp/
* s/ignore_lsan.txt/lsan.supp/

> diff --git a/asan/ignore_asan.txt b/asan/ignore_asan.txt
> new file mode 100644
> index 000000000..2a20114f4
> --- /dev/null
> +++ b/asan/ignore_asan.txt
> @@ -0,0 +1,17 @@
> +# File format:
> +#fun:*
> +#src:*
> +
> +# !test: app-tap/json.test.lua
> +# source: third_party/lua-cjson/lua_cjson.c
> +fun:json_decode
> +
> +# test: unit/base64.test.lua
> +# source: third_party/base64.c
> +fun:base64_decode_block
> +# source: test/unit/base64.c
> +fun:base64_test
> +
> +# !test: unit/msgpack.test
> +# source: src/lib/msgpuck/test/msgpuck.c
> +fun:test_mp_print
> diff --git a/asan/ignore_lsan.txt b/asan/ignore_lsan.txt
> new file mode 100644
> index 000000000..a1237fb86
> --- /dev/null
> +++ b/asan/ignore_lsan.txt
> @@ -0,0 +1,105 @@
> +# File format:
> +#leak:*
> +
> +# test: app/crypto.test.lua
> +# source: /usr/lib/x86_64-linux-gnu/libcrypto.so
> +leak:CRYPTO_zalloc
> +
> +# test: app-tap/http_client.test.lua
> +# source: src/tarantool
> +leak:Curl_setstropt
> +leak:create_conn
> +leak:Curl_conncache_add_conn
> +leak:alloc_addbyter
> +leak:Curl_getaddrinfo_ex
> +leak:Curl_cache_addr
> +leak:Curl_hash_init
> +leak:Curl_hash_add
> +leak:Curl_he2ai
> +leak:Curl_open
> +leak:Curl_resolver_init
> +
> +# test: app-tap/iconv.test.lua
> +# source: /usr/lib/x86_64-linux-gnu/gconv/UTF-16.so
> +leak:gconv_init
> +
> +# test: box*/
> +# source: third_party/luajit
> +leak:lj_BC_FUNCC
> +
> +# test: box/access.test.lua
> +# test: box/access_bin.test.lua
> +# test: box/access_misc.test.lua
> +# source: src/box/error.cc
> +leak:AccessDeniedError::AccessDeniedError
> +
> +# test: box/bitset.test.lua
> +# source: src/lib/bitset/iterator.c
> +leak:tt_bitset_iterator_init
> +
> +# test: box-py/args.test.py
> +# source: /lib/x86_64-linux-gnu/libc.so*
> +leak:libc.so*
> +
> +# test: box-tap/schema-mt.test.lua
> +# source: src/lib/core/coio_task.c
> +leak:coio_on_start
> +# source: src/lib/salad/mhash.h
> +leak:mh_i32ptr_new
> +
> +# test: replication/misc.test.lua
> +# source: src/box/vy_log.c
> +leak:vy_recovery_new_f
> +# source: src/lib/salad/mhash.h
> +leak:mh_i64ptr_new
> +
> +# test: sql-tap/gh2250-trigger-chain-limit.test.lua
> +# source: src/lib/core/exception.cc
> +leak:Exception::operator new
> +
> +# test: sql-tap/trigger9.test.lua
> +# source: src/lib/core/fiber.c
> +leak:cord_start
> +
> +# test: sql-tap/tkt-7bbfb7d442.test.lua
> +# test: sql-tap/view.test.lua
> +# test: sql-tap/with1.test.lua
> +# test: sql-tap/with2.test.lua
> +# source: src/box/sql/malloc.c
> +leak:sql_sized_malloc
> +
> +# test: swim/errinj.test.lua
> +# test: swim/swim.test.lua
> +# source: src/lib/swim/swim.c
> +leak:swim_member_new
> +leak:swim_update_member_payload
> +
> +# !test: unit/bps_tree.test.lua
> +# source: src/lib/salad/bps_tree.h
> +leak:bps_tree_test_create_leaf
> +leak:bps_tree_test_process_insert_leaf
> +
> +# !test: unit/heap.test.lua
> +# source: test/unit/heap.c
> +leak:test_random_delete_workload
> +leak:test_delete_last_node
> +
> +# !test: unit/heap_iterator.test.lua
> +# source: src/lib/salad/heap.h
> +leak:test_heap_reserve
> +
> +# !test: unit/swim.test.lua
> +# source: src/lib/swim/swim_io.c
> +leak:swim_scheduler_set_codec
> +
> +# test: vinyl/errinj.test.lua
> +# source: src/lib/core/fiber.h
> +leak:fiber_cxx_invoke
> +
> +# test: vinyl/errinj_ddl.test.lua
> +# source: src/box/vy_stmt.c
> +leak:vy_stmt_alloc
> +
> +# test: vinyl/recover.test.lua
> +# source: src/lib/core/fiber.c
> +leak:cord_costart_thread_func
> diff --git a/cmake/profile.cmake b/cmake/profile.cmake
> index 0ba31fa2c..22e2fb2c9 100644
> --- a/cmake/profile.cmake
> +++ b/cmake/profile.cmake
> @@ -53,7 +53,7 @@ if (ENABLE_ASAN)
>              "\n")
>      endif()
>  
> -    set(CMAKE_REQUIRED_FLAGS "-fsanitize=address")
> +    set(CMAKE_REQUIRED_FLAGS "-fsanitize=address -fsanitize-blacklist=${CMAKE_SOURCE_DIR}/asan/ignore_asan.txt")
>      check_c_source_compiles("int main(void) {
>          #include <sanitizer/asan_interface.h>
>          void *x;
> @@ -76,5 +76,5 @@ if (ENABLE_ASAN)
>          message(FATAL_ERROR "Cannot enable AddressSanitizer")
>      endif()
>  
> -    add_compile_flags("C;CXX" -fsanitize=address)
> +    add_compile_flags("C;CXX" -fsanitize=address -fsanitize-blacklist=${CMAKE_SOURCE_DIR}/asan/ignore_asan.txt)
>  endif()
> diff --git a/test/box/on_shutdown.skipcond b/test/box/on_shutdown.skipcond
> new file mode 100644
> index 000000000..e46fd1088
> --- /dev/null
> +++ b/test/box/on_shutdown.skipcond
> @@ -0,0 +1,7 @@
> +import os
> +
> +# Disabled at ASAN build due to issue #4360.
> +if os.getenv("ASAN") == 'ON':
> +    self.skip = 1
> +
> +# vim: set ft=python:
> -- 
> 2.17.1
> 

As discussed offline, please send the follow-up patches for all other
branches if needed.

-- 
Best regards,
IM