Re: [PATCH 1/2] kbuild: use modpost-args-y to clean up modpost arguments

[Masahiro Yamada]

On Wed, Sep 7, 2022 at 2:26 PM Nick Desaulniers <ndesaulniers@xxxxxxxxxx> wrote:
>
> On Tue, Sep 6, 2022 at 12:35 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
> >
> > Separate the modpost executable (scripts/mod/modpost) and the command
> > parameters for the next commit.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> > ---
> >
> >  Makefile                 |  2 +-
> >  scripts/Makefile.modpost | 22 +++++++++++-----------
> >  2 files changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index aee4f0769d9d..e07236209606 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1908,7 +1908,7 @@ tags TAGS cscope gtags: FORCE
> >  # ---------------------------------------------------------------------------
> >
> >  PHONY += nsdeps
> > -nsdeps: export KBUILD_NSDEPS=1
> > +nsdeps: export KBUILD_NSDEPS=y
> >  nsdeps: modules
> >         $(Q)$(CONFIG_SHELL) $(srctree)/scripts/nsdeps
> >
> > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> > index ceb1d78140e7..87821a5fe16b 100644
> > --- a/scripts/Makefile.modpost
> > +++ b/scripts/Makefile.modpost
> > @@ -38,17 +38,17 @@ __modpost:
> >  include include/config/auto.conf
> >  include $(srctree)/scripts/Kbuild.include
> >
> > -MODPOST = scripts/mod/modpost                                                          \
> > -       $(if $(CONFIG_MODVERSIONS),-m)                                                  \
> > -       $(if $(CONFIG_MODULE_SRCVERSION_ALL),-a)                                        \
> > -       $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)                                  \
> > -       $(if $(KBUILD_NSDEPS),-d $(MODULES_NSDEPS))                                     \
> > -       $(if $(CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS)$(KBUILD_NSDEPS),-N)       \
> > -       -o $@
> > +MODPOST = scripts/mod/modpost
> > +
> > +modpost-args-$(CONFIG_MODVERSIONS)                             += -m
> > +modpost-args-$(CONFIG_MODULE_SRCVERSION_ALL)                   += -a
> > +modpost-args-$(CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS)  += -N
> > +modpost-args-$(KBUILD_NSDEPS)                                  += -N -d $(MODULES_NSDEPS)
>
> Was it intentional that you included -N for KBUILD_NSDEPS?


Yes.
See $(CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS)$(KBUILD_NSDEPS)
in the original code.


> That seems
> like a change in behavior.  Everything else LGTM but that.


No behavior change.


If both CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS
and KBUILD_NSDEPS are defined, -N was previously passed
just once, but now twice.
This is a slight change, but behavior is still the same.





> > +modpost-args-y += $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)
>
> Ugh, your pattern kind of falls apart for the above ^.  Maybe this is
> the most concise way to express this, but boy I sure do sometimes wish
> for a programming language that had booleans and simple negation.
> Maybe I would regret that in a build system...
> --
> Thanks,
> ~Nick Desaulniers


I do not think CONFIG_SECTION_MISMATCH_WARN_ONLY will live long,
but a cleaner fix is to stop negating the option.

Anyway, I do not care about that.







diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 911606496341..c5a18920646d 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -44,7 +44,7 @@ include $(srctree)/scripts/Kbuild.include
 MODPOST = scripts/mod/modpost
                 \
        $(if $(CONFIG_MODVERSIONS),-m)
                 \
        $(if $(CONFIG_MODULE_SRCVERSION_ALL),-a)
                 \
-       $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)
                 \
+       $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),-E)
                 \
        -o $@

 ifdef MODPOST_VMLINUX
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 2c80da0220c3..be55dd541055 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -33,7 +33,7 @@ static bool external_module;
 static bool warn_unresolved;

 static int sec_mismatch_count;
-static bool sec_mismatch_warn_only = true;
+static bool sec_mismatch_warn_only;
 /* ignore missing files */
 static bool ignore_missing_files;
 /* If set to 1, only warn (instead of error) about missing ns imports */
@@ -2355,7 +2355,7 @@ int main(int argc, char **argv)
                        warn_unresolved = true;
                        break;
                case 'E':
-                       sec_mismatch_warn_only = false;
+                       sec_mismatch_warn_only = true;
                        break;
                case 'N':
                        allow_missing_ns_imports = true;



























-- 
Best Regards
Masahiro Yamada