* [PATCH 1/4] modpost: shorten warning messages in report_sec_mismatch()
@ 2022-08-01 9:38 Masahiro Yamada
2022-08-01 9:39 ` [PATCH 2/4] modpost: add PATTERNS() helper macro Masahiro Yamada
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Masahiro Yamada @ 2022-08-01 9:38 UTC (permalink / raw)
To: linux-kbuild
Cc: Masahiro Yamada, Michal Marek, Nick Desaulniers, linux-kernel
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/4] modpost: add PATTERNS() helper macro
2022-08-01 9:38 [PATCH 1/4] modpost: shorten warning messages in report_sec_mismatch() Masahiro Yamada
@ 2022-08-01 9:39 ` Masahiro Yamada
2022-08-01 9:39 ` [PATCH 3/4] modpost: remove unneeded .symbol_white_list initializers Masahiro Yamada
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Masahiro Yamada @ 2022-08-01 9:39 UTC (permalink / raw)
To: linux-kbuild
Cc: Masahiro Yamada, Michal Marek, Nick Desaulniers, linux-kernel
This will be useful to define a NULL-terminated array inside a function
call.
Currently, string arrays passed to match() are defined in separate
places:
static const char *const init_sections[] = { ALL_INIT_SECTIONS, NULL };
static const char *const text_sections[] = { ALL_TEXT_SECTIONS, NULL };
static const char *const optim_symbols[] = { "*.constprop.*", NULL };
...
/* Check for pattern 5 */
if (match(fromsec, text_sections) &&
match(tosec, init_sections) &&
match(fromsym, optim_symbols))
return 0;
With the new helper macro, you can list the patterns directly in the
function call, like this:
/* Check for pattern 5 */
if (match(fromsec, PATTERNS(ALL_TEXT_SECTIONS)) &&
match(tosec, PATTERNS(ALL_INIT_SECTIONS)) &&
match(fromsym, PATTERNS("*.contprop.*")))
return 0;
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 9e8ae2636ec1..c2949a1a0d5e 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -746,6 +746,13 @@ static bool match(const char *string, const char *const patterns[])
return false;
}
+/* useful to pass patterns to match() directly */
+#define PATTERNS(...) \
+ ({ \
+ static const char *const patterns[] = {__VA_ARGS__, NULL}; \
+ patterns; \
+ })
+
/* sections that we do not want to do full section mismatch check on */
static const char *const section_white_list[] =
{
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/4] modpost: remove unneeded .symbol_white_list initializers
2022-08-01 9:38 [PATCH 1/4] modpost: shorten warning messages in report_sec_mismatch() Masahiro Yamada
2022-08-01 9:39 ` [PATCH 2/4] modpost: add PATTERNS() helper macro Masahiro Yamada
@ 2022-08-01 9:39 ` Masahiro Yamada
2022-08-01 9:39 ` [PATCH 4/4] modpost: remove .symbol_white_list field entirely Masahiro Yamada
2022-08-02 18:13 ` [PATCH 1/4] modpost: shorten warning messages in report_sec_mismatch() Nick Desaulniers
3 siblings, 0 replies; 6+ messages in thread
From: Masahiro Yamada @ 2022-08-01 9:39 UTC (permalink / raw)
To: linux-kbuild
Cc: Masahiro Yamada, Michal Marek, Nick Desaulniers, linux-kernel
The ->symbol_white_list field is referenced in secref_whitelist(),
only when 'fromsec' is data_sections.
/* Check for pattern 2 */
if (match(tosec, init_exit_sections) &&
match(fromsec, data_sections) &&
match(fromsym, mismatch->symbol_white_list))
return 0;
If .fromsec is not data sections, the .symbol_white_list member is
not used by anyone.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index c2949a1a0d5e..bcd1319f3097 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -930,7 +930,6 @@ static const struct sectioncheck sectioncheck[] = {
.fromsec = { TEXT_SECTIONS, NULL },
.bad_tosec = { ALL_INIT_SECTIONS, NULL },
.mismatch = TEXT_TO_ANY_INIT,
- .symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
},
{
.fromsec = { DATA_SECTIONS, NULL },
@@ -951,7 +950,6 @@ static const struct sectioncheck sectioncheck[] = {
.fromsec = { TEXT_SECTIONS, NULL },
.bad_tosec = { ALL_EXIT_SECTIONS, NULL },
.mismatch = TEXT_TO_ANY_EXIT,
- .symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
},
{
.fromsec = { DATA_SECTIONS, NULL },
@@ -964,41 +962,35 @@ static const struct sectioncheck sectioncheck[] = {
.fromsec = { ALL_XXXINIT_SECTIONS, NULL },
.bad_tosec = { INIT_SECTIONS, NULL },
.mismatch = XXXINIT_TO_SOME_INIT,
- .symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
},
/* Do not reference exit code/data from memexit code/data */
{
.fromsec = { ALL_XXXEXIT_SECTIONS, NULL },
.bad_tosec = { EXIT_SECTIONS, NULL },
.mismatch = XXXEXIT_TO_SOME_EXIT,
- .symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
},
/* Do not use exit code/data from init code */
{
.fromsec = { ALL_INIT_SECTIONS, NULL },
.bad_tosec = { ALL_EXIT_SECTIONS, NULL },
.mismatch = ANY_INIT_TO_ANY_EXIT,
- .symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
},
/* Do not use init code/data from exit code */
{
.fromsec = { ALL_EXIT_SECTIONS, NULL },
.bad_tosec = { ALL_INIT_SECTIONS, NULL },
.mismatch = ANY_EXIT_TO_ANY_INIT,
- .symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
},
{
.fromsec = { ALL_PCI_INIT_SECTIONS, NULL },
.bad_tosec = { INIT_SECTIONS, NULL },
.mismatch = ANY_INIT_TO_ANY_EXIT,
- .symbol_white_list = { NULL },
},
/* Do not export init/exit functions or data */
{
.fromsec = { "___ksymtab*", NULL },
.bad_tosec = { INIT_SECTIONS, EXIT_SECTIONS, NULL },
.mismatch = EXPORT_TO_INIT_EXIT,
- .symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
},
{
.fromsec = { "__ex_table", NULL },
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 4/4] modpost: remove .symbol_white_list field entirely
2022-08-01 9:38 [PATCH 1/4] modpost: shorten warning messages in report_sec_mismatch() Masahiro Yamada
2022-08-01 9:39 ` [PATCH 2/4] modpost: add PATTERNS() helper macro Masahiro Yamada
2022-08-01 9:39 ` [PATCH 3/4] modpost: remove unneeded .symbol_white_list initializers Masahiro Yamada
@ 2022-08-01 9:39 ` Masahiro Yamada
2022-08-02 18:13 ` [PATCH 1/4] modpost: shorten warning messages in report_sec_mismatch() Nick Desaulniers
3 siblings, 0 replies; 6+ messages in thread
From: Masahiro Yamada @ 2022-08-01 9:39 UTC (permalink / raw)
To: linux-kbuild
Cc: Masahiro Yamada, Michal Marek, Nick Desaulniers, linux-kernel
It is not so useful to have symbol whitelists in arrays. With this
over-engineering, the code is difficult to follow.
Let's do it more directly, and collect the relevant code to one place.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 55 +++++++++++++------------------------------
1 file changed, 16 insertions(+), 39 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index bcd1319f3097..8484c0798f28 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -845,28 +845,12 @@ static const char *const init_data_sections[] =
/* all init sections */
static const char *const init_sections[] = { ALL_INIT_SECTIONS, NULL };
-/* All init and exit sections (code + data) */
-static const char *const init_exit_sections[] =
- {ALL_INIT_SECTIONS, ALL_EXIT_SECTIONS, NULL };
-
/* all text sections */
static const char *const text_sections[] = { ALL_TEXT_SECTIONS, NULL };
/* data section */
static const char *const data_sections[] = { DATA_SECTIONS, NULL };
-
-/* symbols in .data that may refer to init/exit sections */
-#define DEFAULT_SYMBOL_WHITE_LIST \
- "*driver", \
- "*_template", /* scsi uses *_template a lot */ \
- "*_timer", /* arm uses ops structures named _timer a lot */ \
- "*_sht", /* scsi also used *_sht to some extent */ \
- "*_ops", \
- "*_probe", \
- "*_probe_one", \
- "*_console"
-
static const char *const head_sections[] = { ".head.text*", NULL };
static const char *const linker_symbols[] =
{ "__init_begin", "_sinittext", "_einittext", NULL };
@@ -898,9 +882,6 @@ enum mismatch {
*
* @mismatch: Type of mismatch.
*
- * @symbol_white_list: Do not match a relocation to a symbol in this list
- * even if it is targeting a section in @bad_to_sec.
- *
* @handler: Specific handler to call when a match is found. If NULL,
* default_mismatch_handler() will be called.
*
@@ -910,7 +891,6 @@ struct sectioncheck {
const char *bad_tosec[20];
const char *good_tosec[20];
enum mismatch mismatch;
- const char *symbol_white_list[20];
void (*handler)(const char *modname, struct elf_info *elf,
const struct sectioncheck* const mismatch,
Elf_Rela *r, Elf_Sym *sym, const char *fromsec);
@@ -935,16 +915,11 @@ static const struct sectioncheck sectioncheck[] = {
.fromsec = { DATA_SECTIONS, NULL },
.bad_tosec = { ALL_XXXINIT_SECTIONS, NULL },
.mismatch = DATA_TO_ANY_INIT,
- .symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
},
{
.fromsec = { DATA_SECTIONS, NULL },
.bad_tosec = { INIT_SECTIONS, NULL },
.mismatch = DATA_TO_ANY_INIT,
- .symbol_white_list = {
- "*_template", "*_timer", "*_sht", "*_ops",
- "*_probe", "*_probe_one", "*_console", NULL
- },
},
{
.fromsec = { TEXT_SECTIONS, NULL },
@@ -955,7 +930,6 @@ static const struct sectioncheck sectioncheck[] = {
.fromsec = { DATA_SECTIONS, NULL },
.bad_tosec = { ALL_EXIT_SECTIONS, NULL },
.mismatch = DATA_TO_ANY_EXIT,
- .symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
},
/* Do not reference init code/data from meminit code/data */
{
@@ -1051,15 +1025,6 @@ static const struct sectioncheck *section_mismatch(
* fromsec = .data*
* atsym = __param_ops_*
*
- * Pattern 2:
- * Many drivers utilise a *driver container with references to
- * add, remove, probe functions etc.
- * the pattern is identified by:
- * tosec = init or exit section
- * fromsec = data section
- * atsym = *driver, *_template, *_sht, *_ops, *_probe,
- * *probe_one, *_console, *_timer
- *
* Pattern 3:
* Whitelist all references from .head.text to any init section
*
@@ -1108,10 +1073,22 @@ static int secref_whitelist(const struct sectioncheck *mismatch,
strstarts(fromsym, "__param_ops_"))
return 0;
- /* Check for pattern 2 */
- if (match(tosec, init_exit_sections) &&
- match(fromsec, data_sections) &&
- match(fromsym, mismatch->symbol_white_list))
+ /* symbols in data sections that may refer to any init/exit sections */
+ if (match(fromsec, PATTERNS(DATA_SECTIONS)) &&
+ match(tosec, PATTERNS(ALL_INIT_SECTIONS, ALL_EXIT_SECTIONS)) &&
+ match(fromsym, PATTERNS("*_template", // scsi uses *_template a lot
+ "*_timer", // arm uses ops structures named _timer a lot
+ "*_sht", // scsi also used *_sht to some extent
+ "*_ops",
+ "*_probe",
+ "*_probe_one",
+ "*_console")))
+ return 0;
+
+ /* symbols in data sections that may refer to meminit/exit sections */
+ if (match(fromsec, PATTERNS(DATA_SECTIONS)) &&
+ match(tosec, PATTERNS(ALL_XXXINIT_SECTIONS, ALL_EXIT_SECTIONS)) &&
+ match(fromsym, PATTERNS("*driver")))
return 0;
/* Check for pattern 3 */
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/4] modpost: shorten warning messages in report_sec_mismatch()
2022-08-01 9:38 [PATCH 1/4] modpost: shorten warning messages in report_sec_mismatch() Masahiro Yamada
` (2 preceding siblings ...)
2022-08-01 9:39 ` [PATCH 4/4] modpost: remove .symbol_white_list field entirely Masahiro Yamada
@ 2022-08-02 18:13 ` Nick Desaulniers
2022-08-03 16:35 ` Masahiro Yamada
3 siblings, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2022-08-02 18:13 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-kbuild, Michal Marek, linux-kernel
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
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/4] modpost: shorten warning messages in report_sec_mismatch()
2022-08-02 18:13 ` [PATCH 1/4] modpost: shorten warning messages in report_sec_mismatch() Nick Desaulniers
@ 2022-08-03 16:35 ` Masahiro Yamada
0 siblings, 0 replies; 6+ messages in thread
From: Masahiro Yamada @ 2022-08-03 16:35 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Linux Kbuild mailing list, Michal Marek, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-03 16:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-01 9:38 [PATCH 1/4] modpost: shorten warning messages in report_sec_mismatch() Masahiro Yamada
2022-08-01 9:39 ` [PATCH 2/4] modpost: add PATTERNS() helper macro Masahiro Yamada
2022-08-01 9:39 ` [PATCH 3/4] modpost: remove unneeded .symbol_white_list initializers Masahiro Yamada
2022-08-01 9:39 ` [PATCH 4/4] modpost: remove .symbol_white_list field entirely Masahiro Yamada
2022-08-02 18:13 ` [PATCH 1/4] modpost: shorten warning messages in report_sec_mismatch() Nick Desaulniers
2022-08-03 16:35 ` Masahiro Yamada
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.