Received-SPF: pass (google.com: domain of linux-kernel+bounces-16960-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240104-extable-v2-1-6fdcb64abcb2@google.com> <20240104011419.GN1674809@ZenIV>
In-Reply-To: <20240104011419.GN1674809@ZenIV>
From: Nick Desaulniers <ndesaulniers@google.com>
Date: Thu, 4 Jan 2024 08:45:42 -0800
Message-ID: <CAKwvOdmDh=kAnRVDX-Qhg4KAoPDi7STfU5Ga5f+jZPb8fyyN8Q@mail.gmail.com>
Subject: Re: [PATCH v2] x86/vdso: shrink vdso/extable.i via IWYU
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Tanzir Hasan <tanzirh@google.com>, Andy Lutomirski <luto@kernel.org>, 
	Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, 
	Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org, 
	"H. Peter Anvin" <hpa@zytor.com>, Nick Desaulniers <nnn@google.com>, linux-kernel@vger.kernel.org, 
	Arnd Bergmann <arnd@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 3, 2024 at 5:14=E2=80=AFPM Al Viro <viro@zeniv.linux.org.uk> wr=
ote:
>
> On Thu, Jan 04, 2024 at 12:02:40AM +0000, Tanzir Hasan wrote:
>
> *ugh*
>
> > +#include <linux/stddef.h>
> > +#include <linux/types.h>

Tanzir's tooling is pretty neat; it can print //why// certain indirect
includes were made direct.  In this case:

#include <linux/stddef.h>    // for false, true
#include <linux/types.h>     // for bool

So it's kind of sad that the use of bools like this will depend on two
different headers.  Perhaps it's worth making an include/linux/bool.h
header; at least that naming convention would better match userspace C
programming (I feel the same about not having a stdint.h, too).  Also,
one of the more recent C standards standardized bool, so if we ever
plan to move off -std=3Dgnu11, we will need to adjust the kernel's
definitions of bool anyways.  Doing so in one place rather than two
might be nice.  Adding Arnd, who I think looked at that recently
(can't recall precisely).

>
> Do we have _anything_ that would not want stddef.h?  Seriously -
> NULL, true, false, offsetof...   At that point I'd rather have
> it via -include...

No!  No more -include!  That should never have been allowed in the
kernel.  That pessimizes every translation unit which now must parse
those entire files regardless of whether they are actually used or
not.  And undoing the 3 existing ones is going to be painful.  And we
can't even use precompiled headers easily for those because the x86
kernel uses like 3+ ABIs when building different parts of the kernel
image (and precompiled headers need to have the same compiler flags
for the translation unit as was used to precompile the header).

>
> Are we going to spam that include all over the tree?  Because
> it'd really be just about everything...

We could try to take the tact with these changes of "removing as many
explicit header inclusions as possible while still being able to
build," but I strongly recommend against that.  (Some C or C++
projects do take that approach, some explicitly don't)

Basically, if you can scrape by by building with a minimal number of
headers (because let's say one of the explicit includes happens to
already include stddef.h; that's what I'm referring to as an
"indirect" inclusion or dependency), it makes it harder to refactor
headers in the future, since you can end up with build breakages in .c
files due to such indirect inclusions.  If you make your indirect
includes direct (by explicitly including them, like this patch), then
that becomes a non-issue.

If an indirect dependency is made direct, it doesn't change the number
of tokens to parse, assuming your compiler has the header guard
optimization.

And we don't plan to send patches that make every indirect include
direct unless it results in some amount of reduction in code size for
the parser (as is measured in the commit message of this patch).

I don't buy that include lists are somehow an eyesore, but if they
ever got too long, one could make a new .h file just for that one .c
file which contains the full include list such that the .c file only
includes the one newly created .h file.

--
Anyways, it would be nice to retain these 2 headers explicitly.  One
goal of automating this is that the process and results are
reproducible between two different developers.  If we have automation
that tries to help developers avoid indirect dependencies, and devs
manually touch up the results to remove includes (going back to
relying on indirect dependencies), then someone else runs the
automation, that will result in the manual touch ups being undone.
For this file without too many #ifdef CONFIG_FOO's, it's more obvious
what can be made indirect.  But for other larger translation units, I
worry that having such a posture (of favoring indirect dependencies)
will break randconfigs when attempting to refactor headers or include
lists.

My advice; rely on automation and move on.  (I feel the same about
clang-format btw)
--=20
Thanks,
~Nick Desaulniers