[PATCH 0/2] kbuild: CRC versions for asm functions

[PATCH 0/2] kbuild: CRC versions for asm functions

[Nicholas Piggin]

Hi,

Here's what I've got to deal with the asm exports issue. This will
require arch patches to add asm/asm-prototypes.h that gets picked
up by kbuild.

Any comments? Objections?

Thanks,
Nick

[PATCH 1/2] kbuild: modpost warn if export version crc is missing

[Nicholas Piggin]

This catches the failing ceph CRC on with:

    LD      vmlinux.o
    MODPOST vmlinux.o
  WARNING: EXPORT symbol "ceph_monc_do_statfs" [vmlinux] version
  generation failed, symbol will not be versioned.

When the modules referring to exported symbols are built, there is an
existing warning for missing CRC, but it's not always the case such
any such module will be built, and in any case it is useful to get a
warning at the source.

This gets a little verbose with CONFIG_DEBUG_SECTION_MISMATCH,
producing a warning with each object linked, but I didn't think
that warranted extra complexity to avoid.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/mod/modpost.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index bd83497..08f62a1 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -609,6 +609,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
 {
 	unsigned int crc;
 	enum export export;
+	bool is_crc = false;
 
 	if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
 	    strncmp(symname, "__ksymtab", 9) == 0)
@@ -618,6 +619,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
 
 	/* CRC'd symbol */
 	if (strncmp(symname, CRC_PFX, strlen(CRC_PFX)) == 0) {
+		is_crc = true;
 		crc = (unsigned int) sym->st_value;
 		sym_update_crc(symname + strlen(CRC_PFX), mod, crc,
 				export);
@@ -663,6 +665,10 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
 		else
 			symname++;
 #endif
+		if (is_crc) {
+			const char *e = is_vmlinux(mod->name) ?"":".ko";
+			warn("EXPORT symbol \"%s\" [%s%s] version generation failed, symbol will not be versioned.\n", symname + strlen(CRC_PFX), mod->name, e);
+		}
 		mod->unres = alloc_symbol(symname,
 					  ELF_ST_BIND(sym->st_info) == STB_WEAK,
 					  mod->unres);
-- 
2.9.3

[PATCH 1/2] kbuild: modpost warn if export version crc is missing

[Nicholas Piggin]

This catches the failing ceph CRC on with:

    LD      vmlinux.o
    MODPOST vmlinux.o
  WARNING: EXPORT symbol "ceph_monc_do_statfs" [vmlinux] version
  generation failed, symbol will not be versioned.

When the modules referring to exported symbols are built, there is an
existing warning for missing CRC, but it's not always the case such
any such module will be built, and in any case it is useful to get a
warning at the source.

This gets a little verbose with CONFIG_DEBUG_SECTION_MISMATCH,
producing a warning with each object linked, but I didn't think
that warranted extra complexity to avoid.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/mod/modpost.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index bd83497..08f62a1 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -609,6 +609,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
 {
 	unsigned int crc;
 	enum export export;
+	bool is_crc = false;
 
 	if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
 	    strncmp(symname, "__ksymtab", 9) == 0)
@@ -618,6 +619,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
 
 	/* CRC'd symbol */
 	if (strncmp(symname, CRC_PFX, strlen(CRC_PFX)) == 0) {
+		is_crc = true;
 		crc = (unsigned int) sym->st_value;
 		sym_update_crc(symname + strlen(CRC_PFX), mod, crc,
 				export);
@@ -663,6 +665,10 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
 		else
 			symname++;
 #endif
+		if (is_crc) {
+			const char *e = is_vmlinux(mod->name) ?"":".ko";
+			warn("EXPORT symbol \"%s\" [%s%s] version generation failed, symbol will not be versioned.\n", symname + strlen(CRC_PFX), mod->name, e);
+		}
 		mod->unres = alloc_symbol(symname,
 					  ELF_ST_BIND(sym->st_info) == STB_WEAK,
 					  mod->unres);
-- 
2.9.3

[PATCH 2/2] kbuild: modversions for exported asm symbols

[Nicholas Piggin]

Allow architectures to create asm/asm-prototypes.h file that
provides C prototypes for exported asm functions, which enables
proper CRC versions to be generated for them.

It's done by creating a trivial C program that includes that file
and the EXPORT_SYMBOL()s from the .S file, and preprocesses that
then sends the result to genksyms.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/Makefile.build | 71 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 6 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index de46ab0..edcf876 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -159,7 +159,8 @@ cmd_cpp_i_c       = $(CPP) $(c_flags) -o $@ $<
 $(obj)/%.i: $(src)/%.c FORCE
 	$(call if_changed_dep,cpp_i_c)
 
-cmd_gensymtypes =                                                           \
+# These mirror gensymtypes_S and co below, keep them in synch.
+cmd_gensymtypes_c =                                                         \
     $(CPP) -D__GENKSYMS__ $(c_flags) $< |                                   \
     $(GENKSYMS) $(if $(1), -T $(2))                                         \
      $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX))             \
@@ -169,7 +170,7 @@ cmd_gensymtypes =                                                           \
 quiet_cmd_cc_symtypes_c = SYM $(quiet_modtag) $@
 cmd_cc_symtypes_c =                                                         \
     set -e;                                                                 \
-    $(call cmd_gensymtypes,true,$@) >/dev/null;                             \
+    $(call cmd_gensymtypes_c,true,$@) >/dev/null;                           \
     test -s $@ || rm -f $@
 
 $(obj)/%.symtypes : $(src)/%.c FORCE
@@ -198,9 +199,10 @@ else
 #   the actual value of the checksum generated by genksyms
 
 cmd_cc_o_c = $(CC) $(c_flags) -c -o $(@D)/.tmp_$(@F) $<
-cmd_modversions =								\
+
+cmd_modversions_c =								\
 	if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then		\
-		$(call cmd_gensymtypes,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))	\
+		$(call cmd_gensymtypes_c,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))	\
 		    > $(@D)/.tmp_$(@F:.o=.ver);					\
 										\
 		$(LD) $(LDFLAGS) -r -o $@ $(@D)/.tmp_$(@F) 			\
@@ -268,13 +270,14 @@ endif # CONFIG_STACK_VALIDATION
 define rule_cc_o_c
 	$(call echo-cmd,checksrc) $(cmd_checksrc)			  \
 	$(call cmd_and_fixdep,cc_o_c)					  \
-	$(cmd_modversions)						  \
+	$(cmd_modversions_c)						  \
 	$(cmd_objtool)						          \
 	$(call echo-cmd,record_mcount) $(cmd_record_mcount)
 endef
 
 define rule_as_o_S
 	$(call cmd_and_fixdep,as_o_S)					  \
+	$(cmd_modversions_S)						  \
 	$(cmd_objtool)
 endef
 
@@ -314,6 +317,32 @@ modkern_aflags := $(KBUILD_AFLAGS_KERNEL) $(AFLAGS_KERNEL)
 $(real-objs-m)      : modkern_aflags := $(KBUILD_AFLAGS_MODULE) $(AFLAGS_MODULE)
 $(real-objs-m:.o=.s): modkern_aflags := $(KBUILD_AFLAGS_MODULE) $(AFLAGS_MODULE)
 
+# .S file exports must have their C prototypes defined in asm/asm-prototypes.h
+# or a file that it includes, in order to get versioned symbols. We build a
+# dummy C file that includes asm-prototypes and the EXPORT_SYMBOL lines from
+# the .S file (with trailing ';'), and run genksyms on that, to extract vers.
+#
+# These mirror gensymtypes_c and co above, keep them in synch.
+cmd_gensymtypes_S =                                                         \
+    (echo "\#include <linux/kernel.h>" ;                                    \
+     echo "\#include <asm/asm-prototypes.h>" ;                              \
+     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
+    $(CPP) -D__GENKSYMS__ $(c_flags) -xc - |                                \
+    $(GENKSYMS) $(if $(1), -T $(2))                                         \
+     $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX))             \
+     $(if $(KBUILD_PRESERVE),-p)                                            \
+     -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))
+
+quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
+cmd_cc_symtypes_S =                                                         \
+    set -e;                                                                 \
+    $(call cmd_gensymtypes_S,true,$@) >/dev/null;                           \
+    test -s $@ || rm -f $@
+
+$(obj)/%.symtypes : $(src)/%.S FORCE
+	$(call cmd,cc_symtypes_S)
+
+
 quiet_cmd_cpp_s_S = CPP $(quiet_modtag) $@
 cmd_cpp_s_S       = $(CPP) $(a_flags) -o $@ $<
 
@@ -321,7 +350,37 @@ $(obj)/%.s: $(src)/%.S FORCE
 	$(call if_changed_dep,cpp_s_S)
 
 quiet_cmd_as_o_S = AS $(quiet_modtag)  $@
-cmd_as_o_S       = $(CC) $(a_flags) -c -o $@ $<
+
+ifndef CONFIG_MODVERSIONS
+cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
+
+else
+
+ASM_PROTOTYPES := $(wildcard $(srctree)/arch/$(SRCARCH)/include/asm/asm-prototypes.h)
+
+ifeq ($(ASM_PROTOTYPES),)
+cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
+
+else
+
+# versioning matches the C process described above, with difference that
+# we parse asm-prototypes.h C header to get function definitions.
+
+cmd_as_o_S = $(CC) $(a_flags) -c -o $(@D)/.tmp_$(@F) $<
+
+cmd_modversions_S =								\
+	if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then		\
+		$(call cmd_gensymtypes_S,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))	\
+		    > $(@D)/.tmp_$(@F:.o=.ver);					\
+										\
+		$(LD) $(LDFLAGS) -r -o $@ $(@D)/.tmp_$(@F) 			\
+			-T $(@D)/.tmp_$(@F:.o=.ver);				\
+		rm -f $(@D)/.tmp_$(@F) $(@D)/.tmp_$(@F:.o=.ver);		\
+	else									\
+		mv -f $(@D)/.tmp_$(@F) $@;					\
+	fi;
+endif
+endif
 
 $(obj)/%.o: $(src)/%.S $(objtool_obj) FORCE
 	$(call if_changed_rule,as_o_S)
-- 
2.9.3

[PATCH 2/2] kbuild: modversions for exported asm symbols

[Nicholas Piggin]

Allow architectures to create asm/asm-prototypes.h file that
provides C prototypes for exported asm functions, which enables
proper CRC versions to be generated for them.

It's done by creating a trivial C program that includes that file
and the EXPORT_SYMBOL()s from the .S file, and preprocesses that
then sends the result to genksyms.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/Makefile.build | 71 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 6 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index de46ab0..edcf876 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -159,7 +159,8 @@ cmd_cpp_i_c       = $(CPP) $(c_flags) -o $@ $<
 $(obj)/%.i: $(src)/%.c FORCE
 	$(call if_changed_dep,cpp_i_c)
 
-cmd_gensymtypes =                                                           \
+# These mirror gensymtypes_S and co below, keep them in synch.
+cmd_gensymtypes_c =                                                         \
     $(CPP) -D__GENKSYMS__ $(c_flags) $< |                                   \
     $(GENKSYMS) $(if $(1), -T $(2))                                         \
      $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX))             \
@@ -169,7 +170,7 @@ cmd_gensymtypes =                                                           \
 quiet_cmd_cc_symtypes_c = SYM $(quiet_modtag) $@
 cmd_cc_symtypes_c =                                                         \
     set -e;                                                                 \
-    $(call cmd_gensymtypes,true,$@) >/dev/null;                             \
+    $(call cmd_gensymtypes_c,true,$@) >/dev/null;                           \
     test -s $@ || rm -f $@
 
 $(obj)/%.symtypes : $(src)/%.c FORCE
@@ -198,9 +199,10 @@ else
 #   the actual value of the checksum generated by genksyms
 
 cmd_cc_o_c = $(CC) $(c_flags) -c -o $(@D)/.tmp_$(@F) $<
-cmd_modversions =								\
+
+cmd_modversions_c =								\
 	if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then		\
-		$(call cmd_gensymtypes,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))	\
+		$(call cmd_gensymtypes_c,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))	\
 		    > $(@D)/.tmp_$(@F:.o=.ver);					\
 										\
 		$(LD) $(LDFLAGS) -r -o $@ $(@D)/.tmp_$(@F) 			\
@@ -268,13 +270,14 @@ endif # CONFIG_STACK_VALIDATION
 define rule_cc_o_c
 	$(call echo-cmd,checksrc) $(cmd_checksrc)			  \
 	$(call cmd_and_fixdep,cc_o_c)					  \
-	$(cmd_modversions)						  \
+	$(cmd_modversions_c)						  \
 	$(cmd_objtool)						          \
 	$(call echo-cmd,record_mcount) $(cmd_record_mcount)
 endef
 
 define rule_as_o_S
 	$(call cmd_and_fixdep,as_o_S)					  \
+	$(cmd_modversions_S)						  \
 	$(cmd_objtool)
 endef
 
@@ -314,6 +317,32 @@ modkern_aflags := $(KBUILD_AFLAGS_KERNEL) $(AFLAGS_KERNEL)
 $(real-objs-m)      : modkern_aflags := $(KBUILD_AFLAGS_MODULE) $(AFLAGS_MODULE)
 $(real-objs-m:.o=.s): modkern_aflags := $(KBUILD_AFLAGS_MODULE) $(AFLAGS_MODULE)
 
+# .S file exports must have their C prototypes defined in asm/asm-prototypes.h
+# or a file that it includes, in order to get versioned symbols. We build a
+# dummy C file that includes asm-prototypes and the EXPORT_SYMBOL lines from
+# the .S file (with trailing ';'), and run genksyms on that, to extract vers.
+#
+# These mirror gensymtypes_c and co above, keep them in synch.
+cmd_gensymtypes_S =                                                         \
+    (echo "\#include <linux/kernel.h>" ;                                    \
+     echo "\#include <asm/asm-prototypes.h>" ;                              \
+     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
+    $(CPP) -D__GENKSYMS__ $(c_flags) -xc - |                                \
+    $(GENKSYMS) $(if $(1), -T $(2))                                         \
+     $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX))             \
+     $(if $(KBUILD_PRESERVE),-p)                                            \
+     -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))
+
+quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
+cmd_cc_symtypes_S =                                                         \
+    set -e;                                                                 \
+    $(call cmd_gensymtypes_S,true,$@) >/dev/null;                           \
+    test -s $@ || rm -f $@
+
+$(obj)/%.symtypes : $(src)/%.S FORCE
+	$(call cmd,cc_symtypes_S)
+
+
 quiet_cmd_cpp_s_S = CPP $(quiet_modtag) $@
 cmd_cpp_s_S       = $(CPP) $(a_flags) -o $@ $<
 
@@ -321,7 +350,37 @@ $(obj)/%.s: $(src)/%.S FORCE
 	$(call if_changed_dep,cpp_s_S)
 
 quiet_cmd_as_o_S = AS $(quiet_modtag)  $@
-cmd_as_o_S       = $(CC) $(a_flags) -c -o $@ $<
+
+ifndef CONFIG_MODVERSIONS
+cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
+
+else
+
+ASM_PROTOTYPES := $(wildcard $(srctree)/arch/$(SRCARCH)/include/asm/asm-prototypes.h)
+
+ifeq ($(ASM_PROTOTYPES),)
+cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
+
+else
+
+# versioning matches the C process described above, with difference that
+# we parse asm-prototypes.h C header to get function definitions.
+
+cmd_as_o_S = $(CC) $(a_flags) -c -o $(@D)/.tmp_$(@F) $<
+
+cmd_modversions_S =								\
+	if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then		\
+		$(call cmd_gensymtypes_S,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))	\
+		    > $(@D)/.tmp_$(@F:.o=.ver);					\
+										\
+		$(LD) $(LDFLAGS) -r -o $@ $(@D)/.tmp_$(@F) 			\
+			-T $(@D)/.tmp_$(@F:.o=.ver);				\
+		rm -f $(@D)/.tmp_$(@F) $(@D)/.tmp_$(@F:.o=.ver);		\
+	else									\
+		mv -f $(@D)/.tmp_$(@F) $@;					\
+	fi;
+endif
+endif
 
 $(obj)/%.o: $(src)/%.S $(objtool_obj) FORCE
 	$(call if_changed_rule,as_o_S)
-- 
2.9.3

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Michal Marek]

Hi Nick,

sorry for the late feedback.

Dne 15.10.2016 v 14:43 Nicholas Piggin napsal(a):
> +# .S file exports must have their C prototypes defined in asm/asm-prototypes.h
> +# or a file that it includes, in order to get versioned symbols. We build a
> +# dummy C file that includes asm-prototypes and the EXPORT_SYMBOL lines from
> +# the .S file (with trailing ';'), and run genksyms on that, to extract vers.
> +#
> +# These mirror gensymtypes_c and co above, keep them in synch.
> +cmd_gensymtypes_S =                                                         \
> +    (echo "\#include <linux/kernel.h>" ;                                    \
> +     echo "\#include <asm/asm-prototypes.h>" ;                              \
> +     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
> +    $(CPP) -D__GENKSYMS__ $(c_flags) -xc - |                                \
> +    $(GENKSYMS) $(if $(1), -T $(2))                                         \
> +     $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX))             \
> +     $(if $(KBUILD_PRESERVE),-p)                                            \
> +     -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))

I think it would be cleaner to add the #include to the .S files
themselves and grep for both EXPORT_SYMBOL and #include here. The reason
is that some files might need additional #includes to allow genksyms to
properly expand some function prototypes.

Michal

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Arnd Bergmann]

On Wednesday, October 19, 2016 4:50:26 PM CEST Michal Marek wrote:
> Dne 15.10.2016 v 14:43 Nicholas Piggin napsal(a):
> > +# .S file exports must have their C prototypes defined in asm/asm-prototypes.h
> > +# or a file that it includes, in order to get versioned symbols. We build a
> > +# dummy C file that includes asm-prototypes and the EXPORT_SYMBOL lines from
> > +# the .S file (with trailing ';'), and run genksyms on that, to extract vers.
> > +#
> > +# These mirror gensymtypes_c and co above, keep them in synch.
> > +cmd_gensymtypes_S =                                                         \
> > +    (echo "\#include <linux/kernel.h>" ;                                    \
> > +     echo "\#include <asm/asm-prototypes.h>" ;                              \
> > +     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
> > +    $(CPP) -D__GENKSYMS__ $(c_flags) -xc - |                                \
> > +    $(GENKSYMS) $(if $(1), -T $(2))                                         \
> > +     $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX))             \
> > +     $(if $(KBUILD_PRESERVE),-p)                                            \
> > +     -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))
> 
> I think it would be cleaner to add the #include to the .S files
> themselves and grep for both EXPORT_SYMBOL and #include here. The reason
> is that some files might need additional #includes to allow genksyms to
> properly expand some function prototypes.
> 

This is something I tried earlier, and it wasn't pretty: Some of the assembler
files rely on -D__ASSEMBLER__  to be set in order to read the right headers,
but setting that macro means that all of the declarations get skipped.

I ended up testing for -D__GENKSYMS__ in each .S file, which was also
rather ugly.

	Arnd

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Nicholas Piggin]

On Wed, 19 Oct 2016 16:59:42 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Wednesday, October 19, 2016 4:50:26 PM CEST Michal Marek wrote:
> > Dne 15.10.2016 v 14:43 Nicholas Piggin napsal(a):  
> > > +# .S file exports must have their C prototypes defined in asm/asm-prototypes.h
> > > +# or a file that it includes, in order to get versioned symbols. We build a
> > > +# dummy C file that includes asm-prototypes and the EXPORT_SYMBOL lines from
> > > +# the .S file (with trailing ';'), and run genksyms on that, to extract vers.
> > > +#
> > > +# These mirror gensymtypes_c and co above, keep them in synch.
> > > +cmd_gensymtypes_S =                                                         \
> > > +    (echo "\#include <linux/kernel.h>" ;                                    \
> > > +     echo "\#include <asm/asm-prototypes.h>" ;                              \
> > > +     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
> > > +    $(CPP) -D__GENKSYMS__ $(c_flags) -xc - |                                \
> > > +    $(GENKSYMS) $(if $(1), -T $(2))                                         \
> > > +     $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX))             \
> > > +     $(if $(KBUILD_PRESERVE),-p)                                            \
> > > +     -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))  
> > 
> > I think it would be cleaner to add the #include to the .S files
> > themselves and grep for both EXPORT_SYMBOL and #include here. The reason
> > is that some files might need additional #includes to allow genksyms to
> > properly expand some function prototypes.
> >   
> 
> This is something I tried earlier, and it wasn't pretty: Some of the assembler
> files rely on -D__ASSEMBLER__  to be set in order to read the right headers,
> but setting that macro means that all of the declarations get skipped.
> 
> I ended up testing for -D__GENKSYMS__ in each .S file, which was also
> rather ugly.

Yeah, I had the same idea as you and Michal too. It's conceptually nicer,
but in practice it turned into a mess. If some architectures wanted to start
protecting their .h files and including them into .S for the prototypes, we
could start parsing those too. Should we do the quick and dirty way for 4.9?

Thanks,
Nick

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Arnd Bergmann]

On Thursday, October 20, 2016 2:58:27 PM CEST Nicholas Piggin wrote:
> 
> Yeah, I had the same idea as you and Michal too. It's conceptually nicer,
> but in practice it turned into a mess. If some architectures wanted to start
> protecting their .h files and including them into .S for the prototypes, we
> could start parsing those too. Should we do the quick and dirty way for 4.9?

Let's stay with your approach for now.

	Arnd

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Arnd Bergmann]

On Thursday, October 20, 2016 2:58:27 PM CEST Nicholas Piggin wrote:
> 
> Yeah, I had the same idea as you and Michal too. It's conceptually nicer,
> but in practice it turned into a mess. If some architectures wanted to start
> protecting their .h files and including them into .S for the prototypes, we
> could start parsing those too. Should we do the quick and dirty way for 4.9?

Let's stay with your approach for now.

	Arnd

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Michal Marek]

Dne 20.10.2016 v 10:03 Arnd Bergmann napsal(a):
> On Thursday, October 20, 2016 2:58:27 PM CEST Nicholas Piggin wrote:
>>
>> Yeah, I had the same idea as you and Michal too. It's conceptually nicer,
>> but in practice it turned into a mess. If some architectures wanted to start
>> protecting their .h files and including them into .S for the prototypes, we
>> could start parsing those too. Should we do the quick and dirty way for 4.9?
> 
> Let's stay with your approach for now.

Agreed.

Michal

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Michal Marek]

Dne 20.10.2016 v 10:03 Arnd Bergmann napsal(a):
> On Thursday, October 20, 2016 2:58:27 PM CEST Nicholas Piggin wrote:
>>
>> Yeah, I had the same idea as you and Michal too. It's conceptually nicer,
>> but in practice it turned into a mess. If some architectures wanted to start
>> protecting their .h files and including them into .S for the prototypes, we
>> could start parsing those too. Should we do the quick and dirty way for 4.9?
> 
> Let's stay with your approach for now.

Agreed.

Michal

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Nicholas Piggin]

On Sat, 22 Oct 2016 17:36:34 +0200
Michal Marek <mmarek@suse.com> wrote:

> Dne 20.10.2016 v 10:03 Arnd Bergmann napsal(a):
> > On Thursday, October 20, 2016 2:58:27 PM CEST Nicholas Piggin wrote:  
> >>
> >> Yeah, I had the same idea as you and Michal too. It's conceptually nicer,
> >> but in practice it turned into a mess. If some architectures wanted to start
> >> protecting their .h files and including them into .S for the prototypes, we
> >> could start parsing those too. Should we do the quick and dirty way for 4.9?  
> > 
> > Let's stay with your approach for now.  
> 
> Agreed.

My patch 2/2 needs the following incremental patch applied. It is
to preprocess the .S file properly as asm the same way the kernel
build does, and then building the dummy C from there.

Without this, we don't necessarily get proper symbol expansion or
get conditional compilation, etc. With it, genksyms should see what
the assembler sees.

---
 scripts/Makefile.build | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 2e512a2..6beeb9e 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -321,12 +321,19 @@ $(real-objs-m:.o=.s): modkern_aflags := $(KBUILD_AFLAGS_MODULE) $(AFLAGS_MODULE)
 # or a file that it includes, in order to get versioned symbols. We build a
 # dummy C file that includes asm-prototypes and the EXPORT_SYMBOL lines from
 # the .S file (with trailing ';'), and run genksyms on that, to extract vers.
+# 
+# This is convoluted. The .S file must first be preprocessed to run guards and
+# expand names, then the resulting exports must be constructed into plain
+# EXPORT_SYMBOL(symbol); to build our dummy C file, and that gets preprocessed
+# to make the genksyms input.
 #
 # These mirror gensymtypes_c and co above, keep them in synch.
 cmd_gensymtypes_S =                                                         \
     (echo "\#include <linux/kernel.h>" ;                                    \
      echo "\#include <asm/asm-prototypes.h>" ;                              \
-     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
+    $(CPP) $(a_flags) $< |                                                  \
+     grep ^___EXPORT_SYMBOL |                                               \
+     sed 's/___EXPORT_SYMBOL \([a-zA-Z0-9_]*\),.*/EXPORT_SYMBOL(\1);/' ) |  \
     $(CPP) -D__GENKSYMS__ $(c_flags) -xc - |                                \
     $(GENKSYMS) $(if $(1), -T $(2))                                         \
      $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX))             \
-- 
2.9.3

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Michal Marek]

On 2016-10-31 12:14, Nicholas Piggin wrote:
> +# This is convoluted. The .S file must first be preprocessed to run guards and
> +# expand names, then the resulting exports must be constructed into plain
> +# EXPORT_SYMBOL(symbol); to build our dummy C file, and that gets preprocessed
> +# to make the genksyms input.
>  #
>  # These mirror gensymtypes_c and co above, keep them in synch.
>  cmd_gensymtypes_S =                                                         \
>      (echo "\#include <linux/kernel.h>" ;                                    \
>       echo "\#include <asm/asm-prototypes.h>" ;                              \
> -     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
> +    $(CPP) $(a_flags) $< |                                                  \
> +     grep ^___EXPORT_SYMBOL |                                               \
> +     sed 's/___EXPORT_SYMBOL \([a-zA-Z0-9_]*\),.*/EXPORT_SYMBOL(\1);/' ) |  \

Is this sed pass necessary? Just add -D__GENKSYMS__ also to the first
cpp run and EXPORT_SYMBOL will stay intact.

Anyway, I'm going to merge your patch 2/2 now.

Michal

>      $(CPP) -D__GENKSYMS__ $(c_flags) -xc - |                                \
>      $(GENKSYMS) $(if $(1), -T $(2))                                         \
>       $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX))             \
> 

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Michal Marek]

On 2016-10-31 12:14, Nicholas Piggin wrote:
> +# This is convoluted. The .S file must first be preprocessed to run guards and
> +# expand names, then the resulting exports must be constructed into plain
> +# EXPORT_SYMBOL(symbol); to build our dummy C file, and that gets preprocessed
> +# to make the genksyms input.
>  #
>  # These mirror gensymtypes_c and co above, keep them in synch.
>  cmd_gensymtypes_S =                                                         \
>      (echo "\#include <linux/kernel.h>" ;                                    \
>       echo "\#include <asm/asm-prototypes.h>" ;                              \
> -     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
> +    $(CPP) $(a_flags) $< |                                                  \
> +     grep ^___EXPORT_SYMBOL |                                               \
> +     sed 's/___EXPORT_SYMBOL \([a-zA-Z0-9_]*\),.*/EXPORT_SYMBOL(\1);/' ) |  \

Is this sed pass necessary? Just add -D__GENKSYMS__ also to the first
cpp run and EXPORT_SYMBOL will stay intact.

Anyway, I'm going to merge your patch 2/2 now.

Michal

>      $(CPP) -D__GENKSYMS__ $(c_flags) -xc - |                                \
>      $(GENKSYMS) $(if $(1), -T $(2))                                         \
>       $(patsubst y,-s _,$(CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX))             \
> 

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Michal Marek]

On 2016-11-01 15:19, Michal Marek wrote:
> On 2016-10-31 12:14, Nicholas Piggin wrote:
>> +# This is convoluted. The .S file must first be preprocessed to run guards and
>> +# expand names, then the resulting exports must be constructed into plain
>> +# EXPORT_SYMBOL(symbol); to build our dummy C file, and that gets preprocessed
>> +# to make the genksyms input.
>>  #
>>  # These mirror gensymtypes_c and co above, keep them in synch.
>>  cmd_gensymtypes_S =                                                         \
>>      (echo "\#include <linux/kernel.h>" ;                                    \
>>       echo "\#include <asm/asm-prototypes.h>" ;                              \
>> -     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
>> +    $(CPP) $(a_flags) $< |                                                  \
>> +     grep ^___EXPORT_SYMBOL |                                               \
>> +     sed 's/___EXPORT_SYMBOL \([a-zA-Z0-9_]*\),.*/EXPORT_SYMBOL(\1);/' ) |  \
> 
> Is this sed pass necessary? Just add -D__GENKSYMS__ also to the first
> cpp run and EXPORT_SYMBOL will stay intact.
> 
> Anyway, I'm going to merge your patch 2/2 now.

Now I noticed you posted a combined patch today. What do you thing about
removing the sed call in favor of -D__GENKSYMS__?

Thanks,
Michal

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Nicholas Piggin]

On Tue, 1 Nov 2016 15:19:59 +0100
Michal Marek <mmarek@suse.com> wrote:

> On 2016-10-31 12:14, Nicholas Piggin wrote:
> > +# This is convoluted. The .S file must first be preprocessed to run guards and
> > +# expand names, then the resulting exports must be constructed into plain
> > +# EXPORT_SYMBOL(symbol); to build our dummy C file, and that gets preprocessed
> > +# to make the genksyms input.
> >  #
> >  # These mirror gensymtypes_c and co above, keep them in synch.
> >  cmd_gensymtypes_S =                                                         \
> >      (echo "\#include <linux/kernel.h>" ;                                    \
> >       echo "\#include <asm/asm-prototypes.h>" ;                              \
> > -     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
> > +    $(CPP) $(a_flags) $< |                                                  \
> > +     grep ^___EXPORT_SYMBOL |                                               \
> > +     sed 's/___EXPORT_SYMBOL \([a-zA-Z0-9_]*\),.*/EXPORT_SYMBOL(\1);/' ) |  \  
> 
> Is this sed pass necessary? Just add -D__GENKSYMS__ also to the first
> cpp run and EXPORT_SYMBOL will stay intact.

I can't see how it would work if asm-generic/export.h doesn't have any
tests for GENKSYMS.

> Anyway, I'm going to merge your patch 2/2 now.

Okay. We should come to some resolution of the preprocessing issue too.
I'm logging off for tonight, but if you want to tweak or redo the
incremental patch, feel free.

Thanks,
Nick

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Nicholas Piggin]

On Tue, 1 Nov 2016 15:19:59 +0100
Michal Marek <mmarek@suse.com> wrote:

> On 2016-10-31 12:14, Nicholas Piggin wrote:
> > +# This is convoluted. The .S file must first be preprocessed to run guards and
> > +# expand names, then the resulting exports must be constructed into plain
> > +# EXPORT_SYMBOL(symbol); to build our dummy C file, and that gets preprocessed
> > +# to make the genksyms input.
> >  #
> >  # These mirror gensymtypes_c and co above, keep them in synch.
> >  cmd_gensymtypes_S =                                                         \
> >      (echo "\#include <linux/kernel.h>" ;                                    \
> >       echo "\#include <asm/asm-prototypes.h>" ;                              \
> > -     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
> > +    $(CPP) $(a_flags) $< |                                                  \
> > +     grep ^___EXPORT_SYMBOL |                                               \
> > +     sed 's/___EXPORT_SYMBOL \([a-zA-Z0-9_]*\),.*/EXPORT_SYMBOL(\1);/' ) |  \  
> 
> Is this sed pass necessary? Just add -D__GENKSYMS__ also to the first
> cpp run and EXPORT_SYMBOL will stay intact.

I can't see how it would work if asm-generic/export.h doesn't have any
tests for GENKSYMS.

> Anyway, I'm going to merge your patch 2/2 now.

Okay. We should come to some resolution of the preprocessing issue too.
I'm logging off for tonight, but if you want to tweak or redo the
incremental patch, feel free.

Thanks,
Nick

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Michal Marek]

On 2016-11-01 15:36, Nicholas Piggin wrote:
> On Tue, 1 Nov 2016 15:19:59 +0100
> Michal Marek <mmarek@suse.com> wrote:
> 
>> On 2016-10-31 12:14, Nicholas Piggin wrote:
>>> +# This is convoluted. The .S file must first be preprocessed to run guards and
>>> +# expand names, then the resulting exports must be constructed into plain
>>> +# EXPORT_SYMBOL(symbol); to build our dummy C file, and that gets preprocessed
>>> +# to make the genksyms input.
>>>  #
>>>  # These mirror gensymtypes_c and co above, keep them in synch.
>>>  cmd_gensymtypes_S =                                                         \
>>>      (echo "\#include <linux/kernel.h>" ;                                    \
>>>       echo "\#include <asm/asm-prototypes.h>" ;                              \
>>> -     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
>>> +    $(CPP) $(a_flags) $< |                                                  \
>>> +     grep ^___EXPORT_SYMBOL |                                               \
>>> +     sed 's/___EXPORT_SYMBOL \([a-zA-Z0-9_]*\),.*/EXPORT_SYMBOL(\1);/' ) |  \  
>>
>> Is this sed pass necessary? Just add -D__GENKSYMS__ also to the first
>> cpp run and EXPORT_SYMBOL will stay intact.
> 
> I can't see how it would work if asm-generic/export.h doesn't have any
> tests for GENKSYMS.

You are right, these would have to be added first to mimic the
<linux/export.h> behavior.

Michal

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Michal Marek]

On 2016-11-01 15:36, Nicholas Piggin wrote:
> On Tue, 1 Nov 2016 15:19:59 +0100
> Michal Marek <mmarek@suse.com> wrote:
> 
>> On 2016-10-31 12:14, Nicholas Piggin wrote:
>>> +# This is convoluted. The .S file must first be preprocessed to run guards and
>>> +# expand names, then the resulting exports must be constructed into plain
>>> +# EXPORT_SYMBOL(symbol); to build our dummy C file, and that gets preprocessed
>>> +# to make the genksyms input.
>>>  #
>>>  # These mirror gensymtypes_c and co above, keep them in synch.
>>>  cmd_gensymtypes_S =                                                         \
>>>      (echo "\#include <linux/kernel.h>" ;                                    \
>>>       echo "\#include <asm/asm-prototypes.h>" ;                              \
>>> -     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
>>> +    $(CPP) $(a_flags) $< |                                                  \
>>> +     grep ^___EXPORT_SYMBOL |                                               \
>>> +     sed 's/___EXPORT_SYMBOL \([a-zA-Z0-9_]*\),.*/EXPORT_SYMBOL(\1);/' ) |  \  
>>
>> Is this sed pass necessary? Just add -D__GENKSYMS__ also to the first
>> cpp run and EXPORT_SYMBOL will stay intact.
> 
> I can't see how it would work if asm-generic/export.h doesn't have any
> tests for GENKSYMS.

You are right, these would have to be added first to mimic the
<linux/export.h> behavior.

Michal

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Michal Marek]

On 2016-11-01 15:36, Nicholas Piggin wrote:
> On Tue, 1 Nov 2016 15:19:59 +0100
> Michal Marek <mmarek@suse.com> wrote:
> 
>> On 2016-10-31 12:14, Nicholas Piggin wrote:
>>> +# This is convoluted. The .S file must first be preprocessed to run guards and
>>> +# expand names, then the resulting exports must be constructed into plain
>>> +# EXPORT_SYMBOL(symbol); to build our dummy C file, and that gets preprocessed
>>> +# to make the genksyms input.
>>>  #
>>>  # These mirror gensymtypes_c and co above, keep them in synch.
>>>  cmd_gensymtypes_S =                                                         \
>>>      (echo "\#include <linux/kernel.h>" ;                                    \
>>>       echo "\#include <asm/asm-prototypes.h>" ;                              \
>>> -     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
>>> +    $(CPP) $(a_flags) $< |                                                  \
>>> +     grep ^___EXPORT_SYMBOL |                                               \
>>> +     sed 's/___EXPORT_SYMBOL \([a-zA-Z0-9_]*\),.*/EXPORT_SYMBOL(\1);/' ) |  \  
>>
>> Is this sed pass necessary? Just add -D__GENKSYMS__ also to the first
>> cpp run and EXPORT_SYMBOL will stay intact.
> 
> I can't see how it would work if asm-generic/export.h doesn't have any
> tests for GENKSYMS.
> 
>> Anyway, I'm going to merge your patch 2/2 now.
> 
> Okay. We should come to some resolution of the preprocessing issue too.
> I'm logging off for tonight, but if you want to tweak or redo the
> incremental patch, feel free.

I applied the folded patch as is. The preprocessing is not pretty, but
the 4.9 regression is more important to be fixed.

Thanks,
Michal

Re: [PATCH 2/2] kbuild: modversions for exported asm symbols

[Michal Marek]

On 2016-11-01 15:36, Nicholas Piggin wrote:
> On Tue, 1 Nov 2016 15:19:59 +0100
> Michal Marek <mmarek@suse.com> wrote:
> 
>> On 2016-10-31 12:14, Nicholas Piggin wrote:
>>> +# This is convoluted. The .S file must first be preprocessed to run guards and
>>> +# expand names, then the resulting exports must be constructed into plain
>>> +# EXPORT_SYMBOL(symbol); to build our dummy C file, and that gets preprocessed
>>> +# to make the genksyms input.
>>>  #
>>>  # These mirror gensymtypes_c and co above, keep them in synch.
>>>  cmd_gensymtypes_S =                                                         \
>>>      (echo "\#include <linux/kernel.h>" ;                                    \
>>>       echo "\#include <asm/asm-prototypes.h>" ;                              \
>>> -     grep EXPORT_SYMBOL $< | sed 's/$$/;/' ) |                              \
>>> +    $(CPP) $(a_flags) $< |                                                  \
>>> +     grep ^___EXPORT_SYMBOL |                                               \
>>> +     sed 's/___EXPORT_SYMBOL \([a-zA-Z0-9_]*\),.*/EXPORT_SYMBOL(\1);/' ) |  \  
>>
>> Is this sed pass necessary? Just add -D__GENKSYMS__ also to the first
>> cpp run and EXPORT_SYMBOL will stay intact.
> 
> I can't see how it would work if asm-generic/export.h doesn't have any
> tests for GENKSYMS.
> 
>> Anyway, I'm going to merge your patch 2/2 now.
> 
> Okay. We should come to some resolution of the preprocessing issue too.
> I'm logging off for tonight, but if you want to tweak or redo the
> incremental patch, feel free.

I applied the folded patch as is. The preprocessing is not pretty, but
the 4.9 regression is more important to be fixed.

Thanks,
Michal