On 2 Jan 2024, at 12:44, Ilya Maximets wrote:
> On 12/22/23 12:12, Eelco Chaudron wrote:
>> This patch combines the existing ubsan and asan GitHub actions
>> tests into one.
>
> Not sure how important it is, but worth mentioning that this
> change reduces the test coverage. Some tests are skipped
> for ubsan specifically, but not for asan. IIRC, these are
> tests for the process monitoring and backtraces that trigger
> SIGSEGV; ubsan can't handle this signal, while asan can.
>
> At least, this should be added into commit message.
I was not aware of this. Should we maybe keep them seperate for the base run,
and only combine the datapath ones?
>> Signed-off-by: Eelco Chaudron <[email protected]>
>> ---
>> .ci/linux-build.sh | 4 ++--
>> .github/workflows/build-and-test.yml | 8 ++------
>> 2 files changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
>> index 90581c10b..c780dac01 100755
>> --- a/.ci/linux-build.sh
>> +++ b/.ci/linux-build.sh
>> @@ -102,14 +102,14 @@ else
>> CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${SPARSE_FLAGS}"
>> fi
>>
>> -if [ "$ASAN" ]; then
>> +if [[ $SANITIZERS == *asan* ]]; then
>> # This will override default option configured in tests/atlocal.in.
>> export ASAN_OPTIONS='detect_leaks=1'
>> CFLAGS_ASAN="-fno-omit-frame-pointer -fno-common -fsanitize=address"
>> CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_ASAN}"
>> fi
>>
>> -if [ "$UBSAN" ]; then
>> +if [[ $SANITIZERS == *ubsan* ]]; then
>> # Use the default options configured in tests/atlocal.in, in
>> UBSAN_OPTIONS.
>> CFLAGS_UBSAN="-fno-omit-frame-pointer -fno-common -fsanitize=undefined"
>> CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_UBSAN}"
>
> Do we need two separate cases now? Also, we don't need to
> add -fno-omit-frame-pointer and -fno-common twice.
ACK, I was assuming someone could still request one of the two.
But if we only want to support running both, I can make the SANITIZERS variable
a boolean.
> It is also a little annoying that ASAN_OPTIONS and UBSAN_OPTIONS
> are not really separate sets of options, they do overlap.
> For example, if the log_path is defined in both, the one from
> UBSAN_OPTIONS will actually be used. So all the log files
> will be ubsan.* even if they come from asan. Maybe rename these
> files as well, similarly to what OVN project did?
Yes, will make that change.
> I'm not sure if there are other option conflicts.
Did a quick search, but could not find it.
>> diff --git a/.github/workflows/build-and-test.yml
>> b/.github/workflows/build-and-test.yml
>> index 710757693..b901e6d57 100644
>> --- a/.github/workflows/build-and-test.yml
>> +++ b/.github/workflows/build-and-test.yml
>> @@ -78,14 +78,13 @@ jobs:
>> automake libtool gcc bc libjemalloc2 libjemalloc-dev libssl-dev \
>> llvm-dev libnuma-dev libpcap-dev selinux-policy-dev libbpf-dev \
>> lftp libreswan
>> - ASAN: ${{ matrix.asan }}
>> - UBSAN: ${{ matrix.ubsan }}
>> CC: ${{ matrix.compiler }}
>> DPDK: ${{ matrix.dpdk }}
>> DPDK_SHARED: ${{ matrix.dpdk_shared }}
>> LIBS: ${{ matrix.libs }}
>> M32: ${{ matrix.m32 }}
>> OPTS: ${{ matrix.opts }}
>> + SANITIZERS: ${{ matrix.sanitizers }}
>> STD: ${{ matrix.std }}
>> TESTSUITE: ${{ matrix.testsuite }}
>> TEST_RANGE: ${{ matrix.test_range }}
>> @@ -111,11 +110,8 @@ jobs:
>> - compiler: gcc
>> testsuite: test
>> - compiler: clang
>> + sanitizers: asan ubsan
>
> If we define this variable as 'address,undefined', we could
> plug this variable directly into the '-fsanitize='.
We could do this, see also above about supporting individual options.
>> testsuite: test
>> - asan: asan
>> - - compiler: clang
>> - testsuite: test
>> - ubsan: ubsan
>>
>> - compiler: gcc
>> testsuite: test
>>
>
> Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
