On 2 Jan 2024, at 15:36, Ilya Maximets wrote:
> On 1/2/24 15:12, Eelco Chaudron wrote:
>>
>>
>> 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?
>
> I actually do not know if these test will be broken if we have both asan
> and ubsan. It might be that asan will be able to prevent a failure by
> capturing the signal. Could you try that?
>
> If that's the case we could change the TESTS_WITH_UBSAN check to check
> for ubsan+!asan (looking at the actual check it is not very robust already,
> so might need adjustments regardless...).
Thanks for the suggestions! Will do some experiments and send a v2.
//Eelco
>>
>>>> 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.
>
> A list might be better. The list of sanitizers could be plugged directly
> into -fsanitize, i.e. -fsanitize=${SANITIZERS}.
>
>>
>>> 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.
>
> All the CFLAGS are actually the same in both cases, except for the
> main -fsanitize value. Exporting ASAN_OPTIONS unconditionally
> also should not be a problem. So, this change can be made even if
> want to use sanitizers separately.
>
>>
>>>> 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
