| Message ID | 170324353685.1849689.7766414272675204016.stgit@ebuild |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,1/2] ci: Combine the ubsan and asan sanitizer runs. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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. > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > .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. 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? I'm not sure if there are other option conflicts. > 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='. > testsuite: test > - asan: asan > - - compiler: clang > - testsuite: test > - ubsan: ubsan > > - compiler: gcc > testsuite: test > Best regards, Ilya Maximets.
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 <echaudro@redhat.com> >> --- >> .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.
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 <echaudro@redhat.com> >>> --- >>> .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. >
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 <echaudro@redhat.com> >>>> --- >>>> .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. >>
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}" 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 testsuite: test - asan: asan - - compiler: clang - testsuite: test - ubsan: ubsan - compiler: gcc testsuite: test
This patch combines the existing ubsan and asan GitHub actions tests into one. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- .ci/linux-build.sh | 4 ++-- .github/workflows/build-and-test.yml | 8 ++------ 2 files changed, 4 insertions(+), 8 deletions(-)