diff mbox series

modpost: allow modpost to fail on warnings

Message ID 20200918215010.250580-1-pierre-louis.bossart@linux.intel.com
State New
Headers show
Series modpost: allow modpost to fail on warnings | expand

Commit Message

Pierre-Louis Bossart Sept. 18, 2020, 9:50 p.m. UTC
From: Filipe Brandenburger <filbranden@google.com>

Set KBUILD_MODPOST_FAIL_ON_WARNINGS to a non-empty value to make the
kbuild fail when modpost generates any warnings. This will avoid
misses such as [1] where the SOF CI did not catch a missing module
license.

This was initially contributed in 2016 [2], rebase/clean-ups and tests
by Pierre Bossart.

Test example:
$ KBUILD_MODPOST_FAIL_ON_WARNINGS=1 make
  GEN     Makefile
  DESCEND  objtool
  CALL    sof-dev/scripts/atomic/check-atomics.sh
  CALL    sof-dev/scripts/checksyscalls.sh
  CHK     include/generated/compile.h
  MODPOST Module.symvers
Kernel: arch/x86/boot/bzImage is ready  (#13)
WARNING: modpost: missing MODULE_LICENSE() in sound/soc/intel/boards/snd-soc-sof-sdw.o
make[2]: *** [sof-dev/scripts/Makefile.modpost:114: Module.symvers] Error 2

[1] https://lkml.org/lkml/2020/9/17/2343
[2] https://patchwork.kernel.org/patch/8343431/

Signed-off-by: Filipe Brandenburger <filbranden@google.com>
Cc: Greg Thelen <gthelen@google.com>
Cc: Michael Davidson <md@google.com>
Cc: Eugene Surovegin <surovegin@google.com>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Co-developed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 Documentation/kbuild/kbuild.rst |  5 +++++
 scripts/Makefile.modpost        |  5 ++++-
 scripts/mod/modpost.c           | 12 +++++++++++-
 3 files changed, 20 insertions(+), 2 deletions(-)

Comments

Masahiro Yamada Sept. 19, 2020, 6:21 a.m. UTC | #1
On Sat, Sep 19, 2020 at 6:50 AM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
> From: Filipe Brandenburger <filbranden@google.com>
>
> Set KBUILD_MODPOST_FAIL_ON_WARNINGS to a non-empty value to make the
> kbuild fail when modpost generates any warnings. This will avoid
> misses such as [1] where the SOF CI did not catch a missing module
> license.
>
> This was initially contributed in 2016 [2], rebase/clean-ups and tests
> by Pierre Bossart.
>
> Test example:
> $ KBUILD_MODPOST_FAIL_ON_WARNINGS=1 make
>   GEN     Makefile
>   DESCEND  objtool
>   CALL    sof-dev/scripts/atomic/check-atomics.sh
>   CALL    sof-dev/scripts/checksyscalls.sh
>   CHK     include/generated/compile.h
>   MODPOST Module.symvers
> Kernel: arch/x86/boot/bzImage is ready  (#13)
> WARNING: modpost: missing MODULE_LICENSE() in sound/soc/intel/boards/snd-soc-sof-sdw.o
> make[2]: *** [sof-dev/scripts/Makefile.modpost:114: Module.symvers] Error 2


I think [1] should be an error instead of a warning
by default.



> [1] https://lkml.org/lkml/2020/9/17/2343
> [2] https://patchwork.kernel.org/patch/8343431/
>
> Signed-off-by: Filipe Brandenburger <filbranden@google.com>
> Cc: Greg Thelen <gthelen@google.com>
> Cc: Michael Davidson <md@google.com>
> Cc: Eugene Surovegin <surovegin@google.com>
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Co-developed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  Documentation/kbuild/kbuild.rst |  5 +++++
>  scripts/Makefile.modpost        |  5 ++++-
>  scripts/mod/modpost.c           | 12 +++++++++++-
>  3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/kbuild/kbuild.rst b/Documentation/kbuild/kbuild.rst
> index 2d1fc03d346e..cc102aad8619 100644
> --- a/Documentation/kbuild/kbuild.rst
> +++ b/Documentation/kbuild/kbuild.rst
> @@ -229,6 +229,11 @@ KBUILD_MODPOST_WARN can be set to avoid errors in case of undefined
>  symbols in the final module linking stage. It changes such errors
>  into warnings.
>
> +KBUILD_MODPOST_FAIL_ON_WARNINGS
> +-------------------------------
> +KBUILD_MODPOST_FAIL_ON_WARNINGS can be set to turn all warnings into
> +errors in the final module linking stage.
> +
>  KBUILD_MODPOST_NOFINAL
>  ----------------------
>  KBUILD_MODPOST_NOFINAL can be set to skip the final link of modules.
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index f54b6ac37ac2..69297cd6f8ce 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -34,6 +34,8 @@
>
>  # KBUILD_MODPOST_WARN can be set to avoid error out in case of undefined
>  # symbols in the final module linking stage
> +# KBUILD_MODPOST_FAIL_ON_WARNINGS can be set to fail whenever modpost
> +# generates warnings
>  # KBUILD_MODPOST_NOFINAL can be set to skip the final link of modules.
>  # This is solely useful to speed up test compiles
>
> @@ -47,7 +49,8 @@ MODPOST = scripts/mod/modpost                                                         \
>         $(if $(CONFIG_MODVERSIONS),-m)                                                  \
>         $(if $(CONFIG_MODULE_SRCVERSION_ALL),-a)                                        \
>         $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)                                  \
> -       $(if $(KBUILD_MODPOST_WARN),-w) \
> +       $(if $(KBUILD_MODPOST_WARN),-w)                                                 \
> +       $(if $(KBUILD_MODPOST_FAIL_ON_WARNINGS),-F)                                     \
>         -o $@
>
>  ifdef MODPOST_VMLINUX
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 69341b36f271..422f1cfca289 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -39,6 +39,9 @@ static int sec_mismatch_fatal = 0;
>  static int ignore_missing_files;
>  /* If set to 1, only warn (instead of error) about missing ns imports */
>  static int allow_missing_ns_imports;
> +/* Turn warnings into errors */
> +static int fail_on_warnings;
> +static int warnings_count;
>
>  enum export {
>         export_plain,      export_unused,     export_gpl,
> @@ -59,6 +62,7 @@ modpost_log(enum loglevel loglevel, const char *fmt, ...)
>         switch (loglevel) {
>         case LOG_WARN:
>                 fprintf(stderr, "WARNING: ");
> +               warnings_count++;
>                 break;
>         case LOG_ERROR:
>                 fprintf(stderr, "ERROR: ");
> @@ -2559,7 +2563,7 @@ int main(int argc, char **argv)
>         struct dump_list *dump_read_start = NULL;
>         struct dump_list **dump_read_iter = &dump_read_start;
>
> -       while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) {
> +       while ((opt = getopt(argc, argv, "ei:mnT:o:awEFNd:")) != -1) {
>                 switch (opt) {
>                 case 'e':
>                         external_module = 1;
> @@ -2588,6 +2592,9 @@ int main(int argc, char **argv)
>                 case 'w':
>                         warn_unresolved = 1;
>                         break;
> +               case 'F':
> +                       fail_on_warnings = 1;
> +                       break;
>                 case 'E':
>                         sec_mismatch_fatal = 1;
>                         break;
> @@ -2671,5 +2678,8 @@ int main(int argc, char **argv)
>
>         free(buf.p);
>
> +       if (fail_on_warnings && warnings_count)
> +               err |= 2;
> +
>         return err;
>  }
> --
> 2.25.1
>
Pierre-Louis Bossart Sept. 21, 2020, 2:50 p.m. UTC | #2
Thanks for the review,

>> Set KBUILD_MODPOST_FAIL_ON_WARNINGS to a non-empty value to make the
>> kbuild fail when modpost generates any warnings. This will avoid
>> misses such as [1] where the SOF CI did not catch a missing module
>> license.
>>
>> This was initially contributed in 2016 [2], rebase/clean-ups and tests
>> by Pierre Bossart.
>>
>> Test example:
>> $ KBUILD_MODPOST_FAIL_ON_WARNINGS=1 make
>>    GEN     Makefile
>>    DESCEND  objtool
>>    CALL    sof-dev/scripts/atomic/check-atomics.sh
>>    CALL    sof-dev/scripts/checksyscalls.sh
>>    CHK     include/generated/compile.h
>>    MODPOST Module.symvers
>> Kernel: arch/x86/boot/bzImage is ready  (#13)
>> WARNING: modpost: missing MODULE_LICENSE() in sound/soc/intel/boards/snd-soc-sof-sdw.o
>> make[2]: *** [sof-dev/scripts/Makefile.modpost:114: Module.symvers] Error 2
> 
> 
> I think [1] should be an error instead of a warning
> by default.

would the following patch be what you have in mind?

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 422f1cfca289..ae1eb67aa0f2 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2018,7 +2018,7 @@ static void read_symbols(const char *modname)
         if (!mod->is_vmlinux) {
                 license = get_modinfo(&info, "license");
                 if (!license)
-                       warn("missing MODULE_LICENSE() in %s\n", modname);
+                       error("missing MODULE_LICENSE() in %s\n", modname);
                 while (license) {
                         if (license_is_gpl_compatible(license))
                                 mod->gpl_compatible = 1;


If yes, also wondering if we can still add the option to treat warnings 
as errors as an opt-in behavior?

Thanks!
-Pierre
Masahiro Yamada Sept. 24, 2020, 5:22 p.m. UTC | #3
On Mon, Sep 21, 2020 at 11:51 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
> Thanks for the review,
>
> >> Set KBUILD_MODPOST_FAIL_ON_WARNINGS to a non-empty value to make the
> >> kbuild fail when modpost generates any warnings. This will avoid
> >> misses such as [1] where the SOF CI did not catch a missing module
> >> license.
> >>
> >> This was initially contributed in 2016 [2], rebase/clean-ups and tests
> >> by Pierre Bossart.
> >>
> >> Test example:
> >> $ KBUILD_MODPOST_FAIL_ON_WARNINGS=1 make
> >>    GEN     Makefile
> >>    DESCEND  objtool
> >>    CALL    sof-dev/scripts/atomic/check-atomics.sh
> >>    CALL    sof-dev/scripts/checksyscalls.sh
> >>    CHK     include/generated/compile.h
> >>    MODPOST Module.symvers
> >> Kernel: arch/x86/boot/bzImage is ready  (#13)
> >> WARNING: modpost: missing MODULE_LICENSE() in sound/soc/intel/boards/snd-soc-sof-sdw.o
> >> make[2]: *** [sof-dev/scripts/Makefile.modpost:114: Module.symvers] Error 2
> >
> >
> > I think [1] should be an error instead of a warning
> > by default.
>
> would the following patch be what you have in mind?


No.
error() does not exist.

merror() exists, but the difference from warn()
is just a prefix.

If any error happens, modpost should return the error code.





> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 422f1cfca289..ae1eb67aa0f2 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2018,7 +2018,7 @@ static void read_symbols(const char *modname)
>          if (!mod->is_vmlinux) {
>                  license = get_modinfo(&info, "license");
>                  if (!license)
> -                       warn("missing MODULE_LICENSE() in %s\n", modname);
> +                       error("missing MODULE_LICENSE() in %s\n", modname);
>                  while (license) {
>                          if (license_is_gpl_compatible(license))
>                                  mod->gpl_compatible = 1;
>
>
> If yes, also wondering if we can still add the option to treat warnings
> as errors as an opt-in behavior?


I want to add a new option only when it is necessary to do so.

I am not sure which warnings are real warnings.


> Thanks!
> -Pierre
Pierre-Louis Bossart Sept. 24, 2020, 6:13 p.m. UTC | #4
>>> I think [1] should be an error instead of a warning
>>> by default.
>>
>> would the following patch be what you have in mind?
> 
> 
> No.
> error() does not exist.
> 
> merror() exists, but the difference from warn()
> is just a prefix.
> 
> If any error happens, modpost should return the error code.

Sorry, I am not able to understand your recommendation. Existing code 
which calls merror() does not exit with an error code, e.g.

static void sym_update_namespace(const char *symname, const char *namespace)
{
	struct symbol *s = find_symbol(symname);

	/*
	 * That symbol should have been created earlier and thus this is
	 * actually an assertion.
	 */
	if (!s) {
		merror("Could not update namespace(%s) for symbol %s\n",
		       namespace, symname);
		return;
	}

What would be suggestion be then in this case?

change all functions to return -EINVAL after each use of merror() or 
something?

Or just add an exit(1) after all uses of merror()?

>> If yes, also wondering if we can still add the option to treat warnings
>> as errors as an opt-in behavior?
> 
> 
> I want to add a new option only when it is necessary to do so.
> 
> I am not sure which warnings are real warnings.

That was the point of the suggested option, treat all warnings as errors 
and ignore the warning if it's no big deal. it's standard with gcc 
-Werror, it's be good to extend this to modpost - or what would be the 
drawback of doing so?
diff mbox series

Patch

diff --git a/Documentation/kbuild/kbuild.rst b/Documentation/kbuild/kbuild.rst
index 2d1fc03d346e..cc102aad8619 100644
--- a/Documentation/kbuild/kbuild.rst
+++ b/Documentation/kbuild/kbuild.rst
@@ -229,6 +229,11 @@  KBUILD_MODPOST_WARN can be set to avoid errors in case of undefined
 symbols in the final module linking stage. It changes such errors
 into warnings.
 
+KBUILD_MODPOST_FAIL_ON_WARNINGS
+-------------------------------
+KBUILD_MODPOST_FAIL_ON_WARNINGS can be set to turn all warnings into
+errors in the final module linking stage.
+
 KBUILD_MODPOST_NOFINAL
 ----------------------
 KBUILD_MODPOST_NOFINAL can be set to skip the final link of modules.
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index f54b6ac37ac2..69297cd6f8ce 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -34,6 +34,8 @@ 
 
 # KBUILD_MODPOST_WARN can be set to avoid error out in case of undefined
 # symbols in the final module linking stage
+# KBUILD_MODPOST_FAIL_ON_WARNINGS can be set to fail whenever modpost
+# generates warnings
 # KBUILD_MODPOST_NOFINAL can be set to skip the final link of modules.
 # This is solely useful to speed up test compiles
 
@@ -47,7 +49,8 @@  MODPOST = scripts/mod/modpost								\
 	$(if $(CONFIG_MODVERSIONS),-m)							\
 	$(if $(CONFIG_MODULE_SRCVERSION_ALL),-a)					\
 	$(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)					\
-	$(if $(KBUILD_MODPOST_WARN),-w) \
+	$(if $(KBUILD_MODPOST_WARN),-w)							\
+	$(if $(KBUILD_MODPOST_FAIL_ON_WARNINGS),-F)					\
 	-o $@
 
 ifdef MODPOST_VMLINUX
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 69341b36f271..422f1cfca289 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -39,6 +39,9 @@  static int sec_mismatch_fatal = 0;
 static int ignore_missing_files;
 /* If set to 1, only warn (instead of error) about missing ns imports */
 static int allow_missing_ns_imports;
+/* Turn warnings into errors */
+static int fail_on_warnings;
+static int warnings_count;
 
 enum export {
 	export_plain,      export_unused,     export_gpl,
@@ -59,6 +62,7 @@  modpost_log(enum loglevel loglevel, const char *fmt, ...)
 	switch (loglevel) {
 	case LOG_WARN:
 		fprintf(stderr, "WARNING: ");
+		warnings_count++;
 		break;
 	case LOG_ERROR:
 		fprintf(stderr, "ERROR: ");
@@ -2559,7 +2563,7 @@  int main(int argc, char **argv)
 	struct dump_list *dump_read_start = NULL;
 	struct dump_list **dump_read_iter = &dump_read_start;
 
-	while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) {
+	while ((opt = getopt(argc, argv, "ei:mnT:o:awEFNd:")) != -1) {
 		switch (opt) {
 		case 'e':
 			external_module = 1;
@@ -2588,6 +2592,9 @@  int main(int argc, char **argv)
 		case 'w':
 			warn_unresolved = 1;
 			break;
+		case 'F':
+			fail_on_warnings = 1;
+			break;
 		case 'E':
 			sec_mismatch_fatal = 1;
 			break;
@@ -2671,5 +2678,8 @@  int main(int argc, char **argv)
 
 	free(buf.p);
 
+	if (fail_on_warnings && warnings_count)
+		err |= 2;
+
 	return err;
 }