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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Changes the parameter name to avoid shadowing a variable.
Signed-off-by: Leonardo Bras <[email protected]>
---
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
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 <[email protected]>
---
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
Adds -Wshadow on KBUILD_HOSTCFLAGS to show shadow warnings
on tools built for HOST.
Signed-off-by: Leonardo Bras <[email protected]>
---
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
Hi Ingo,
On Wed, Oct 24, 2018 at 1:04 PM Leonardo Bras <[email protected]> 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 <[email protected]>
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
On Wed, Oct 24, 2018 at 1:04 PM Leonardo Bras <[email protected]> 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 <[email protected]>
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
On Wed, Oct 24, 2018 at 1:05 PM Leonardo Bras <[email protected]> wrote:
>
> Changes the parameter name to avoid shadowing a variable.
>
> Signed-off-by: Leonardo Bras <[email protected]>
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
Sorry, I will take care next time.
Thank you,
Leonardo Bras
On Sun, Oct 28, 2018 at 1:37 PM Masahiro Yamada
<[email protected]> wrote:
>
> On Wed, Oct 24, 2018 at 1:04 PM Leonardo Bras <[email protected]> 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 <[email protected]>
>
>
>
> 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
Em seg, 2018-10-29 às 01:42 +0900, Masahiro Yamada escreveu:
> On Wed, Oct 24, 2018 at 1:05 PM Leonardo Bras <[email protected]>
> wrote:
> > Changes the parameter name to avoid shadowing a variable.
> >
> > Signed-off-by: Leonardo Bras <[email protected]>
>
> 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