Messages in this thread | | | Date | Sun, 1 Mar 2020 22:10:41 -0700 | From | Nathan Chancellor <> | Subject | Re: [GIT PULL] Second batch of KVM changes for Linux 5.6-rc4 (or rc5) |
| |
+ 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
| |