[PATCH] kbuild: compile-test global headers to ensure they are self-contained

[PATCH] kbuild: compile-test global headers to ensure they are self-contained

[Masahiro Yamada]

Make as many headers self-contained as possible so that they can be
included without relying on a specific include order.

This commit compiles only a few headers, but it is a good start point.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 Makefile         |  1 +
 include/Makefile | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)
 create mode 100644 include/Makefile

diff --git a/Makefile b/Makefile
index c23f5e8381ad..82c1722dd9e9 100644
--- a/Makefile
+++ b/Makefile
@@ -610,6 +610,7 @@ drivers-y	:= drivers/ sound/
 drivers-$(CONFIG_SAMPLES) += samples/
 net-y		:= net/
 libs-y		:= lib/
+libs-$(CONFIG_HEADER_TEST) += include/
 core-y		:= usr/
 virt-y		:= virt/
 endif # KBUILD_EXTMOD
diff --git a/include/Makefile b/include/Makefile
new file mode 100644
index 000000000000..68a76ac732c3
--- /dev/null
+++ b/include/Makefile
@@ -0,0 +1,31 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+# extend the test coverage when existing errors are fixed
+
+header-test += linux/w*.h
+header-test += linux/x*.h
+header-test += linux/y*.h
+header-test += ras/*.h
+header-test += soc/at91/*.h
+header-test += soc/bcm2835/*.h
+header-test += soc/mediatek/*.h
+header-test += soc/sa1100/*.h
+
+all-headers = $(patsubst $(srctree)/include/%,%,\
+	    $(wildcard $(addprefix $(srctree)/include/, $(header-test))))
+
+# Do not include directly
+no-header-test += linux/compiler-clang.h
+no-header-test += linux/compiler-gcc.h
+no-header-test += linux/patchkey.h
+no-header-test += linux/rwlock_api_smp.h
+no-header-test += linux/spinlock_types_up.h
+no-header-test += linux/spinlock_up.h
+no-header-test += linux/wimax/debug.h
+no-header-test += rdma/uverbs_named_ioctl.h
+
+# Conditionally included
+no-header-test += linux/byteorder/big_endian.h
+no-header-test += linux/byteorder/little_endian.h
+
+header-test-y = $(filter-out $(no-header-test), $(all-headers))
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] kbuild: compile-test global headers to ensure they are self-contained

[Sam Ravnborg]

Hi Masahiro.

On Sat, Jun 22, 2019 at 01:39:31AM +0900, Masahiro Yamada wrote:
> Make as many headers self-contained as possible so that they can be
> included without relying on a specific include order.
It is very nice finally to get some infrastructure to validate header
files.

But to avoid too many conflicts while including more and more headers
that are selfcontained we really need something that is more
distributed.
So for example all header files in include/drm/* could be in one
Makefile, incl. sub-directories, but the same Makefile would not include
the files in include/soc/

If you just show how ot do it, others can follow-up with the
relevant directories.

	Sam

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] kbuild: compile-test global headers to ensure they are self-contained

[Masahiro Yamada]

Hi Sam,

On Sat, Jun 22, 2019 at 2:51 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Masahiro.
>
> On Sat, Jun 22, 2019 at 01:39:31AM +0900, Masahiro Yamada wrote:
> > Make as many headers self-contained as possible so that they can be
> > included without relying on a specific include order.
> It is very nice finally to get some infrastructure to validate header
> files.
>
> But to avoid too many conflicts while including more and more headers
> that are selfcontained we really need something that is more
> distributed.
> So for example all header files in include/drm/* could be in one
> Makefile, incl. sub-directories, but the same Makefile would not include
> the files in include/soc/
>
> If you just show how ot do it, others can follow-up with the
> relevant directories.


At first, I tried to split Makefile per directory,
and add header-test-y one by one.

I think you expect they look like this:


include/Makefile:

subdir-y += drm
subdir-y += linux
subdir-y += media


include/drm/Makefile:

header-test-y += drm_cache.h
header-test-y += drm_file.h
header-test-y += drm_util.h
...


include/linux/Makefile:

header-test-y += io.h
header-test-y += list.h
header-test-y += kernel.h
header-test-y += types.h
...



This is a straightforward way,
but I see some disadvantages.

Currently, there are more than 4000 headers
under include/.

So, to cover (almost) all of them, we must
list out 4000 entries.

When somebody adds a new header,
he will be very likely to forget to add
header-test-y for it.
So, newly added headers will always
fall off the coverage.


So, I am trying to take an opposite approach.

Add all headers in include/ by wildcard, then
filter-out one that cannot be self-contained.

In my analysis, 70% of headers are already conf-contained.
After some fixups, 95% of headers can become self-contained.

At this moment, the wildcard only covers some directories
or patterns, but my plan is to extend the wildcard gradually.


Please feel free to suggest alternative ideas.


-- 
Best Regards
Masahiro Yamada

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] kbuild: compile-test global headers to ensure they are self-contained

[Sam Ravnborg]

Hi Masahiro

> At first, I tried to split Makefile per directory,
> and add header-test-y one by one.
> 
> I think you expect they look like this:
> 
> 
> include/Makefile:
> 
> subdir-y += drm
> subdir-y += linux
> subdir-y += media

So far we agree.

> include/drm/Makefile:
> 
> header-test-y += drm_cache.h
> header-test-y += drm_file.h
> header-test-y += drm_util.h
> ...
On this level it would be better to do:
header-test-y += $(call find_all_header_files)

# drm files that are not self-contained
header-test-n += drm_legacy.h

Likewise for all subdirs below include/
and include/linux should maybe be split up a little too.
Maybe include/uapi/ would need to be slipt a little.

Then we have a manageable number of Makefiles.
New header files are automatically checked and
we have a list of files to fix.
If for example drm/ have too much failures to a start
then we can add that directory to the higler level Makefile later.

The above is more or less what you already started,
but the difference is that we have it pushed down one or two
directories.

The header-test-n logic could be moved to the generic part,
and a helper could be made to find all header files.
Then the Makefiles would be more or less trival, with all the
Kbuild magic hidden away.

> In my analysis, 70% of headers are already conf-contained.
> After some fixups, 95% of headers can become self-contained.

Sounds good. And we do not want 100%, but close...

	Sam

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] kbuild: compile-test global headers to ensure they are self-contained

[Sam Ravnborg]

Hi Masahiro
> > ...
> On this level it would be better to do:
> header-test-y += $(call find_all_header_files)
> 
> # drm files that are not self-contained
> header-test-n += drm_legacy.h

I made a quick hack on top of your patch to demonstrate
the ideas.
See patch below.

When all header files below include/drm are self-contained it will be a
single line:

    header-test-y += $(all_headers_with_subdir)

But for now I skipped the subdirs as they was not in too good shape.

My gmake foo escapted me tonight, so $(call all_headers_with_subdir)
did not really do what I wanted. Which is why I use the macro direct.
The main part is that we have support in the generic part to find the
header files and to filter out header files we do not want to check.

Later we may extend the checking to something more than that they
can build. We could check for CONFIG_ symbols in uapi/ and more.

Another note. Maybe we should name the files Kbuild, as this is what we
usually do in include/*

But I also sometimes regret that I introduced this second name.
Back then the idea was to globally rename Makefile => Kbuild.
But I never advocated this, as the pain was much bigger than the gain.
Another thing to be cleaned up one day maybe...

	Sam


diff --git a/include/Makefile b/include/Makefile
index 68a76ac732c3..09e854d504f6 100644
--- a/include/Makefile
+++ b/include/Makefile
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
+subdir-y += drm
+
 # extend the test coverage when existing errors are fixed
 
 header-test += linux/w*.h
diff --git a/include/drm/Makefile b/include/drm/Makefile
new file mode 100644
index 000000000000..61a762964ef0
--- /dev/null
+++ b/include/drm/Makefile
@@ -0,0 +1,21 @@
+# Verify all header files
+
+header-test-y += $(all_headers)
+
+# Blacklist header files that do not yet pass the test
+# Keep list sorted
+header-test-n += ati_pcigart.h
+header-test-n += drm_audio_component.h
+header-test-n += drm_auth.h
+header-test-n += drm_debugfs.h
+header-test-n += drm_debugfs_crc.h
+header-test-n += drm_displayid.h
+header-test-n += drm_fb_cma_helper.h
+header-test-n += drm_fixed.h
+header-test-n += drm_format_helper.h
+header-test-n += drm_lease.h
+header-test-n += drm_legacy.h
+header-test-n += drm_plane_helper.h
+header-test-n += drm_rect.h
+header-test-n += i915_component.h
+header-test-n += intel-gtt.h
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 0ae07e83d393..df29c65c6490 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -66,6 +66,20 @@ define filechk
 	fi
 endef
 
+######
+# Support functions for header-test
+define all_headers
+	$(patsubst $(srctree)/$(src)/%,%,	\
+	$(wildcard $(srctree)/$(src)/*.h))
+endef
+
+define all_headers_with_subdir
+	$(patsubst $(srctree)/$(src)/%,%,	\
+	$(shell find $(srctree)/$(src)/ '*.h'))
+endef
+
+
+
 ######
 # gcc support functions
 # See documentation in Documentation/kbuild/makefiles.txt
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 3e630fcaffd1..e2f765e9d1e1 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -67,6 +67,7 @@ extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
 endif
 
 # Test self-contained headers
+header-test-y := $(filter-out $(header-test-n), $(header-test-y))
 extra-$(CONFIG_HEADER_TEST) += $(patsubst %.h,%.hdrtest.o,$(header-test-y))
 
 # Add subdir path

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] kbuild: compile-test global headers to ensure they are self-contained

[Sam Ravnborg]

> 
> When all header files below include/drm are self-contained it will be a
> single line:
> 
>     header-test-y += $(all_headers_with_subdir)
In reality it will likely be the above, and then a list of 

header-test-n += foo.h

For the header files that we for one or the other reason do not want to
make self-contained.
It would be nice to have the list of ignored files close to their home
and not a full list in one Makefile in include/

> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 3e630fcaffd1..e2f765e9d1e1 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -67,6 +67,7 @@ extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
>  endif
>  
>  # Test self-contained headers
> +header-test-y := $(filter-out $(header-test-n), $(header-test-y))
This part should include the logic to filter out duplicates too.
I think we may do something wrong if the same header is listed twice.

We could also extend this with a check that all files in header-test-n
exits.

	Sam

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] kbuild: compile-test global headers to ensure they are self-contained

[Masahiro Yamada]

Hi Sam,

On Tue, Jun 25, 2019 at 3:11 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> >
> > When all header files below include/drm are self-contained it will be a
> > single line:
> >
> >     header-test-y += $(all_headers_with_subdir)
> In reality it will likely be the above, and then a list of
>
> header-test-n += foo.h
>
> For the header files that we for one or the other reason do not want to
> make self-contained.
> It would be nice to have the list of ignored files close to their home
> and not a full list in one Makefile in include/
>
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 3e630fcaffd1..e2f765e9d1e1 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -67,6 +67,7 @@ extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
> >  endif
> >
> >  # Test self-contained headers
> > +header-test-y := $(filter-out $(header-test-n), $(header-test-y))
> This part should include the logic to filter out duplicates too.
> I think we may do something wrong if the same header is listed twice.
>
> We could also extend this with a check that all files in header-test-n
> exits.
>
>         Sam

Thanks for your comments.

Some followups:

[1] I prefer 'header-test-' to 'header-test-n'
for excluding headers.
In some places, it will be useful to
be able to write like this:
header-test-$(CONFIG_FOO) += foo.h


[2] I proposed somewhat generalized header-test-pattern-y instead of
providing both of 'all_headers' and 'all_headers_with_subdir'

BTW, "all headers" should be added with care.
scripts/Makefile.asm-generic and scripts/Makefile.headersinst
cater to removing stale headers.
But, we do not explicitly clean other headers.
We always be careful about potential matching to stale headers.


[3] I tried both 'one big single Makefile' and
    'each Makefile in sub-directories'

I am slightly in favor of the former. Maybe I could be wrong,
and we may switch to the other approach.
But, I'd like to start with a single Makefile, and see how bad it is.


I sent v2:
https://patchwork.kernel.org/project/linux-kbuild/list/?series=138507


-- 
Best Regards
Masahiro Yamada

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel