Message-ID: <87pndvrpvj.fsf@mpe.ellerman.id.au>
Date:   Mon, 02 Mar 2020 21:51:44 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KVM list <kvm@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>
Subject: Re: [GIT PULL] Second batch of KVM changes for Linux 5.6-rc4 (or rc5)

Linus Torvalds <torvalds@...ux-foundation.org> writes:
> On Sun, Mar 1, 2020 at 1:03 PM Paolo Bonzini <pbonzini@...hat.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.
>
> 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.
>
> 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?

My memory is that we have had very few reports of it actually causing
problems. But I don't have hard data to back that up.

It has tripped up the Clang folks, but that's partly because they're
building clang HEAD, and also because ~zero powerpc kernel developers
are building regularly with clang. I'm trying to fix the latter ...


The thing that makes me disable -Werror (enable PPC_DISABLE_WERROR) most
often is bisecting back to before fixes for my current compiler were
merged.

For example with GCC 8 if you go back before ~4.18 you hit the warning
fixed by bee20031772a ("disable -Wattribute-alias warning for
SYSCALL_DEFINEx()").

And then building with GCC head sometimes requires disabling -Werror
because of some new warning, sometimes valid sometimes not.

I think we could mostly avoid those problems by having the option only
on by default for known compiler versions.

eg:

config WERROR
	bool "Build with -Werror"
	default CC_IS_GCC && (GCC_VERSION >= 70000 && GCC_VERSION <= 90000)

And we could bump the upper version up once each new GCC version has had
any problems ironed out.

> Could we have a _generic_ option that just gets enabled by default,
> except it gets disabled by _known_ issues (like KASAN).

Right now I don't think we could have a generic option that's enabled by
default, there's too many warnings floating around on minor arches and
in odd configurations.

But we could have a generic option that signifies the desire to build
with -Werror where possible, and then each arch/subsystem/etc could use
that config option to enable -Werror in stages.

Then after a release or three we could change the option to globally
enable -Werror and opt-out any areas that are still problematic.

It's also possible to use -Wno-error to turn certain warnings back into
warnings even when -Werror is set, so that's another way we could
incrementally attack the problem.


It'd also be nice if we could do:

 $ make WERROR=0

Or something similarly obvious to turn off the WERROR option. That way
users don't even have to edit their .config manually, they just rerun
make with WERROR=0 and it works.


> Being disabled for "make allmodconfig" is kind of against one of the
> _points_ of "the build should be warning-free".

True, it was just the conservative choice to disable it for allmod/yes.
We should probably revisit that these days.

cheers

Powered by blists - more mailing lists