| Message ID | 20220801093902.1506297-1-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] modpost: shorten warning messages in report_sec_mismatch() | expand |
On Mon, Aug 1, 2022 at 2:41 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > Each section mismatch results in long warning messages. Too much. :( Yes; they are too verbose. That said, I have found the recommendations about annotations for function attributes handy in the past and would be sad to see them go. They remind me of "note" diagnostics from the compiler that add additional context to "warning"/"error" diagnostics on what the recommended next steps are for fixing them. Is there a "happy middle ground" here? > > Make each warning fit in one line, and remove a lot of messy code. > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > --- > > scripts/mod/modpost.c | 179 +++--------------------------------------- > 1 file changed, 9 insertions(+), 170 deletions(-) > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index a8ee27496da7..9e8ae2636ec1 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -1238,42 +1238,6 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr, > return near; > } > > -/* > - * Convert a section name to the function/data attribute > - * .init.text => __init > - * .memexitconst => __memconst > - * etc. > - * > - * The memory of returned value has been allocated on a heap. The user of this > - * method should free it after usage. > -*/ > -static char *sec2annotation(const char *s) > -{ > - if (match(s, init_exit_sections)) { > - char *p = NOFAIL(malloc(20)); > - char *r = p; > - > - *p++ = '_'; > - *p++ = '_'; > - if (*s == '.') > - s++; > - while (*s && *s != '.') > - *p++ = *s++; > - *p = '\0'; > - if (*s == '.') > - s++; > - if (strstr(s, "rodata") != NULL) > - strcat(p, "const "); > - else if (strstr(s, "data") != NULL) > - strcat(p, "data "); > - else > - strcat(p, " "); > - return r; > - } else { > - return NOFAIL(strdup("")); > - } > -} > - > static int is_function(Elf_Sym *sym) > { > if (sym) > @@ -1282,19 +1246,6 @@ static int is_function(Elf_Sym *sym) > return -1; > } > > -static void print_section_list(const char * const list[20]) > -{ > - const char *const *s = list; > - > - while (*s) { > - fprintf(stderr, "%s", *s); > - s++; > - if (*s) > - fprintf(stderr, ", "); > - } > - fprintf(stderr, "\n"); > -} > - > static inline void get_pretty_name(int is_func, const char** name, const char** name_p) > { > switch (is_func) { > @@ -1312,141 +1263,31 @@ static inline void get_pretty_name(int is_func, const char** name, const char** > static void report_sec_mismatch(const char *modname, > const struct sectioncheck *mismatch, > const char *fromsec, > - unsigned long long fromaddr, > const char *fromsym, > - int from_is_func, > - const char *tosec, const char *tosym, > - int to_is_func) > + const char *tosec, const char *tosym) > { > - const char *from, *from_p; > - const char *to, *to_p; > - char *prl_from; > - char *prl_to; > - > sec_mismatch_count++; > > - get_pretty_name(from_is_func, &from, &from_p); > - get_pretty_name(to_is_func, &to, &to_p); > - > - warn("%s(%s+0x%llx): Section mismatch in reference from the %s %s%s " > - "to the %s %s:%s%s\n", > - modname, fromsec, fromaddr, from, fromsym, from_p, to, tosec, > - tosym, to_p); > - > switch (mismatch->mismatch) { > case TEXT_TO_ANY_INIT: > - prl_from = sec2annotation(fromsec); > - prl_to = sec2annotation(tosec); > - fprintf(stderr, > - "The function %s%s() references\n" > - "the %s %s%s%s.\n" > - "This is often because %s lacks a %s\n" > - "annotation or the annotation of %s is wrong.\n", > - prl_from, fromsym, > - to, prl_to, tosym, to_p, > - fromsym, prl_to, tosym); > - free(prl_from); > - free(prl_to); > - break; > - case DATA_TO_ANY_INIT: { > - prl_to = sec2annotation(tosec); > - fprintf(stderr, > - "The variable %s references\n" > - "the %s %s%s%s\n" > - "If the reference is valid then annotate the\n" > - "variable with __init* or __refdata (see linux/init.h) " > - "or name the variable:\n", > - fromsym, to, prl_to, tosym, to_p); > - print_section_list(mismatch->symbol_white_list); > - free(prl_to); > - break; > - } > + case DATA_TO_ANY_INIT: > case TEXT_TO_ANY_EXIT: > - prl_to = sec2annotation(tosec); > - fprintf(stderr, > - "The function %s() references a %s in an exit section.\n" > - "Often the %s %s%s has valid usage outside the exit section\n" > - "and the fix is to remove the %sannotation of %s.\n", > - fromsym, to, to, tosym, to_p, prl_to, tosym); > - free(prl_to); > - break; > - case DATA_TO_ANY_EXIT: { > - prl_to = sec2annotation(tosec); > - fprintf(stderr, > - "The variable %s references\n" > - "the %s %s%s%s\n" > - "If the reference is valid then annotate the\n" > - "variable with __exit* (see linux/init.h) or " > - "name the variable:\n", > - fromsym, to, prl_to, tosym, to_p); > - print_section_list(mismatch->symbol_white_list); > - free(prl_to); > - break; > - } > + case DATA_TO_ANY_EXIT: > case XXXINIT_TO_SOME_INIT: > case XXXEXIT_TO_SOME_EXIT: > - prl_from = sec2annotation(fromsec); > - prl_to = sec2annotation(tosec); > - fprintf(stderr, > - "The %s %s%s%s references\n" > - "a %s %s%s%s.\n" > - "If %s is only used by %s then\n" > - "annotate %s with a matching annotation.\n", > - from, prl_from, fromsym, from_p, > - to, prl_to, tosym, to_p, > - tosym, fromsym, tosym); > - free(prl_from); > - free(prl_to); > - break; > case ANY_INIT_TO_ANY_EXIT: > - prl_from = sec2annotation(fromsec); > - prl_to = sec2annotation(tosec); > - fprintf(stderr, > - "The %s %s%s%s references\n" > - "a %s %s%s%s.\n" > - "This is often seen when error handling " > - "in the init function\n" > - "uses functionality in the exit path.\n" > - "The fix is often to remove the %sannotation of\n" > - "%s%s so it may be used outside an exit section.\n", > - from, prl_from, fromsym, from_p, > - to, prl_to, tosym, to_p, > - prl_to, tosym, to_p); > - free(prl_from); > - free(prl_to); > - break; > case ANY_EXIT_TO_ANY_INIT: > - prl_from = sec2annotation(fromsec); > - prl_to = sec2annotation(tosec); > - fprintf(stderr, > - "The %s %s%s%s references\n" > - "a %s %s%s%s.\n" > - "This is often seen when error handling " > - "in the exit function\n" > - "uses functionality in the init path.\n" > - "The fix is often to remove the %sannotation of\n" > - "%s%s so it may be used outside an init section.\n", > - from, prl_from, fromsym, from_p, > - to, prl_to, tosym, to_p, > - prl_to, tosym, to_p); > - free(prl_from); > - free(prl_to); > + warn("%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n", > + modname, fromsym, fromsec, tosym, tosec); > break; > case EXPORT_TO_INIT_EXIT: > - prl_to = sec2annotation(tosec); > - fprintf(stderr, > - "The symbol %s is exported and annotated %s\n" > - "Fix this by removing the %sannotation of %s " > - "or drop the export.\n", > - tosym, prl_to, prl_to, tosym); > - free(prl_to); > + warn("%s: EXPORT_SYMBOL used for init/exit symbol: %s (section: %s)\n", > + modname, tosym, tosec); > break; > case EXTABLE_TO_NON_TEXT: > - fatal("There's a special handler for this mismatch type, " > - "we should never get here."); > + fatal("There's a special handler for this mismatch type, we should never get here.\n"); > break; > } > - fprintf(stderr, "\n"); > } > > static void default_mismatch_handler(const char *modname, struct elf_info *elf, > @@ -1470,9 +1311,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf, > if (secref_whitelist(mismatch, > fromsec, fromsym, tosec, tosym)) { > report_sec_mismatch(modname, mismatch, > - fromsec, r->r_offset, fromsym, > - is_function(from), tosec, tosym, > - is_function(to)); > + fromsec, fromsym, tosec, tosym); > } > } > > -- > 2.34.1 >
On Wed, Aug 3, 2022 at 3:13 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Mon, Aug 1, 2022 at 2:41 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > Each section mismatch results in long warning messages. Too much. > > :( > > Yes; they are too verbose. That said, I have found the > recommendations about annotations for function attributes handy in the > past and would be sad to see them go. They remind me of "note" > diagnostics from the compiler that add additional context to > "warning"/"error" diagnostics on what the recommended next steps are > for fixing them. > > Is there a "happy middle ground" here? I do not know. modpost became painfully ugly. Moreover, the current hint is not necessarily precise. ("lacks a __initdata" in the following) [sample code] int dummy __initdata; void set_dummy(void) { dummy = 1; } [warning] WARNING: modpost: vmlinux.o(.text+0x194412): Section mismatch in reference from the function set_dummy() to the variable .init.data:dummy The function set_dummy() references the variable __initdata dummy. This is often because set_dummy lacks a __initdata annotation or the annotation of dummy is wrong. -- Best Regards Masahiro Yamada
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index a8ee27496da7..9e8ae2636ec1 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -1238,42 +1238,6 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr, return near; } -/* - * Convert a section name to the function/data attribute - * .init.text => __init - * .memexitconst => __memconst - * etc. - * - * The memory of returned value has been allocated on a heap. The user of this - * method should free it after usage. -*/ -static char *sec2annotation(const char *s) -{ - if (match(s, init_exit_sections)) { - char *p = NOFAIL(malloc(20)); - char *r = p; - - *p++ = '_'; - *p++ = '_'; - if (*s == '.') - s++; - while (*s && *s != '.') - *p++ = *s++; - *p = '\0'; - if (*s == '.') - s++; - if (strstr(s, "rodata") != NULL) - strcat(p, "const "); - else if (strstr(s, "data") != NULL) - strcat(p, "data "); - else - strcat(p, " "); - return r; - } else { - return NOFAIL(strdup("")); - } -} - static int is_function(Elf_Sym *sym) { if (sym) @@ -1282,19 +1246,6 @@ static int is_function(Elf_Sym *sym) return -1; } -static void print_section_list(const char * const list[20]) -{ - const char *const *s = list; - - while (*s) { - fprintf(stderr, "%s", *s); - s++; - if (*s) - fprintf(stderr, ", "); - } - fprintf(stderr, "\n"); -} - static inline void get_pretty_name(int is_func, const char** name, const char** name_p) { switch (is_func) { @@ -1312,141 +1263,31 @@ static inline void get_pretty_name(int is_func, const char** name, const char** static void report_sec_mismatch(const char *modname, const struct sectioncheck *mismatch, const char *fromsec, - unsigned long long fromaddr, const char *fromsym, - int from_is_func, - const char *tosec, const char *tosym, - int to_is_func) + const char *tosec, const char *tosym) { - const char *from, *from_p; - const char *to, *to_p; - char *prl_from; - char *prl_to; - sec_mismatch_count++; - get_pretty_name(from_is_func, &from, &from_p); - get_pretty_name(to_is_func, &to, &to_p); - - warn("%s(%s+0x%llx): Section mismatch in reference from the %s %s%s " - "to the %s %s:%s%s\n", - modname, fromsec, fromaddr, from, fromsym, from_p, to, tosec, - tosym, to_p); - switch (mismatch->mismatch) { case TEXT_TO_ANY_INIT: - prl_from = sec2annotation(fromsec); - prl_to = sec2annotation(tosec); - fprintf(stderr, - "The function %s%s() references\n" - "the %s %s%s%s.\n" - "This is often because %s lacks a %s\n" - "annotation or the annotation of %s is wrong.\n", - prl_from, fromsym, - to, prl_to, tosym, to_p, - fromsym, prl_to, tosym); - free(prl_from); - free(prl_to); - break; - case DATA_TO_ANY_INIT: { - prl_to = sec2annotation(tosec); - fprintf(stderr, - "The variable %s references\n" - "the %s %s%s%s\n" - "If the reference is valid then annotate the\n" - "variable with __init* or __refdata (see linux/init.h) " - "or name the variable:\n", - fromsym, to, prl_to, tosym, to_p); - print_section_list(mismatch->symbol_white_list); - free(prl_to); - break; - } + case DATA_TO_ANY_INIT: case TEXT_TO_ANY_EXIT: - prl_to = sec2annotation(tosec); - fprintf(stderr, - "The function %s() references a %s in an exit section.\n" - "Often the %s %s%s has valid usage outside the exit section\n" - "and the fix is to remove the %sannotation of %s.\n", - fromsym, to, to, tosym, to_p, prl_to, tosym); - free(prl_to); - break; - case DATA_TO_ANY_EXIT: { - prl_to = sec2annotation(tosec); - fprintf(stderr, - "The variable %s references\n" - "the %s %s%s%s\n" - "If the reference is valid then annotate the\n" - "variable with __exit* (see linux/init.h) or " - "name the variable:\n", - fromsym, to, prl_to, tosym, to_p); - print_section_list(mismatch->symbol_white_list); - free(prl_to); - break; - } + case DATA_TO_ANY_EXIT: case XXXINIT_TO_SOME_INIT: case XXXEXIT_TO_SOME_EXIT: - prl_from = sec2annotation(fromsec); - prl_to = sec2annotation(tosec); - fprintf(stderr, - "The %s %s%s%s references\n" - "a %s %s%s%s.\n" - "If %s is only used by %s then\n" - "annotate %s with a matching annotation.\n", - from, prl_from, fromsym, from_p, - to, prl_to, tosym, to_p, - tosym, fromsym, tosym); - free(prl_from); - free(prl_to); - break; case ANY_INIT_TO_ANY_EXIT: - prl_from = sec2annotation(fromsec); - prl_to = sec2annotation(tosec); - fprintf(stderr, - "The %s %s%s%s references\n" - "a %s %s%s%s.\n" - "This is often seen when error handling " - "in the init function\n" - "uses functionality in the exit path.\n" - "The fix is often to remove the %sannotation of\n" - "%s%s so it may be used outside an exit section.\n", - from, prl_from, fromsym, from_p, - to, prl_to, tosym, to_p, - prl_to, tosym, to_p); - free(prl_from); - free(prl_to); - break; case ANY_EXIT_TO_ANY_INIT: - prl_from = sec2annotation(fromsec); - prl_to = sec2annotation(tosec); - fprintf(stderr, - "The %s %s%s%s references\n" - "a %s %s%s%s.\n" - "This is often seen when error handling " - "in the exit function\n" - "uses functionality in the init path.\n" - "The fix is often to remove the %sannotation of\n" - "%s%s so it may be used outside an init section.\n", - from, prl_from, fromsym, from_p, - to, prl_to, tosym, to_p, - prl_to, tosym, to_p); - free(prl_from); - free(prl_to); + warn("%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n", + modname, fromsym, fromsec, tosym, tosec); break; case EXPORT_TO_INIT_EXIT: - prl_to = sec2annotation(tosec); - fprintf(stderr, - "The symbol %s is exported and annotated %s\n" - "Fix this by removing the %sannotation of %s " - "or drop the export.\n", - tosym, prl_to, prl_to, tosym); - free(prl_to); + warn("%s: EXPORT_SYMBOL used for init/exit symbol: %s (section: %s)\n", + modname, tosym, tosec); break; case EXTABLE_TO_NON_TEXT: - fatal("There's a special handler for this mismatch type, " - "we should never get here."); + fatal("There's a special handler for this mismatch type, we should never get here.\n"); break; } - fprintf(stderr, "\n"); } static void default_mismatch_handler(const char *modname, struct elf_info *elf, @@ -1470,9 +1311,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf, if (secref_whitelist(mismatch, fromsec, fromsym, tosec, tosym)) { report_sec_mismatch(modname, mismatch, - fromsec, r->r_offset, fromsym, - is_function(from), tosec, tosym, - is_function(to)); + fromsec, fromsym, tosec, tosym); } }
Each section mismatch results in long warning messages. Too much. Make each warning fit in one line, and remove a lot of messy code. Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- scripts/mod/modpost.c | 179 +++--------------------------------------- 1 file changed, 9 insertions(+), 170 deletions(-)