linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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)
Date: Sun, 1 Mar 2020 22:10:41 -0700	[thread overview]
Message-ID: <20200302051041.GA2698@ubuntu-m2-xlarge-x86> (raw)
In-Reply-To: <CAHk-=wiin_LkqP2Cm5iPc5snUXYqZVoMFawZ-rjhZnawven8SA@mail.gmail.com>

+ 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

  parent reply	other threads:[~2020-03-02  5:10 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-01 19:03 [GIT PULL] Second batch of KVM changes for Linux 5.6-rc4 (or rc5) Paolo Bonzini
2020-03-01 21:33 ` Linus Torvalds
2020-03-01 23:04   ` Paolo Bonzini
2020-03-02  5:10   ` Nathan Chancellor [this message]
2020-03-02 10:51   ` Michael Ellerman
2020-03-02 12:14     ` Segher Boessenkool
2020-03-01 22:45 ` pr-tracker-bot
2020-03-02  5:38 ` Naresh Kamboju
2020-03-02  7:21   ` Wanpeng Li
2020-03-02 11:31     ` Anders Roxell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200302051041.GA2698@ubuntu-m2-xlarge-x86 \
    --to=natechancellor@gmail.com \
    --cc=clang-built-linux@googlegroups.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mpe@ellerman.id.au \
    --cc=pbonzini@redhat.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).