[ovs-dev] [PATCH ovn 2/2] ci: Enable AddressSanitizer in Linux CI runs.

[Dumitru Ceara]

On 12/16/20 11:24 AM, Ilya Maximets wrote:
> On 12/16/20 9:11 AM, Dumitru Ceara wrote:
>> To make sure no memory leaks or invalid accesses reported by
>> AddressSanitizer are missed, also skip rechecking tests if
>> AddressSanitizer reports are present in the test run directory.
> 
> Hi, Dumitru.
> 
> Thanks for working on this!  I think that it's very useful as we just
> fixed all the leaks detected by the testsuite.  Some comments inline.
> 
> Best regards, Ilya Maximets.
> 
>>
>> Note: This doubles GitHub Actions CI time from ~9 minutes (before the
>>       change) to ~18 minutes (after the change).
> 
> It might make sense to only have one job that runs under address sanitizer.
> That should save some CI time.  It might be a single clang build with tests
> enabled (I have no experience with gcc+sanitizer).
> 

Makes sense, I'll enable it only for clang+tests.

>>
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>> ---
>>  .ci/linux-build.sh |    2 +-
>>  tests/automake.mk  |    6 +++++-
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
>> index 0e9f87f..6943b01 100755
>> --- a/.ci/linux-build.sh
>> +++ b/.ci/linux-build.sh
>> @@ -3,7 +3,7 @@
>>  set -o errexit
>>  set -x
>>  
>> -CFLAGS=""
>> +CFLAGS="-fsanitize=address -fno-omit-frame-pointer -fno-common"
> 
> This affects both OVS and OVN.  It might be better to only build OVN
> with sanitizer enabled to avoid random issues in OVS code.
> This should also speed tests up a bit, I guess.
> 

Great point.  It does.  Testing it out it only adds 4m to the test run
(instead of 8m when enabling sanitizer for OVS too).

>>  SPARSE_FLAGS=""
>>  EXTRA_OPTS="--enable-Werror"
>>  
>> diff --git a/tests/automake.mk b/tests/automake.mk
>> index c5c286e..34464c7 100644
>> --- a/tests/automake.mk
>> +++ b/tests/automake.mk
>> @@ -52,6 +52,7 @@ check_SCRIPTS += tests/atlocal
>>  
>>  TESTSUITE = $(srcdir)/tests/testsuite
>>  TESTSUITE_PATCH = $(srcdir)/tests/testsuite.patch
>> +TESTSUITE_DIR = $(abs_top_builddir)/tests/testsuite.dir
>>  SYSTEM_KMOD_TESTSUITE = $(srcdir)/tests/system-kmod-testsuite
>>  SYSTEM_USERSPACE_TESTSUITE = $(srcdir)/tests/system-userspace-testsuite
>>  DISTCLEANFILES += tests/atconfig tests/atlocal
>> @@ -61,8 +62,11 @@ AUTOTEST_PATH = $(ovs_builddir)/utilities:$(ovs_builddir)/vswitchd:$(ovs_builddi
>>  export ovs_srcdir
>>  
>>  check-local:
>> +	find $(TESTSUITE_DIR) -name 'asan.*' | xargs -n1 rm -f
> 
> This should not be necessary to remove asan reports, because autotest removes
> directories before the test.  As we discussed off-list, this was to avoid
> skipping the recheck if started with e.g., 'TESTSUITEFLAGS="43" RECHECK="yes',
> while there is an asan report for some other test, but it's not very frequent
> scenario, and I don't think that we will want to loose asan reports for other
> tests in this case anyway.
> 

Makes sense, I'll fix it in v2.

Thanks for the review!

Regards,
Dumitru

>>  	set $(SHELL) '$(TESTSUITE)' -C tests AUTOTEST_PATH=$(AUTOTEST_PATH); \
>> -	"$$@" $(TESTSUITEFLAGS) || (test X'$(RECHECK)' = Xyes && "$$@" --recheck)
>> +	"$$@" $(TESTSUITEFLAGS) || \
>> +	(test -z "$$(find $(TESTSUITE_DIR) -name 'asan.*')" && \
>> +	 test X'$(RECHECK)' = Xyes && "$$@" --recheck)
>>  

>>  # Python Coverage support.
>>  # Requires coverage.py http://nedbatchelder.com/code/coverage/.
>>
>