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...). > >>> 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
