* [PATCH 2/3] modpost: remove unreachable code after fatal()
2023-11-25 10:31 [PATCH 1/3] modpost: move __attribute__((format(printf, 2, 3))) to modpost.h Masahiro Yamada
@ 2023-11-25 10:31 ` Masahiro Yamada
2023-11-30 0:29 ` Nathan Chancellor
2023-11-25 10:31 ` [PATCH 3/3] modpost: move exit(1) for fatal() to modpost.h Masahiro Yamada
2023-11-30 0:17 ` [PATCH 1/3] modpost: move __attribute__((format(printf, 2, 3))) " Nathan Chancellor
2 siblings, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2023-11-25 10:31 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nicolas Schier, Masahiro Yamada
fatal() never returns. Remove unreachable code.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index ca0a90158f85..013fc5031bc7 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -473,11 +473,9 @@ static int parse_elf(struct elf_info *info, const char *filename)
fatal("%s: not relocatable object.", filename);
/* Check if file offset is correct */
- if (hdr->e_shoff > info->size) {
+ if (hdr->e_shoff > info->size)
fatal("section header offset=%lu in file '%s' is bigger than filesize=%zu\n",
(unsigned long)hdr->e_shoff, filename, info->size);
- return 0;
- }
if (hdr->e_shnum == SHN_UNDEF) {
/*
@@ -515,12 +513,11 @@ static int parse_elf(struct elf_info *info, const char *filename)
const char *secname;
int nobits = sechdrs[i].sh_type == SHT_NOBITS;
- if (!nobits && sechdrs[i].sh_offset > info->size) {
+ if (!nobits && sechdrs[i].sh_offset > info->size)
fatal("%s is truncated. sechdrs[i].sh_offset=%lu > sizeof(*hrd)=%zu\n",
filename, (unsigned long)sechdrs[i].sh_offset,
sizeof(*hdr));
- return 0;
- }
+
secname = secstrings + sechdrs[i].sh_name;
if (strcmp(secname, ".modinfo") == 0) {
if (nobits)
--
2.40.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/3] modpost: remove unreachable code after fatal()
2023-11-25 10:31 ` [PATCH 2/3] modpost: remove unreachable code after fatal() Masahiro Yamada
@ 2023-11-30 0:29 ` Nathan Chancellor
0 siblings, 0 replies; 7+ messages in thread
From: Nathan Chancellor @ 2023-11-30 0:29 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-kbuild, linux-kernel, Nicolas Schier
On Sat, Nov 25, 2023 at 07:31:15PM +0900, Masahiro Yamada wrote:
> fatal() never returns. Remove unreachable code.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> ---
>
> scripts/mod/modpost.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index ca0a90158f85..013fc5031bc7 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -473,11 +473,9 @@ static int parse_elf(struct elf_info *info, const char *filename)
> fatal("%s: not relocatable object.", filename);
>
> /* Check if file offset is correct */
> - if (hdr->e_shoff > info->size) {
> + if (hdr->e_shoff > info->size)
> fatal("section header offset=%lu in file '%s' is bigger than filesize=%zu\n",
> (unsigned long)hdr->e_shoff, filename, info->size);
> - return 0;
> - }
>
> if (hdr->e_shnum == SHN_UNDEF) {
> /*
> @@ -515,12 +513,11 @@ static int parse_elf(struct elf_info *info, const char *filename)
> const char *secname;
> int nobits = sechdrs[i].sh_type == SHT_NOBITS;
>
> - if (!nobits && sechdrs[i].sh_offset > info->size) {
> + if (!nobits && sechdrs[i].sh_offset > info->size)
> fatal("%s is truncated. sechdrs[i].sh_offset=%lu > sizeof(*hrd)=%zu\n",
> filename, (unsigned long)sechdrs[i].sh_offset,
> sizeof(*hdr));
> - return 0;
> - }
> +
> secname = secstrings + sechdrs[i].sh_name;
> if (strcmp(secname, ".modinfo") == 0) {
> if (nobits)
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] modpost: move exit(1) for fatal() to modpost.h
2023-11-25 10:31 [PATCH 1/3] modpost: move __attribute__((format(printf, 2, 3))) to modpost.h Masahiro Yamada
2023-11-25 10:31 ` [PATCH 2/3] modpost: remove unreachable code after fatal() Masahiro Yamada
@ 2023-11-25 10:31 ` Masahiro Yamada
2023-11-30 0:32 ` Nathan Chancellor
2023-11-30 0:17 ` [PATCH 1/3] modpost: move __attribute__((format(printf, 2, 3))) " Nathan Chancellor
2 siblings, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2023-11-25 10:31 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nicolas Schier, Masahiro Yamada
fatal() never returns, but compilers are not aware of this fact because
exit(1) is called within the modpost_log() definition.
Move exit(1) to the fatal() macro so that compilers can identify
unreachable code flows.
Remove the initializer for 'taddr' in section_rel(), as compilers now
recognize this is not an uninitialized bug.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 4 +---
scripts/mod/modpost.h | 2 +-
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 013fc5031bc7..696c583a14ec 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -84,8 +84,6 @@ void modpost_log(enum loglevel loglevel, const char *fmt, ...)
vfprintf(stderr, fmt, arglist);
va_end(arglist);
- if (loglevel == LOG_FATAL)
- exit(1);
if (loglevel == LOG_ERROR)
error_occurred = true;
}
@@ -1415,7 +1413,7 @@ static void section_rel(struct module *mod, struct elf_info *elf,
for (rel = start; rel < stop; rel++) {
Elf_Sym *tsym;
- Elf_Addr taddr = 0, r_offset;
+ Elf_Addr taddr, r_offset;
unsigned int r_type, r_sym;
void *loc;
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 9fe974dc1a52..aaa67b24775e 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -215,4 +215,4 @@ modpost_log(enum loglevel loglevel, const char *fmt, ...);
*/
#define warn(fmt, args...) modpost_log(LOG_WARN, fmt, ##args)
#define error(fmt, args...) modpost_log(LOG_ERROR, fmt, ##args)
-#define fatal(fmt, args...) modpost_log(LOG_FATAL, fmt, ##args)
+#define fatal(fmt, args...) do { modpost_log(LOG_FATAL, fmt, ##args); exit(1); } while (1)
--
2.40.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 3/3] modpost: move exit(1) for fatal() to modpost.h
2023-11-25 10:31 ` [PATCH 3/3] modpost: move exit(1) for fatal() to modpost.h Masahiro Yamada
@ 2023-11-30 0:32 ` Nathan Chancellor
2023-12-03 9:03 ` Masahiro Yamada
0 siblings, 1 reply; 7+ messages in thread
From: Nathan Chancellor @ 2023-11-30 0:32 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-kbuild, linux-kernel, Nicolas Schier
On Sat, Nov 25, 2023 at 07:31:16PM +0900, Masahiro Yamada wrote:
> fatal() never returns, but compilers are not aware of this fact because
> exit(1) is called within the modpost_log() definition.
>
> Move exit(1) to the fatal() macro so that compilers can identify
> unreachable code flows.
>
> Remove the initializer for 'taddr' in section_rel(), as compilers now
> recognize this is not an uninitialized bug.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
While I did not see any warnings when testing, it seems like this patch
should come before patch 2 to avoid any potential problems from dropping
the unreachable code (since the compiler won't be able to infer that
fatal() is __noreturn without this change); doesn't really matter though
so:
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> ---
>
> scripts/mod/modpost.c | 4 +---
> scripts/mod/modpost.h | 2 +-
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 013fc5031bc7..696c583a14ec 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -84,8 +84,6 @@ void modpost_log(enum loglevel loglevel, const char *fmt, ...)
> vfprintf(stderr, fmt, arglist);
> va_end(arglist);
>
> - if (loglevel == LOG_FATAL)
> - exit(1);
> if (loglevel == LOG_ERROR)
> error_occurred = true;
> }
> @@ -1415,7 +1413,7 @@ static void section_rel(struct module *mod, struct elf_info *elf,
>
> for (rel = start; rel < stop; rel++) {
> Elf_Sym *tsym;
> - Elf_Addr taddr = 0, r_offset;
> + Elf_Addr taddr, r_offset;
> unsigned int r_type, r_sym;
> void *loc;
>
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index 9fe974dc1a52..aaa67b24775e 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -215,4 +215,4 @@ modpost_log(enum loglevel loglevel, const char *fmt, ...);
> */
> #define warn(fmt, args...) modpost_log(LOG_WARN, fmt, ##args)
> #define error(fmt, args...) modpost_log(LOG_ERROR, fmt, ##args)
> -#define fatal(fmt, args...) modpost_log(LOG_FATAL, fmt, ##args)
> +#define fatal(fmt, args...) do { modpost_log(LOG_FATAL, fmt, ##args); exit(1); } while (1)
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 3/3] modpost: move exit(1) for fatal() to modpost.h
2023-11-30 0:32 ` Nathan Chancellor
@ 2023-12-03 9:03 ` Masahiro Yamada
0 siblings, 0 replies; 7+ messages in thread
From: Masahiro Yamada @ 2023-12-03 9:03 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: linux-kbuild, linux-kernel, Nicolas Schier
On Thu, Nov 30, 2023 at 9:32 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Sat, Nov 25, 2023 at 07:31:16PM +0900, Masahiro Yamada wrote:
> > fatal() never returns, but compilers are not aware of this fact because
> > exit(1) is called within the modpost_log() definition.
> >
> > Move exit(1) to the fatal() macro so that compilers can identify
> > unreachable code flows.
> >
> > Remove the initializer for 'taddr' in section_rel(), as compilers now
> > recognize this is not an uninitialized bug.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> While I did not see any warnings when testing, it seems like this patch
> should come before patch 2 to avoid any potential problems from dropping
> the unreachable code (since the compiler won't be able to infer that
> fatal() is __noreturn without this change); doesn't really matter though
> so:
My eyes spotted 2/3, but I can swap the patch order
and rephase the patch description because clang can
detect the unreachable code.
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
>
> > ---
> >
> > scripts/mod/modpost.c | 4 +---
> > scripts/mod/modpost.h | 2 +-
> > 2 files changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 013fc5031bc7..696c583a14ec 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -84,8 +84,6 @@ void modpost_log(enum loglevel loglevel, const char *fmt, ...)
> > vfprintf(stderr, fmt, arglist);
> > va_end(arglist);
> >
> > - if (loglevel == LOG_FATAL)
> > - exit(1);
> > if (loglevel == LOG_ERROR)
> > error_occurred = true;
> > }
> > @@ -1415,7 +1413,7 @@ static void section_rel(struct module *mod, struct elf_info *elf,
> >
> > for (rel = start; rel < stop; rel++) {
> > Elf_Sym *tsym;
> > - Elf_Addr taddr = 0, r_offset;
> > + Elf_Addr taddr, r_offset;
> > unsigned int r_type, r_sym;
> > void *loc;
> >
> > diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> > index 9fe974dc1a52..aaa67b24775e 100644
> > --- a/scripts/mod/modpost.h
> > +++ b/scripts/mod/modpost.h
> > @@ -215,4 +215,4 @@ modpost_log(enum loglevel loglevel, const char *fmt, ...);
> > */
> > #define warn(fmt, args...) modpost_log(LOG_WARN, fmt, ##args)
> > #define error(fmt, args...) modpost_log(LOG_ERROR, fmt, ##args)
> > -#define fatal(fmt, args...) modpost_log(LOG_FATAL, fmt, ##args)
> > +#define fatal(fmt, args...) do { modpost_log(LOG_FATAL, fmt, ##args); exit(1); } while (1)
> > --
> > 2.40.1
> >
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] modpost: move __attribute__((format(printf, 2, 3))) to modpost.h
2023-11-25 10:31 [PATCH 1/3] modpost: move __attribute__((format(printf, 2, 3))) to modpost.h Masahiro Yamada
2023-11-25 10:31 ` [PATCH 2/3] modpost: remove unreachable code after fatal() Masahiro Yamada
2023-11-25 10:31 ` [PATCH 3/3] modpost: move exit(1) for fatal() to modpost.h Masahiro Yamada
@ 2023-11-30 0:17 ` Nathan Chancellor
2 siblings, 0 replies; 7+ messages in thread
From: Nathan Chancellor @ 2023-11-30 0:17 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: linux-kbuild, linux-kernel, Nicolas Schier
On Sat, Nov 25, 2023 at 07:31:14PM +0900, Masahiro Yamada wrote:
> This attribute must be added to the function declaration in a header
> for comprehensive checking of all the callsites.
>
> Fixes: 6d9a89ea4b06 ("kbuild: declare the modpost error functions as printf like")
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> ---
>
> scripts/mod/modpost.c | 3 +--
> scripts/mod/modpost.h | 3 ++-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index cb6406f485a9..ca0a90158f85 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -60,8 +60,7 @@ static unsigned int nr_unresolved;
>
> #define MODULE_NAME_LEN (64 - sizeof(Elf_Addr))
>
> -void __attribute__((format(printf, 2, 3)))
> -modpost_log(enum loglevel loglevel, const char *fmt, ...)
> +void modpost_log(enum loglevel loglevel, const char *fmt, ...)
> {
> va_list arglist;
>
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index 69baf014da4f..9fe974dc1a52 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -197,7 +197,8 @@ enum loglevel {
> LOG_FATAL
> };
>
> -void modpost_log(enum loglevel loglevel, const char *fmt, ...);
> +void __attribute__((format(printf, 2, 3)))
> +modpost_log(enum loglevel loglevel, const char *fmt, ...);
>
> /*
> * warn - show the given message, then let modpost continue running, still
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread