Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 1 Mar 2020 22:10:41 -0700
From:   Nathan Chancellor <natechancellor@gmail.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Michael Ellerman <mpe@ellerman.id.au>,
        linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com
Subject: Re: [GIT PULL] Second batch of KVM changes for Linux 5.6-rc4 (or rc5)
Message-ID: <20200302051041.GA2698@ubuntu-m2-xlarge-x86>
References: <1583089390-36084-1-git-send-email-pbonzini@redhat.com>
 <CAHk-=wiin_LkqP2Cm5iPc5snUXYqZVoMFawZ-rjhZnawven8SA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wiin_LkqP2Cm5iPc5snUXYqZVoMFawZ-rjhZnawven8SA@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

+ CBL mailing list, my two cents below.

On Sun, Mar 01, 2020 at 03:33:48PM -0600, Linus Torvalds wrote:
> On Sun, Mar 1, 2020 at 1:03 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > Paolo Bonzini (4):
> >       KVM: allow disabling -Werror
> 
> Honestly, this is just badly done.
> 
> You've basically made it enable -Werror only for very random
> configurations - and apparently the one you test.
> 
> Doing things like COMPILE_TEST disables it, but so does not having
> EXPERT enabled.
> 
> So it looks entirely ad-hoc and makes very little sense. At least the
> "with KASAN, disable this" part makes sense, since that's a known
> source or warnings. But everything else looks very random.
> 
> I've merged this, but I wonder why you couldn't just do what I
> suggested originally?
> 
> Seriously, if you script your build tests, and don't even look at the
> results, then you might as well use
> 
>    make KCFLAGS=-Werror
> 
> instead of having this kind of completely random option that has
> almost no logic to it at all.
> 
> And if you depend entirely on random build infrastructure like the
> 0day bot etc, this likely _is_ going to break when it starts using a
> new gcc version, or when it starts testing using clang, or whatever.
> So then we end up with another odd random situation where now kvm (and
> only kvm) will fail those builds just because they are automated.

Just FYI, 0day clang builds are live now. I have not seen anything from
KVM yet but I do not know how many configurations are being tested and
such:

https://lore.kernel.org/lkml/CAKwvOdn9mpsjpAbVQbS0LC9iPtNrCZU+Pbh2Bt7kSXa4S8KQEg@mail.gmail.com/

> Yes, as I said in that original thread, I'd love to do -Werror in
> general, at which point it wouldn't be some random ad-hoc kvm special
> case for some random option. But the "now it causes problems for
> random compiler versions" is a real issue again - but at least it
> wouldn't be a random kernel subsystem that happens to trigger it, it
> would be a _generic_ issue, and we'd have everybody involved when a
> compiler change introduces a new warning.

Indeed; our CI for clang builds is all done with the master versions of
clang available from apt.llvm.org so we can catch issues as soon as
possible and having -Werror would mean a potentially benign issue (like
one we are currently dealing with where clang's -Wvoid-pointer-to-int-cast
warns when casting to an enum where gcc does not:
https://reviews.llvm.org/D72231#1878528) would cause our CI to go
instantly red across the board and not catch other issues.

I would say a CONFIG_CC_WERROR or something of that nature would not
necessarily be a bad thing since we could just disable it for our CI (or
have it be default disabled) but I think if someone cares about -Werror,
they should just use the KCFLAGS trick at the point, since I would think
that would be easier than maintaining a separate config option.

> I've pulled this for now, but I really think it's a horrible hack, and
> it's just done entirely wrong.
> 
> Adding the powerpc people, since they have more history with their
> somewhat less hacky one. Except that one automatically gets disabled
> by "make allmodconfig" and friends, which is also kind of pointless.
> 
> Michael, what tends to be the triggers for people using
> PPC_DISABLE_WERROR? Do you have reports for it? Could we have a
> _generic_ option that just gets enabled by default, except it gets
> disabled by _known_ issues (like KASAN).
> 
> Being disabled for "make allmodconfig" is kind of against one of the
> _points_ of "the build should be warning-free".
> 
>                Linus

FWIW, our clang powerpc builds were broken for 4 months because of
arch/powerpc using -Werror:

https://github.com/ClangBuiltLinux/linux/issues/625

We have infrastructure in place to carry out of tree patches if
absolutely necessary, which we had to use for a while:

https://github.com/ClangBuiltLinux/continuous-integration/commit/4d1b3c74bcb3ed0090073c9d9e6ae303425d4adc
https://github.com/ClangBuiltLinux/continuous-integration/commit/0c3885049e2a6e28c72be13f5b05fb25ff79904b
https://github.com/ClangBuiltLinux/continuous-integration/commit/533fcec33fdb8526333a6fd91d24534eeb96bed5

Even now, there is a warning in the RDMA subsystem that points to a very
clear bug that has still not been merged into your tree:

https://git.kernel.org/rdma/rdma/c/4ca501d6aaf21de31541deac35128bbea8427aa6

I am not blaming anyone for that, I get that every maintainer has their
own test suite and pull request schedule but I believe that it shows
the general apathy towards warning fixes from maintainers (at least,
from the perspective of someone who sends a large amount of warning
fixes). I would argue this should change before -Werror could even
begin to be considered as a default option.

Cheers,
Nathan