On Fri, May 07, 2021 at 09:10:44AM +0200, Tim Wiederhake wrote: > On Fri, 2021-05-07 at 08:36 +0200, Erik Skultety wrote: > > 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@xxxxxxxxxx> > > > > --- > > > > .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. > > AddressSanitizer finds issues like leaks, use-after-free, and double- > free of memory. As there are currently none of these issues found, any > new finding would be a regression which in my opinion does warrant a > build failure. > > The sanitizer is not known to produce false positives, but should that > situation arise in the future, we can use of suppression lists, see > https://clang.llvm.org/docs/AddressSanitizer.html#issue-suppression. > > > 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 > > I agree. $TEMPORARILY_DISABLED is a rather big hammer though to disable > sanitation entirely; I do not see a need for this but left it in for > consistency with how other build jobs can be made non-required. Arguably it may be a big hammer, but I actually don't object to having it in. Like I said, we already have a precedent plus the job spec you propose doesn't hinder readability in any way nor does it add any complexity. Regards, Erik
