[RFC,1/3] kbuild: add support for ensuring headers are self-contained

Submitted by Jani Nikula on May 16, 2019, 7:48 p.m.

Details

Message ID 20190516194818.29230-1-jani.nikula@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Jani Nikula May 16, 2019, 7:48 p.m.
Sometimes it's useful to be able to explicitly ensure certain headers
remain self-contained, i.e. that they are compilable as standalone
units, by including and/or forward declaring everything they depend on.

Add special target header-test-y where individual Makefiles can add
headers to be tested if CONFIG_HEADER_TEST is enabled. This will
generate a dummy C file per header that gets built as part of extra-y.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Michal Marek <michal.lkml@markovi.net>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 Documentation/kbuild/makefiles.txt |  7 +++++++
 init/Kconfig                       |  9 +++++++++
 scripts/Makefile.build             | 10 ++++++++++
 scripts/Makefile.lib               |  3 +++
 4 files changed, 29 insertions(+)

Patch hide | download patch | download mbox

diff --git a/Documentation/kbuild/makefiles.txt b/Documentation/kbuild/makefiles.txt
index 03c065855eaf..73df58e5ea0c 100644
--- a/Documentation/kbuild/makefiles.txt
+++ b/Documentation/kbuild/makefiles.txt
@@ -1036,6 +1036,13 @@  When kbuild executes, the following steps are followed (roughly):
 	In this example, extra-y is used to list object files that
 	shall be built, but shall not be linked as part of built-in.a.
 
+    header-test-y
+
+	header-test-y specifies headers (*.h) in the current directory that
+	should be compile tested to ensure they are self-contained,
+	i.e. compilable as standalone units. If CONFIG_HEADER_TEST is enabled,
+	this autogenerates dummy sources to include the headers, and builds them
+	as part of extra-y.
 
 --- 6.7 Commands useful for building a boot image
 
diff --git a/init/Kconfig b/init/Kconfig
index 4592bf7997c0..d91b157201b1 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -95,6 +95,15 @@  config COMPILE_TEST
 	  here. If you are a user/distributor, say N here to exclude useless
 	  drivers to be distributed.
 
+config HEADER_TEST
+	bool "Compile test headers that should be standalone compilable"
+	help
+	  Compile test headers listed in header-test-y target to ensure they are
+	  self-contained, i.e. compilable as standalone units.
+
+	  If you are a developer or tester and want to ensure the requested
+	  headers are self-contained, say Y here. Otherwise, choose N.
+
 config LOCALVERSION
 	string "Local version - append to kernel release"
 	help
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 76ca30cc4791..4d4bf698467a 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -291,6 +291,16 @@  quiet_cmd_cc_lst_c = MKLST   $@
 $(obj)/%.lst: $(src)/%.c FORCE
 	$(call if_changed_dep,cc_lst_c)
 
+# Dummy C sources for header test (header-test-y target)
+# ---------------------------------------------------------------------------
+
+quiet_cmd_header_test = HDRTEST $@
+      cmd_header_test = echo "\#include \"$(<F)\"" > $@
+
+# FIXME: would be nice to be able to limit this implicit rule to header-test-y
+$(obj)/%.header_test.c: $(src)/%.h FORCE
+	$(call if_changed,header_test)
+
 # Compile assembler sources (.S)
 # ---------------------------------------------------------------------------
 
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 8a1f64f17740..c2839de06485 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -66,6 +66,9 @@  extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))
 extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
 endif
 
+# Test self-contained headers
+extra-$(CONFIG_HEADER_TEST) += $(patsubst %.h,%.header_test.o,$(header-test-y))
+
 # Add subdir path
 
 extra-y		:= $(addprefix $(obj)/,$(extra-y))

Comments

Quoting Jani Nikula (2019-05-16 20:48:16)
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 8a1f64f17740..c2839de06485 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -66,6 +66,9 @@ extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))
>  extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
>  endif
>  
> +# Test self-contained headers
> +extra-$(CONFIG_HEADER_TEST) += $(patsubst %.h,%.header_test.o,$(header-test-y))

This didn't get pulled into clean-files.

clean-files-$(CONFIG_HEADER_TEST) += $(patsubst %.h,%.header_test.c,$(header-test-y))
?

Not enough. Nor is clean-files-y +=...

And it should also be put into the global gitignore I think.
-Chris
On Fri, May 17, 2019 at 4:48 AM Jani Nikula <jani.nikula@intel.com> wrote:
>
> Sometimes it's useful to be able to explicitly ensure certain headers
> remain self-contained, i.e. that they are compilable as standalone
> units, by including and/or forward declaring everything they depend on.
>
> Add special target header-test-y where individual Makefiles can add
> headers to be tested if CONFIG_HEADER_TEST is enabled. This will
> generate a dummy C file per header that gets built as part of extra-y.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Michal Marek <michal.lkml@markovi.net>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  Documentation/kbuild/makefiles.txt |  7 +++++++
>  init/Kconfig                       |  9 +++++++++
>  scripts/Makefile.build             | 10 ++++++++++
>  scripts/Makefile.lib               |  3 +++
>  4 files changed, 29 insertions(+)
>
> diff --git a/Documentation/kbuild/makefiles.txt b/Documentation/kbuild/makefiles.txt
> index 03c065855eaf..73df58e5ea0c 100644
> --- a/Documentation/kbuild/makefiles.txt
> +++ b/Documentation/kbuild/makefiles.txt
> @@ -1036,6 +1036,13 @@ When kbuild executes, the following steps are followed (roughly):
>         In this example, extra-y is used to list object files that
>         shall be built, but shall not be linked as part of built-in.a.
>
> +    header-test-y
> +
> +       header-test-y specifies headers (*.h) in the current directory that
> +       should be compile tested to ensure they are self-contained,
> +       i.e. compilable as standalone units. If CONFIG_HEADER_TEST is enabled,
> +       this autogenerates dummy sources to include the headers, and builds them
> +       as part of extra-y.
>
>  --- 6.7 Commands useful for building a boot image
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 4592bf7997c0..d91b157201b1 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -95,6 +95,15 @@ config COMPILE_TEST
>           here. If you are a user/distributor, say N here to exclude useless
>           drivers to be distributed.
>
> +config HEADER_TEST
> +       bool "Compile test headers that should be standalone compilable"
> +       help
> +         Compile test headers listed in header-test-y target to ensure they are
> +         self-contained, i.e. compilable as standalone units.
> +
> +         If you are a developer or tester and want to ensure the requested
> +         headers are self-contained, say Y here. Otherwise, choose N.
> +
>  config LOCALVERSION
>         string "Local version - append to kernel release"
>         help
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 76ca30cc4791..4d4bf698467a 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -291,6 +291,16 @@ quiet_cmd_cc_lst_c = MKLST   $@
>  $(obj)/%.lst: $(src)/%.c FORCE
>         $(call if_changed_dep,cc_lst_c)
>
> +# Dummy C sources for header test (header-test-y target)
> +# ---------------------------------------------------------------------------
> +
> +quiet_cmd_header_test = HDRTEST $@
> +      cmd_header_test = echo "\#include \"$(<F)\"" > $@
> +
> +# FIXME: would be nice to be able to limit this implicit rule to header-test-y
> +$(obj)/%.header_test.c: $(src)/%.h FORCE
> +       $(call if_changed,header_test)
> +
>  # Compile assembler sources (.S)
>  # ---------------------------------------------------------------------------
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 8a1f64f17740..c2839de06485 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -66,6 +66,9 @@ extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))
>  extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
>  endif
>
> +# Test self-contained headers
> +extra-$(CONFIG_HEADER_TEST) += $(patsubst %.h,%.header_test.o,$(header-test-y))
> +
>  # Add subdir path
>
>  extra-y                := $(addprefix $(obj)/,$(extra-y))
> --
> 2.20.1
>


Thanks, probably we should do this.

At least, this check will be useful
for uapi headers since the kernel does not
test the self-containedness of
exported headers, (then turned out be problematic
later in user-space).

I will take a little time to considier
how far we can extend the idea about
"headers should be self-contained".

Thank you.
On Fri, May 17, 2019 at 5:35 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Jani Nikula (2019-05-16 20:48:16)
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 8a1f64f17740..c2839de06485 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -66,6 +66,9 @@ extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))
> >  extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
> >  endif
> >
> > +# Test self-contained headers
> > +extra-$(CONFIG_HEADER_TEST) += $(patsubst %.h,%.header_test.o,$(header-test-y))
>
> This didn't get pulled into clean-files.
>
> clean-files-$(CONFIG_HEADER_TEST) += $(patsubst %.h,%.header_test.c,$(header-test-y))
> ?
>
> Not enough. Nor is clean-files-y +=...


The correct syntax is 'clean-files +='
instead of 'clean-files-y +='


If Kbuild supports this in the core Makefiles,
we can add "-o -name '*.header_test.c'"
to the top Makefile.



> And it should also be put into the global gitignore I think.


Documentation/dontdiff too.



> -Chris
On Sat, 18 May 2019, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> On Fri, May 17, 2019 at 4:48 AM Jani Nikula <jani.nikula@intel.com> wrote:
>>
>> Sometimes it's useful to be able to explicitly ensure certain headers
>> remain self-contained, i.e. that they are compilable as standalone
>> units, by including and/or forward declaring everything they depend on.
>>
>> Add special target header-test-y where individual Makefiles can add
>> headers to be tested if CONFIG_HEADER_TEST is enabled. This will
>> generate a dummy C file per header that gets built as part of extra-y.
>>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Cc: Michal Marek <michal.lkml@markovi.net>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  Documentation/kbuild/makefiles.txt |  7 +++++++
>>  init/Kconfig                       |  9 +++++++++
>>  scripts/Makefile.build             | 10 ++++++++++
>>  scripts/Makefile.lib               |  3 +++
>>  4 files changed, 29 insertions(+)
>>
>> diff --git a/Documentation/kbuild/makefiles.txt b/Documentation/kbuild/makefiles.txt
>> index 03c065855eaf..73df58e5ea0c 100644
>> --- a/Documentation/kbuild/makefiles.txt
>> +++ b/Documentation/kbuild/makefiles.txt
>> @@ -1036,6 +1036,13 @@ When kbuild executes, the following steps are followed (roughly):
>>         In this example, extra-y is used to list object files that
>>         shall be built, but shall not be linked as part of built-in.a.
>>
>> +    header-test-y
>> +
>> +       header-test-y specifies headers (*.h) in the current directory that
>> +       should be compile tested to ensure they are self-contained,
>> +       i.e. compilable as standalone units. If CONFIG_HEADER_TEST is enabled,
>> +       this autogenerates dummy sources to include the headers, and builds them
>> +       as part of extra-y.
>>
>>  --- 6.7 Commands useful for building a boot image
>>
>> diff --git a/init/Kconfig b/init/Kconfig
>> index 4592bf7997c0..d91b157201b1 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -95,6 +95,15 @@ config COMPILE_TEST
>>           here. If you are a user/distributor, say N here to exclude useless
>>           drivers to be distributed.
>>
>> +config HEADER_TEST
>> +       bool "Compile test headers that should be standalone compilable"
>> +       help
>> +         Compile test headers listed in header-test-y target to ensure they are
>> +         self-contained, i.e. compilable as standalone units.
>> +
>> +         If you are a developer or tester and want to ensure the requested
>> +         headers are self-contained, say Y here. Otherwise, choose N.
>> +
>>  config LOCALVERSION
>>         string "Local version - append to kernel release"
>>         help
>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>> index 76ca30cc4791..4d4bf698467a 100644
>> --- a/scripts/Makefile.build
>> +++ b/scripts/Makefile.build
>> @@ -291,6 +291,16 @@ quiet_cmd_cc_lst_c = MKLST   $@
>>  $(obj)/%.lst: $(src)/%.c FORCE
>>         $(call if_changed_dep,cc_lst_c)
>>
>> +# Dummy C sources for header test (header-test-y target)
>> +# ---------------------------------------------------------------------------
>> +
>> +quiet_cmd_header_test = HDRTEST $@
>> +      cmd_header_test = echo "\#include \"$(<F)\"" > $@
>> +
>> +# FIXME: would be nice to be able to limit this implicit rule to header-test-y
>> +$(obj)/%.header_test.c: $(src)/%.h FORCE
>> +       $(call if_changed,header_test)
>> +
>>  # Compile assembler sources (.S)
>>  # ---------------------------------------------------------------------------
>>
>> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>> index 8a1f64f17740..c2839de06485 100644
>> --- a/scripts/Makefile.lib
>> +++ b/scripts/Makefile.lib
>> @@ -66,6 +66,9 @@ extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))
>>  extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
>>  endif
>>
>> +# Test self-contained headers
>> +extra-$(CONFIG_HEADER_TEST) += $(patsubst %.h,%.header_test.o,$(header-test-y))
>> +
>>  # Add subdir path
>>
>>  extra-y                := $(addprefix $(obj)/,$(extra-y))
>> --
>> 2.20.1
>>
>
>
> Thanks, probably we should do this.
>
> At least, this check will be useful
> for uapi headers since the kernel does not
> test the self-containedness of
> exported headers, (then turned out be problematic
> later in user-space).
>
> I will take a little time to considier
> how far we can extend the idea about
> "headers should be self-contained".

Thanks! Please let me know if/when you need further action from me, I
won't post new versions until then.


BR,
Jani.
Hi Jani,

On Mon, May 20, 2019 at 6:16 PM Jani Nikula <jani.nikula@intel.com> wrote:
>
> >
> > I will take a little time to considier
> > how far we can extend the idea about
> > "headers should be self-contained".
>
> Thanks! Please let me know if/when you need further action from me, I
> won't post new versions until then.


Could you send v2 with the following changes ?


[1] Could you rename *.header_test.c to *.hdrtest.c ?
    (I just thought .header_test.c was a bit too long.)

[2] %.hdrtest.c should not depend on the header

This will avoid unnecessary regeneration of *.hdrtest.c

quiet_cmd_header_test = HDRTEST $@
      cmd_header_test = echo "\#include \"$*.h\"" > $@

$(obj)/%.hdrtest.c:
        $(call cmd,header_test)

[3] Please add '*.hdrtest.c' to  .gitignore, Documentation/dontdiff

[4] Please add '*.hdrtest.c' to 'make clean' (around line 1640 of top Makefile)