<HTML><BODY><p>Igor,<br><br>Thank you for your review, I've made all changes that you suggested.</p><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Среда, 20 ноября 2019, 17:40 +03:00 от Igor Munkin <imun@tarantool.org>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15742608510862847401_BODY">Sasha,<br><br>
The patch doesn't seem to be cherry-picked but partially based on<br>
55f7586. Please adjust the commit message and changes considering the<br>
comments I left below.</div></div></div></div></blockquote>Done.<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_15742608510862847401_BODY"><br>
On 06.11.19, Alexander V. Tikhonov wrote:<br>
> Added ASAN tesing in commit process, used clang-8 for<br>
> ASAN build under debian-buster image. Added for testing<br>
> only the passing test suites, the rest of the tests<br>
> not used and will be enabled durring issue #4360.<br><br>
Please drop few lines about the fact you've reduced a number of targets<br>
being tested via Travis, therefore this part is stripped from the<br>
original patch.<br></div></div></div></div></blockquote>Wrote more information on LTO.<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_15742608510862847401_BODY"><br>
> <br>
> Closes #4359<br>
> <br>
> (cherry picked from commit 55f7586ac36760f31255fee7b70606f466f30e04)<br><br>
As I mentioned above, adjust this line, because it's not a cherry-pick<br>
but adapting to 1.10 branch.<br></div></div></div></div></blockquote>Corrected the message.<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_15742608510862847401_BODY"><br>
> ---<br>
> <br>
> Github: <a href="https://github.com/tarantool/tarantool/tree/avtikhon/asan_1.10" target="_blank">https://github.com/tarantool/tarantool/tree/avtikhon/asan_1.10</a><br>
> <br>
> .gitlab-ci.yml | 5 +++++<br>
> .travis.mk | 24 ++++++++++++++++++++++++<br>
> test/app-tap/json.skipcond | 7 +++++++<br>
> test/unit/guard.skipcond | 7 +++++++<br>
> test/unit/msgpack.skipcond | 7 +++++++<br>
> 5 files changed, 50 insertions(+)<br>
> create mode 100644 test/app-tap/json.skipcond<br>
> create mode 100644 test/unit/guard.skipcond<br>
> create mode 100644 test/unit/msgpack.skipcond<br>
> <br>
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml<br>
> index 04bb4b1cc..5d3702ae5 100644<br>
> --- a/.gitlab-ci.yml<br>
> +++ b/.gitlab-ci.yml<br>
> @@ -85,6 +85,11 @@ release_lto_clang8:<br>
> script:<br>
> - ${GITLAB_MAKE} test_debian_no_deps<br>
> <br>
> +release_asan_clang8:<br>
> + <<: *docker_test_clang8_definition<br>
> + script:<br>
> + - ${GITLAB_MAKE} test_asan_debian_no_deps<br>
> +<br>
> osx_13_release:<br>
> <<: *release_only_definition<br>
> <<: *vbox_definition<br>
> diff --git a/.travis.mk b/.travis.mk<br>
> index bfb3016da..8454eb4a5 100644<br>
> --- a/.travis.mk<br>
> +++ b/.travis.mk<br>
> @@ -16,6 +16,9 @@ test: test_$(TRAVIS_OS_NAME)<br>
> # Redirect some targets via docker<br>
> test_linux: docker_test_debian<br>
> coverage: docker_coverage_debian<br>
> +lto: docker_test_debian<br>
> +lto_clang8: docker_test_debian_clang8<br><br>
All LTO related stuff has been stripped from the original patch, thus<br>
these definitions seems to be excess.<br></div></div></div></div></blockquote>Right, that was mistake - removed these lines.<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_15742608510862847401_BODY"><br>
> +asan: docker_test_asan_debian<br>
> <br>
> docker_%:<br>
> mkdir -p ~/.cache/ccache<br>
> @@ -68,6 +71,8 @@ deps_buster_clang_8: deps_debian<br>
> apt-get update<br>
> apt-get install -y clang-8 llvm-8-dev<br>
> <br>
> +# Release<br>
> +<br>
> build_debian:<br>
> cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}<br>
> make -j<br>
> @@ -77,6 +82,10 @@ test_debian_no_deps: build_debian<br>
> <br>
> test_debian: deps_debian test_debian_no_deps<br>
> <br>
> +test_debian_clang8: deps_debian deps_buster_clang_8 test_debian_no_deps<br>
> +<br>
> +# Debug with coverage<br>
> +<br>
> build_coverage_debian:<br>
> cmake . -DCMAKE_BUILD_TYPE=Debug -DENABLE_GCOV=ON<br>
> make -j<br>
> @@ -97,6 +106,21 @@ test_coverage_debian_no_deps: build_coverage_debian<br>
> <br>
> coverage_debian: deps_debian test_coverage_debian_no_deps<br>
> <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>
> + 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>
> +<br>
> +test_asan_debian: deps_debian deps_buster_clang_8 test_asan_debian_no_deps<br>
> +<br>
> #######<br>
> # OSX #<br>
> #######<br><br>
Side note: I guess it's a good practice for future to write a rationale<br>
for excluded tests right to skipcond file.<br></div></div></div></div></blockquote><p>That is the traditional way that is in use for now, may be decide to change</p><p> in the future, but it will be the standalone patch for all the skipcond files.</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_15742608510862847401_BODY"><br>
> diff --git a/test/app-tap/json.skipcond b/test/app-tap/json.skipcond<br>
> new file mode 100644<br>
> index 000000000..e46fd1088<br>
> --- /dev/null<br>
> +++ b/test/app-tap/json.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>
> diff --git a/test/unit/guard.skipcond b/test/unit/guard.skipcond<br>
> new file mode 100644<br>
> index 000000000..e46fd1088<br>
> --- /dev/null<br>
> +++ b/test/unit/guard.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>
> diff --git a/test/unit/msgpack.skipcond b/test/unit/msgpack.skipcond<br>
> new file mode 100644<br>
> index 000000000..e46fd1088<br>
> --- /dev/null<br>
> +++ b/test/unit/msgpack.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>
> -- <br>
> 2.17.1<br>
> <br><br>
-- <br>
Best regards,<br>
IM<br><br></div></div></div></div></blockquote>
<br>
<br>-- <br>Alexander Tikhonov<br></BODY></HTML>