[PATCH v3 0/5] Adds -Wshadow on KBUILD_HOSTCFLAGS and fix warnings

[PATCH v3 0/5] Adds -Wshadow on KBUILD_HOSTCFLAGS and fix warnings

[Leonardo Bras]

This patchset add -Wshadow on KBUILD_HOSTCFLAGS and fixes
all code that show this warning.

Changes in v3:
- Better Cover letter
- Better commit message for patch 1/5.
- Fixes what should change on patch 3/5
- Removes accent of my second name (better for searching at lkml.org)


v2: https://lkml.org/lkml/2018/10/23/151
v1: https://lkml.org/lkml/2018/10/17/169

Leonardo Bras (5):
  x86/vdso: Renames variable to fix shadow warning.
  kbuild: Removes unnecessary shadowed local variable.
  Creates macro to avoid variable shadowing
  modpost: Changes parameter name to avoid shadowing.
  Adds -Wshadow on KBUILD_HOSTCFLAGS

 Makefile                     |  2 +-
 arch/x86/entry/vdso/vdso2c.h | 13 +++++++------
 scripts/asn1_compiler.c      |  2 +-
 scripts/mod/file2alias.c     | 19 +++++++++++++++----
 scripts/mod/modpost.c        |  4 ++--
 5 files changed, 26 insertions(+), 14 deletions(-)

-- 
2.19.1

[PATCH v3 1/5] x86/vdso: Renames variable to fix shadow warning.

[Leonardo Bras]

The go32() and go64() functions have an argument and a local variable
called ‘name’.  Rename both to clarify the code and to fix a warning
with -Wshadow.

Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
---
 arch/x86/entry/vdso/vdso2c.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/x86/entry/vdso/vdso2c.h b/arch/x86/entry/vdso/vdso2c.h
index fa847a620f40..a20b134de2a8 100644
--- a/arch/x86/entry/vdso/vdso2c.h
+++ b/arch/x86/entry/vdso/vdso2c.h
@@ -7,7 +7,7 @@
 
 static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
 			 void *stripped_addr, size_t stripped_len,
-			 FILE *outfile, const char *name)
+			 FILE *outfile, const char *image_name)
 {
 	int found_load = 0;
 	unsigned long load_size = -1;  /* Work around bogus warning */
@@ -93,11 +93,12 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
 		int k;
 		ELF(Sym) *sym = raw_addr + GET_LE(&symtab_hdr->sh_offset) +
 			GET_LE(&symtab_hdr->sh_entsize) * i;
-		const char *name = raw_addr + GET_LE(&strtab_hdr->sh_offset) +
-			GET_LE(&sym->st_name);
+		const char *sym_name = raw_addr +
+				       GET_LE(&strtab_hdr->sh_offset) +
+				       GET_LE(&sym->st_name);
 
 		for (k = 0; k < NSYMS; k++) {
-			if (!strcmp(name, required_syms[k].name)) {
+			if (!strcmp(sym_name, required_syms[k].name)) {
 				if (syms[k]) {
 					fail("duplicate symbol %s\n",
 					     required_syms[k].name);
@@ -134,7 +135,7 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
 	if (syms[sym_vvar_start] % 4096)
 		fail("vvar_begin must be a multiple of 4096\n");
 
-	if (!name) {
+	if (!image_name) {
 		fwrite(stripped_addr, stripped_len, 1, outfile);
 		return;
 	}
@@ -157,7 +158,7 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
 	}
 	fprintf(outfile, "\n};\n\n");
 
-	fprintf(outfile, "const struct vdso_image %s = {\n", name);
+	fprintf(outfile, "const struct vdso_image %s = {\n", image_name);
 	fprintf(outfile, "\t.data = raw_data,\n");
 	fprintf(outfile, "\t.size = %lu,\n", mapping_size);
 	if (alt_sec) {
-- 
2.19.1

Re: [PATCH v3 1/5] x86/vdso: Renames variable to fix shadow warning.

[Masahiro Yamada]

Hi Ingo,


On Wed, Oct 24, 2018 at 1:04 PM Leonardo Bras <leobras.c@gmail.com> wrote:
>
> The go32() and go64() functions have an argument and a local variable
> called ‘name’.  Rename both to clarify the code and to fix a warning
> with -Wshadow.
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>


Please take care of this in x86 tree instead of kbuild.


Thanks.


> ---
>  arch/x86/entry/vdso/vdso2c.h | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/entry/vdso/vdso2c.h b/arch/x86/entry/vdso/vdso2c.h
> index fa847a620f40..a20b134de2a8 100644
> --- a/arch/x86/entry/vdso/vdso2c.h
> +++ b/arch/x86/entry/vdso/vdso2c.h
> @@ -7,7 +7,7 @@
>
>  static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
>                          void *stripped_addr, size_t stripped_len,
> -                        FILE *outfile, const char *name)
> +                        FILE *outfile, const char *image_name)
>  {
>         int found_load = 0;
>         unsigned long load_size = -1;  /* Work around bogus warning */
> @@ -93,11 +93,12 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
>                 int k;
>                 ELF(Sym) *sym = raw_addr + GET_LE(&symtab_hdr->sh_offset) +
>                         GET_LE(&symtab_hdr->sh_entsize) * i;
> -               const char *name = raw_addr + GET_LE(&strtab_hdr->sh_offset) +
> -                       GET_LE(&sym->st_name);
> +               const char *sym_name = raw_addr +
> +                                      GET_LE(&strtab_hdr->sh_offset) +
> +                                      GET_LE(&sym->st_name);
>
>                 for (k = 0; k < NSYMS; k++) {
> -                       if (!strcmp(name, required_syms[k].name)) {
> +                       if (!strcmp(sym_name, required_syms[k].name)) {
>                                 if (syms[k]) {
>                                         fail("duplicate symbol %s\n",
>                                              required_syms[k].name);
> @@ -134,7 +135,7 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
>         if (syms[sym_vvar_start] % 4096)
>                 fail("vvar_begin must be a multiple of 4096\n");
>
> -       if (!name) {
> +       if (!image_name) {
>                 fwrite(stripped_addr, stripped_len, 1, outfile);
>                 return;
>         }
> @@ -157,7 +158,7 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
>         }
>         fprintf(outfile, "\n};\n\n");
>
> -       fprintf(outfile, "const struct vdso_image %s = {\n", name);
> +       fprintf(outfile, "const struct vdso_image %s = {\n", image_name);
>         fprintf(outfile, "\t.data = raw_data,\n");
>         fprintf(outfile, "\t.size = %lu,\n", mapping_size);
>         if (alt_sec) {
> --
> 2.19.1
>


-- 
Best Regards
Masahiro Yamada

[PATCH v3 2/5] kbuild: Removes unnecessary shadowed local variable.

[Leonardo Bras]

Removes an unnecessary shadowed local variable (start).
It was used only once, with the same value it was started before
the if block.

Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
---
 scripts/asn1_compiler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/asn1_compiler.c b/scripts/asn1_compiler.c
index c146020fc783..1b28787028d3 100644
--- a/scripts/asn1_compiler.c
+++ b/scripts/asn1_compiler.c
@@ -413,7 +413,7 @@ static void tokenise(char *buffer, char *end)
 
 			/* Handle string tokens */
 			if (isalpha(*p)) {
-				const char **dir, *start = p;
+				const char **dir;
 
 				/* Can be a directive, type name or element
 				 * name.  Find the end of the name.
-- 
2.19.1

Re: [PATCH v3 2/5] kbuild: Removes unnecessary shadowed local variable.

[Masahiro Yamada]

On Wed, Oct 24, 2018 at 1:04 PM Leonardo Bras <leobras.c@gmail.com> wrote:
>
> Removes an unnecessary shadowed local variable (start).
> It was used only once, with the same value it was started before
> the if block.
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>



Applied to linux-kbuild
with some fixups in the subject.

Please do not add a period to the end of the subject.






> ---
>  scripts/asn1_compiler.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/asn1_compiler.c b/scripts/asn1_compiler.c
> index c146020fc783..1b28787028d3 100644
> --- a/scripts/asn1_compiler.c
> +++ b/scripts/asn1_compiler.c
> @@ -413,7 +413,7 @@ static void tokenise(char *buffer, char *end)
>
>                         /* Handle string tokens */
>                         if (isalpha(*p)) {
> -                               const char **dir, *start = p;
> +                               const char **dir;
>
>                                 /* Can be a directive, type name or element
>                                  * name.  Find the end of the name.
> --
> 2.19.1
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3 2/5] kbuild: Removes unnecessary shadowed local variable.

[Leonardo Bras]

Sorry, I will take care next time.

Thank you,

Leonardo Bras

On Sun, Oct 28, 2018 at 1:37 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Wed, Oct 24, 2018 at 1:04 PM Leonardo Bras <leobras.c@gmail.com> wrote:
> >
> > Removes an unnecessary shadowed local variable (start).
> > It was used only once, with the same value it was started before
> > the if block.
> >
> > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
>
>
>
> Applied to linux-kbuild
> with some fixups in the subject.
>
> Please do not add a period to the end of the subject.
>
>
>
>
>
>
> > ---
> >  scripts/asn1_compiler.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/asn1_compiler.c b/scripts/asn1_compiler.c
> > index c146020fc783..1b28787028d3 100644
> > --- a/scripts/asn1_compiler.c
> > +++ b/scripts/asn1_compiler.c
> > @@ -413,7 +413,7 @@ static void tokenise(char *buffer, char *end)
> >
> >                         /* Handle string tokens */
> >                         if (isalpha(*p)) {
> > -                               const char **dir, *start = p;
> > +                               const char **dir;
> >
> >                                 /* Can be a directive, type name or element
> >                                  * name.  Find the end of the name.
> > --
> > 2.19.1
> >
>
>
> --
> Best Regards
> Masahiro Yamada

[PATCH v3 3/5] Creates macro to avoid variable shadowing

[Leonardo Bras]

Creates DEF_FIELD_ADDR_VAR as a more generic version of the DEF_FIELD_ADD
macro, allowing usage of a variable name other than the struct element name.
Also, sets DEF_FIELD_ADDR as a specific usage of DEF_FILD_ADDR_VAR in which
the var name is the same as the struct element name.
Then, makes use of DEF_FIELD_ADDR_VAR to create a variable of another name,
in order to avoid variable shadowing.

Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
---
 scripts/mod/file2alias.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 7be43697ff84..ed468313ddeb 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -95,12 +95,20 @@ extern struct devtable *__start___devtable[], *__stop___devtable[];
  */
 #define DEF_FIELD(m, devid, f) \
 	typeof(((struct devid *)0)->f) f = TO_NATIVE(*(typeof(f) *)((m) + OFF_##devid##_##f))
+
+/* Define a variable v that holds the address of field f of struct devid
+ * based at address m.  Due to the way typeof works, for a field of type
+ * T[N] the variable has type T(*)[N], _not_ T*.
+ */
+#define DEF_FIELD_ADDR_VAR(m, devid, f, v) \
+	typeof(((struct devid *)0)->f) *v = ((m) + OFF_##devid##_##f)
+
 /* Define a variable f that holds the address of field f of struct devid
  * based at address m.  Due to the way typeof works, for a field of type
  * T[N] the variable has type T(*)[N], _not_ T*.
  */
 #define DEF_FIELD_ADDR(m, devid, f) \
-	typeof(((struct devid *)0)->f) *f = ((m) + OFF_##devid##_##f)
+	DEF_FIELD_ADDR_VAR(m, devid, f, f)
 
 /* Add a table entry.  We test function type matches while we're here. */
 #define ADD_TO_DEVTABLE(device_id, type, function) \
@@ -644,7 +652,7 @@ static void do_pnp_card_entries(void *symval, unsigned long size,
 
 	for (i = 0; i < count; i++) {
 		unsigned int j;
-		DEF_FIELD_ADDR(symval + i*id_size, pnp_card_device_id, devs);
+		DEF_FIELD_ADDR(symval + i * id_size, pnp_card_device_id, devs);
 
 		for (j = 0; j < PNP_MAX_DEVICES; j++) {
 			const char *id = (char *)(*devs)[j].id;
@@ -656,10 +664,13 @@ static void do_pnp_card_entries(void *symval, unsigned long size,
 
 			/* find duplicate, already added value */
 			for (i2 = 0; i2 < i && !dup; i2++) {
-				DEF_FIELD_ADDR(symval + i2*id_size, pnp_card_device_id, devs);
+				DEF_FIELD_ADDR_VAR(symval + i2 * id_size,
+						   pnp_card_device_id,
+						   devs, devs_dup);
 
 				for (j2 = 0; j2 < PNP_MAX_DEVICES; j2++) {
-					const char *id2 = (char *)(*devs)[j2].id;
+					const char *id2 =
+						(char *)(*devs_dup)[j2].id;
 
 					if (!id2[0])
 						break;
-- 
2.19.1

[PATCH v3 4/5] modpost: Changes parameter name to avoid shadowing.

[Leonardo Bras]

Changes the parameter name to avoid shadowing a variable.

Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
---
 scripts/mod/modpost.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 0d998c54564d..368fe42340df 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2228,13 +2228,13 @@ static int add_versions(struct buffer *b, struct module *mod)
 }
 
 static void add_depends(struct buffer *b, struct module *mod,
-			struct module *modules)
+			struct module *module_list)
 {
 	struct symbol *s;
 	struct module *m;
 	int first = 1;
 
-	for (m = modules; m; m = m->next)
+	for (m = module_list; m; m = m->next)
 		m->seen = is_vmlinux(m->name);
 
 	buf_printf(b, "\n");
-- 
2.19.1

Re: [PATCH v3 4/5] modpost: Changes parameter name to avoid shadowing.

[Masahiro Yamada]

On Wed, Oct 24, 2018 at 1:05 PM Leonardo Bras <leobras.c@gmail.com> wrote:
>
> Changes the parameter name to avoid shadowing a variable.
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>


For this one, I'd rather like to see code refactoring
than renaming the variable.

I will take a closer look.




> ---
>  scripts/mod/modpost.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 0d998c54564d..368fe42340df 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2228,13 +2228,13 @@ static int add_versions(struct buffer *b, struct module *mod)
>  }
>
>  static void add_depends(struct buffer *b, struct module *mod,
> -                       struct module *modules)
> +                       struct module *module_list)
>  {
>         struct symbol *s;
>         struct module *m;
>         int first = 1;
>
> -       for (m = modules; m; m = m->next)
> +       for (m = module_list; m; m = m->next)
>                 m->seen = is_vmlinux(m->name);
>
>         buf_printf(b, "\n");
> --
> 2.19.1
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3 4/5] modpost: Changes parameter name to avoid shadowing.

[Leonardo Bras]

Em seg, 2018-10-29 às 01:42 +0900, Masahiro Yamada escreveu:
> On Wed, Oct 24, 2018 at 1:05 PM Leonardo Bras <leobras.c@gmail.com>
> wrote:
> > Changes the parameter name to avoid shadowing a variable.
> > 
> > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> 
> For this one, I'd rather like to see code refactoring
> than renaming the variable.
> 
> I will take a closer look.


What do you suggest to refactor?
I volunteer for this work.

Leonardo Bras

Re: [PATCH v3 4/5] modpost: Changes parameter name to avoid shadowing.

[Leonardo Bras]

Em qua, 2018-10-31 às 20:24 -0300, Leonardo Bras escreveu:
> Em seg, 2018-10-29 às 01:42 +0900, Masahiro Yamada escreveu:
> > On Wed, Oct 24, 2018 at 1:05 PM Leonardo Bras <leobras.c@gmail.com>
> > wrote:
> > > Changes the parameter name to avoid shadowing a variable.
> > > 
> > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > 
> > For this one, I'd rather like to see code refactoring
> > than renaming the variable.
> > 
> > I will take a closer look.
> 
> What do you suggest to refactor?
> I volunteer for this work.
> 
> Leonardo Bras
> 

I refactored the code to move the global variable *modules to a local
context at function main, and changed the functions to accept *modules
as a parameter.

Is that what you had in mind?

https://lkml.org/lkml/2018/11/1/725

Thanks for reading,

Leonardo Bras

[PATCH v3 5/5] Adds -Wshadow on KBUILD_HOSTCFLAGS

[Leonardo Bras]

Adds -Wshadow on KBUILD_HOSTCFLAGS to show shadow warnings
on tools built for HOST.

Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index e8b599b4dcde..3edae5d359b5 100644
--- a/Makefile
+++ b/Makefile
@@ -360,7 +360,7 @@ HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null)
 
 HOSTCC       = gcc
 HOSTCXX      = g++
-KBUILD_HOSTCFLAGS   := -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 \
+KBUILD_HOSTCFLAGS   := -Wall -Wshadow -Wmissing-prototypes -Wstrict-prototypes -O2 \
 		-fomit-frame-pointer -std=gnu89 $(HOST_LFS_CFLAGS) \
 		$(HOSTCFLAGS)
 KBUILD_HOSTCXXFLAGS := -O2 $(HOST_LFS_CFLAGS) $(HOSTCXXFLAGS)
-- 
2.19.1