Re: [PATCH] lib/string: shrink lib/string.i via IWYU

From: Nick Desaulniers <ndesaulniers@google.com>
To: Al Viro <viro@zeniv.linux.org.uk>, Andy Shevchenko <andy@kernel.org>
Cc: tanzirh@google.com, Kees Cook <keescook@chromium.org>,
	 linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
	 Nick DeSaulniers <nnn@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	llvm@lists.linux.dev
Subject: Re: [PATCH] lib/string: shrink lib/string.i via IWYU
Date: Tue, 5 Dec 2023 14:20:55 -0800	[thread overview]
Message-ID: <CAKwvOdm3PrKhGi9DuQvaYN6O-R-EtGQ=BhFm7D4ZPDqU_ZzUkA@mail.gmail.com> (raw)
In-Reply-To: <20231205221521.GH1674809@ZenIV>

On Tue, Dec 5, 2023 at 2:15 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Dec 06, 2023 at 12:01:56AM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 05, 2023 at 01:51:10PM -0800, Nick Desaulniers wrote:
> > > On Tue, Dec 5, 2023 at 1:38 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > > > On Tue, Dec 05, 2023 at 08:58:53PM +0000, tanzirh@google.com wrote:
> >
> > ...
> >
> > > > > IWYU is implemented using the IWYUScripts github repository which is a tool that is
> > > > > currently undergoing development. These changes seek to improve build times.
> > > > >
> > > > > This change to lib/string.c resulted in a preprocessed size of
> > > > > lib/string.i from 26371 lines to 5232 lines (-80%).
> > > >
> > > > It also breeds includes of asm/*.h, by the look of the output, which is
> > > > not a good thing in general ;-/  E.g. #include <asm/uaccess.h> *anywhere*
> > > > outside of linux/uaccess.h is a bad idea.
> > >
> > > It's not clear to me when it's ok to #include <asm/*.h>.  Is there a
> > > convention here that I'm missing?
> >
> > The mandatory ones can be used, but not all of them.
> > In some cases you even must include asm and not linux
> > (unaligned.h, byteorder.h, maybe others...).
> >
> > As I told, it comes with experience, we lack of the
> > respective documentation (or file which is good for
> > automation checks, like with IWYU).
>
> It would certainly be nice to have such information in the tree;
> "where should I pick $SYMBOL from?" is something one needs to
> find out often enough.  To a large extent it's covered by "where
> in include/*.h do we have it defined?", but that's not all there
> is to it.  E.g. "get_user() => use linux/uaccess.h".
>
> There's also stuff like "$SYMBOL should not be used outside of arch/*
> and include/*, better use $OTHER_SYMBOL", etc.

That's basically one of the tables we maintain.
https://github.com/ClangBuiltLinux/IWYUScripts/blob/main/symbol.imp

Of course, such a table is a living document; whether it resides in
tree or out of tree eventually I don't particularly care either way.
There are outstanding questions like "is it even possible to
autogenerate such a table (vs manual curation)" and "are the ones
we've coded so far correct?"  I don't expect to have all of the
answers with the initial implementation, but instead to collect
feedback from kernel devs and iterate from there.  Very helpful
responses in this thread so far; we appreciate it.

I suspect that folks that have played with IWYU in the past
encountered great difficulty since these override tables are poorly
documented, and the out of the box defaults assume more of a userspace
layout for common definitions (which our script which wraps IWYU
disables).
-- 
Thanks,
~Nick Desaulniers