Linux-KBuild Archive on lore.kernel.org
 help / color / Atom feed
From: Arnd Bergmann <arnd@arndb.de>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Nick Desaulniers <ndesaulniers@google.com>,
	netdev <netdev@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Michal Marek <michal.lkml@markovi.net>,
	Rohit Maheshwari <rohitm@chelsio.com>,
	Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
	clang-built-linux <clang-built-linux@googlegroups.com>
Subject: Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today
Date: Fri, 2 Oct 2020 15:15:53 +0200
Message-ID: <CAK8P3a1Cyxo4mt2Kug92EvBhZJt2X6ct0_8JbZgo0zf0GSuanA@mail.gmail.com> (raw)
In-Reply-To: <20201002125110.GJ4067422@lunn.ch>

On Fri, Oct 2, 2020 at 2:51 PM Andrew Lunn <andrew@lunn.ch> wrote:
> On Fri, Oct 02, 2020 at 02:20:50PM +0200, Arnd Bergmann wrote:
> > On Fri, Oct 2, 2020 at 3:44 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > > On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote:
> > > >
> > > > I'm not a fan of this approach.  Are DATESTAMP configs just going to
> > > > keep being added? Is it going to complicate isolating the issue from a
> > > > randconfig build?  If we want more things to build warning-free at
> > > > W=1, then why don't we start moving warnings from W=1 into the
> > > > default, until this is no W=1 left?  That way we're cutting down on
> > > > the kernel's configuration combinatorial explosion, rather than adding
> > > > to it?
> >
> > I'm also a little sceptical about the datestamp.
>
> Hi Arnd
>
> Any suggestions for an alternative?

I think we should deal with it the same way we deal with new
compiler versions: before a new compiler starts getting widely
used, someone has to address the new warnings that show up,
or at the minimum they have to get turned off by default until
they are addressed.

Today, moving a warning flag from W=1 to default requires that
there won't be any regressions in the output. The same should
apply to adding W=1 warnings if there is a way for drivers to
default-enable them.

> > It won't change with the configuration, but it will change with the
> > specific compiler version. When you enable a warning in a
> > particular driver or directory, this may have different effects
> > on one compiler compared to another: clang and gcc sometimes
> > differ in their interpretation of which specific forms of an expression
> > to warn about or not, and any multiplexing options like -Wextra
> > or -Wformat may turn on additional warnings in later releases.
>
> How do we deal with this at the moment? Or will -Wextra and -Wformat
> never get moved into the default?

I think for Wextra, that would likely stay with W=1, though individual
warnings in that set should be enabled by default whenever they
make sense. For -Wformat, we probably want the opposite and
enable the global option by default but make individual sub-options
W=1 or W=2 if there is too much undesired output.

> > I think the two approaches are orthogonal, and I would like to
> > see both happening as much as possible:
> >
> > - any warning flag in the W=1 set (including many things implied
> >   by -Wextra that have or should have their own flags) that
> >   only causes a few lines of output should just be enabled by
> >   default after we address the warnings
>
> Is there currently any simple way to get statistics about how many
> actual warnings there are per warnings flag in W=1? W=1 on the tree as
> a whole does not look good, but maybe there is some low hanging fruit
> and we can quickly promote some from W=1 to default?

I have done this a few times in the past, essentially building a
few hundred randconfig kernels across multiple architectures
and then processing the output in a script. I usually treat a
file:line:warning tuple as a single instance and then count
how many there are.

> > - Code with maintainers that care should have a way to enable
> >   the entire W=1 set per directory or per file after addressing all
> >   the warnings they do see, with new flags only getting added to
> >   W=1 when they don't cause regressions.
>
> Yes, this is what i'm trying to push forward here. I don't
> particularly care how it happen, so if somebody comes up with a
> generally acceptable idea, i will go away and implement it.

I actually have a set of patches that I started a while ago to
move the logic from scripts/Makefile.extrawarn into
include/linux/warnings.h, using '_Pragma("GCC diagnostic ...")'
with some infrastructure around it, to also allow drivers to
set the level as well as individual warnings when needed.

I never managed to get that patch series into a state for submission
so far, with a few things that need to be addressed first:

- any Makefile that changes warning options needs to be
  converted to use macro syntax

- I need to check that the patches don't accidentally disable
  warnings that are currently enabled (this is harder than
  checking the reverse)

- some specific warnings have problems with this new method
  for options that control multiple other options.

        Arnd

  reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-01  1:12 [RFC PATCH net-next v2 0/2] driver/net/ethernet W=1 by default Andrew Lunn
2020-10-01  1:12 ` [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today Andrew Lunn
2020-10-01 23:09   ` Nick Desaulniers
2020-10-02  1:44     ` Andrew Lunn
2020-10-02 12:20       ` Arnd Bergmann
2020-10-02 12:51         ` Andrew Lunn
2020-10-02 13:15           ` Arnd Bergmann [this message]
2020-10-12  1:00         ` Masahiro Yamada
2020-10-12  8:05           ` Arnd Bergmann
2020-10-05 17:31       ` Nick Desaulniers
2020-10-05 19:49         ` Andrew Lunn
2020-10-05 20:03           ` Arnd Bergmann
2020-10-05 21:08             ` Andrew Lunn
2020-10-11 13:03               ` Masahiro Yamada
2020-10-12  8:11                 ` Arnd Bergmann
2020-10-16 14:12                 ` Andrew Lunn
     [not found]                   ` <CAK8P3a1nBhmf1PQwHHbEjiVgRTXi4UuJAbwuK92CKEbR=yKGWw@mail.gmail.com>
2020-10-17 14:57                     ` Andrew Lunn
2020-10-02 11:08   ` kernel test robot
2020-10-01  1:12 ` [PATCH net-next v2 2/2] driver/net/ethernet: Sign up for W=1 as defined on 20200930 Andrew Lunn

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=CAK8P3a1Cyxo4mt2Kug92EvBhZJt2X6ct0_8JbZgo0zf0GSuanA@mail.gmail.com \
    --to=arnd@arndb.de \
    --cc=andrew@lunn.ch \
    --cc=clang-built-linux@googlegroups.com \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=masahiroy@kernel.org \
    --cc=michal.lkml@markovi.net \
    --cc=ndesaulniers@google.com \
    --cc=netdev@vger.kernel.org \
    --cc=rohitm@chelsio.com \
    /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

Linux-KBuild Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kbuild/0 linux-kbuild/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kbuild linux-kbuild/ https://lore.kernel.org/linux-kbuild \
		linux-kbuild@vger.kernel.org
	public-inbox-index linux-kbuild

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kbuild


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git