Igor,

Thank you for your review, I've made all changes that you suggested.

Среда, 20 ноября 2019, 17:40 +03:00 от Igor Munkin <imun@tarantool.org>:

Sasha,

The patch doesn't seem to be cherry-picked but partially based on
55f7586. Please adjust the commit message and changes considering the
comments I left below.

On 06.11.19, Alexander V. Tikhonov wrote:
> Added ASAN tesing in commit process, used clang-8 for
> ASAN build under debian-buster image. Added for testing
> only the passing test suites, the rest of the tests
> not used and will be enabled durring issue #4360.

Please drop few lines about the fact you've reduced a number of targets
being tested via Travis, therefore this part is stripped from the
original patch.

>
> Closes #4359
>
> (cherry picked from commit 55f7586ac36760f31255fee7b70606f466f30e04)

As I mentioned above, adjust this line, because it's not a cherry-pick
but adapting to 1.10 branch.

> ---
>
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/asan_1.10
>
> .gitlab-ci.yml | 5 +++++
> .travis.mk | 24 ++++++++++++++++++++++++
> test/app-tap/json.skipcond | 7 +++++++
> test/unit/guard.skipcond | 7 +++++++
> test/unit/msgpack.skipcond | 7 +++++++
> 5 files changed, 50 insertions(+)
> create mode 100644 test/app-tap/json.skipcond
> create mode 100644 test/unit/guard.skipcond
> create mode 100644 test/unit/msgpack.skipcond
>
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 04bb4b1cc..5d3702ae5 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -85,6 +85,11 @@ release_lto_clang8:
> script:
> - ${GITLAB_MAKE} test_debian_no_deps
>
> +release_asan_clang8:
> + <<: *docker_test_clang8_definition
> + script:
> + - ${GITLAB_MAKE} test_asan_debian_no_deps
> +
> osx_13_release:
> <<: *release_only_definition
> <<: *vbox_definition
> diff --git a/.travis.mk b/.travis.mk
> index bfb3016da..8454eb4a5 100644
> --- a/.travis.mk
> +++ b/.travis.mk
> @@ -16,6 +16,9 @@ test: test_$(TRAVIS_OS_NAME)
> # Redirect some targets via docker
> test_linux: docker_test_debian
> coverage: docker_coverage_debian
> +lto: docker_test_debian
> +lto_clang8: docker_test_debian_clang8

All LTO related stuff has been stripped from the original patch, thus
these definitions seems to be excess.

> +asan: docker_test_asan_debian
>
> docker_%:
> mkdir -p ~/.cache/ccache
> @@ -68,6 +71,8 @@ deps_buster_clang_8: deps_debian
> apt-get update
> apt-get install -y clang-8 llvm-8-dev
>
> +# Release
> +
> build_debian:
> cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
> make -j
> @@ -77,6 +82,10 @@ test_debian_no_deps: build_debian
>
> test_debian: deps_debian test_debian_no_deps
>
> +test_debian_clang8: deps_debian deps_buster_clang_8 test_debian_no_deps
> +
> +# Debug with coverage
> +
> build_coverage_debian:
> cmake . -DCMAKE_BUILD_TYPE=Debug -DENABLE_GCOV=ON
> make -j
> @@ -97,6 +106,21 @@ test_coverage_debian_no_deps: build_coverage_debian
>
> 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}
> + 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/
> +
> +test_asan_debian: deps_debian deps_buster_clang_8 test_asan_debian_no_deps
> +
> #######
> # OSX #
> #######

Side note: I guess it's a good practice for future to write a rationale
for excluded tests right to skipcond file.

That is the traditional way that is in use for now, may be decide to change

in the future, but it will be the standalone patch for all the skipcond files.

> diff --git a/test/app-tap/json.skipcond b/test/app-tap/json.skipcond
> new file mode 100644
> index 000000000..e46fd1088
> --- /dev/null
> +++ b/test/app-tap/json.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:
> diff --git a/test/unit/guard.skipcond b/test/unit/guard.skipcond
> new file mode 100644
> index 000000000..e46fd1088
> --- /dev/null
> +++ b/test/unit/guard.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:
> diff --git a/test/unit/msgpack.skipcond b/test/unit/msgpack.skipcond
> new file mode 100644
> index 000000000..e46fd1088
> --- /dev/null
> +++ b/test/unit/msgpack.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
>

--
Best regards,
IM