Re: [libvirt PATCH v2 7/7] ci: Enable address and undefined behavior sanitizers

On Thu, May 06, 2021 at 05:34:55PM +0200, Peter Krempa wrote: > On Thu, May 06, 2021 at 17:08:38 +0200, Tim Wiederhake wrote: > > meson supports the following sanitizers: "address" (e.g. out-of- > > bounds > > memory access, use-after-free, etc.), "thread" (data races), > > "undefined" > > (e.g. signed integer overflow), and "memory" (use of > > uninitialized > > memory). Note that not all sanitizers are supported by all > > compilers, > > and that more sanitizers exist. > > > > Not all sanitizers can be enabled at the same time, but "address" > > and > > "undefined" can. Both thread and memory sanitizers require an > > instrumented > > build of all dependencies, including libc. > > > > gcc and clang use different implementations of these sanitizers > > and > > have proven to find different issues. Create CI jobs for both. > > > > Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com&gt; > > --- > > .gitlab-ci.yml | 35 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) > > > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > > index 89f618e678..4de4c30d7f 100644 > > --- a/.gitlab-ci.yml > > +++ b/.gitlab-ci.yml > > @@ -73,6 +73,26 @@ stages: > > rpmbuild --nodeps -ta build/meson-dist/libvirt-*.tar.xz; > > fi > > > > +.sanitizer_build_job: > > + stage: builds > > + image: $CI_REGISTRY_IMAGE/ci-ubuntu-2004:latest > > + needs: > > + - x64-ubuntu-2004-container > > + rules: > > + - if: "$TEMPORARILY_DISABLED" > > + allow_failure: true > > Does this mean that if $TEMPORARILY_DISABLED is not passed then the > sanitizer error causes a pipeline failure? Yes, that's exactly what would happen. > If yes then I'd like to know how we are going to waive false- > positives > as modifying the code is the wrong action in such case. I agree that sanitizers should probably not cause hard failures of the pipeline.

On the other hand that's exactly what would happen with coverity which is also setup as a hard failure, so we kinda have a precedent. The question you need to answer for yourself is - if we set both coverity and sanitizer jobs to soft failures by default, how likely it is that someone is going to look at those failures and fix them in a timely manner? That's why the TEMPORARILY_DISABLED variable exists in the first place, if a failure occurs, someone has to look at the issue, determine that it's a false positive and unless we're immediately able to figure out how to alleviate the issue (e.g. adding a rule to coverity to ignore a certain false positive), we convert the job to a soft failing one. Once we addressed the problem in the sanitizer, we can again enable the job fully. Erik