[v2,8/9] objtool: Detect missing __noreturn annotations

  Most "unreachable instruction" warnings these days seem to actually be
the result of a missing __noreturn annotation.  Add an explicit check
for that.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 tools/objtool/Documentation/objtool.txt |  6 ++++++
 tools/objtool/check.c                   | 14 +++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)
  

On Wed, Apr 12, 2023 at 12:03:23PM -0700, Josh Poimboeuf wrote:
> Most "unreachable instruction" warnings these days seem to actually be
> the result of a missing __noreturn annotation.  Add an explicit check
> for that.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
>  tools/objtool/Documentation/objtool.txt |  6 ++++++
>  tools/objtool/check.c                   | 14 +++++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt
> index 5a69c207a10e..2cd1fa16ed08 100644
> --- a/tools/objtool/Documentation/objtool.txt
> +++ b/tools/objtool/Documentation/objtool.txt
> @@ -303,6 +303,12 @@ the objtool maintainers.
>     If it's not actually in a callable function (e.g. kernel entry code),
>     change ENDPROC to END.
>  
> +3. file.o: warning: objtool: foo+0x48c: bar() is missing a __noreturn annotation
> +
> +   The call from foo() to bar() doesn't return, but bar() is missing the
> +   __noreturn annotation.  NOTE: In addition to adding the __noreturn
> +   annotation, the function name also needs to be added to
> +   'global_noreturns' in tools/objtool/check.c.

Do we want something like the below (except perhaps less horrible) ?

---
 tools/objtool/Makefile     |  1 +
 tools/objtool/check.c      | 27 +--------------------------
 tools/objtool/noreturns    | 26 ++++++++++++++++++++++++++
 tools/objtool/noreturns.sh |  7 +++++++
 4 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 83b100c1e7f6..50b6cd241571 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -63,6 +63,7 @@ include $(srctree)/tools/build/Makefile.include
 
 $(OBJTOOL_IN): fixdep $(LIBSUBCMD) FORCE
 	$(Q)$(CONFIG_SHELL) ./sync-check.sh
+	$(Q)$(CONFIG_SHELL) ./noreturns.sh
 	$(Q)$(MAKE) $(build)=objtool $(HOST_OVERRIDES) CFLAGS="$(OBJTOOL_CFLAGS)" \
 		LDFLAGS="$(OBJTOOL_LDFLAGS)"
 
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index f937be1afe65..f558730c27b6 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -197,32 +197,7 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
 	 * attribute isn't provided in ELF data. Keep 'em sorted.
 	 */
 	static const char * const global_noreturns[] = {
-		"__invalid_creds",
-		"__module_put_and_kthread_exit",
-		"__reiserfs_panic",
-		"__stack_chk_fail",
-		"__ubsan_handle_builtin_unreachable",
-		"cpu_bringup_and_idle",
-		"cpu_startup_entry",
-		"do_exit",
-		"do_group_exit",
-		"do_task_dead",
-		"ex_handler_msr_mce",
-		"fortify_panic",
-		"kthread_complete_and_exit",
-		"kthread_exit",
-		"kunit_try_catch_throw",
-		"lbug_with_loc",
-		"machine_real_restart",
-		"make_task_dead",
-		"panic",
-		"rewind_stack_and_make_dead",
-		"sev_es_terminate",
-		"snp_abort",
-		"stop_this_cpu",
-		"usercopy_abort",
-		"xen_cpu_bringup_again",
-		"xen_start_kernel",
+#include "noreturns.h"
 	};
 
 	if (!func)
diff --git a/tools/objtool/noreturns b/tools/objtool/noreturns
new file mode 100644
index 000000000000..75f35fbb34d1
--- /dev/null
+++ b/tools/objtool/noreturns
@@ -0,0 +1,26 @@
+__invalid_creds
+__module_put_and_kthread_exit
+__reiserfs_panic
+__stack_chk_fail
+__ubsan_handle_builtin_unreachable
+cpu_bringup_and_idle
+cpu_startup_entry
+do_exit
+do_group_exit
+do_task_dead
+ex_handler_msr_mce
+fortify_panic
+kthread_complete_and_exit
+kthread_exit
+kunit_try_catch_throw
+lbug_with_loc
+machine_real_restart
+make_task_dead
+panic
+rewind_stack_and_make_dead
+sev_es_terminate
+snp_abort
+stop_this_cpu
+usercopy_abort
+xen_cpu_bringup_again
+xen_start_kernel
diff --git a/tools/objtool/noreturns.sh b/tools/objtool/noreturns.sh
new file mode 100755
index 000000000000..f728cb61e665
--- /dev/null
+++ b/tools/objtool/noreturns.sh
@@ -0,0 +1,7 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+cat noreturns | while read func
+do
+	echo \"${func}\",
+done > noreturns.h
  

> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -4485,7 +4485,8 @@ static int validate_sls(struct objtool_file *file)
>  
>  static int validate_reachable_instructions(struct objtool_file *file)
>  {
> -	struct instruction *insn;
> +	struct instruction *insn, *prev_insn;
> +	struct symbol *call_dest;
>  	int warnings = 0;
>  
>  	if (file->ignore_unreachables)
> @@ -4495,6 +4496,17 @@ static int validate_reachable_instructions(struct objtool_file *file)
>  		if (insn->visited || ignore_unreachable_insn(file, insn))
>  			continue;
>  
> +		prev_insn = prev_insn_same_sec(file, insn);
> +		if (prev_insn && prev_insn->dead_end) {
> +			call_dest = insn_call_dest(prev_insn);
> +			if (call_dest) {
> +				WARN_INSN(insn, "%s() is missing a __noreturn annotation",
> +					  call_dest->name);
> +				warnings++;
> +				continue;

A nit but this and

> +			}
> +		}
> +
>  		WARN_INSN(insn, "unreachable instruction");
>  		warnings++;

this makes me thinking. Wouldn't it be confusing to anyone that there is 
no correspondence between warnings and a number of actual reported 
warnings through WARN_INSN()? In the future when there would be a usage 
for warnings. It does not really matter now.

Miroslav
  

On Thu, Apr 13, 2023 at 10:48:01AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 12, 2023 at 12:03:23PM -0700, Josh Poimboeuf wrote:
> > Most "unreachable instruction" warnings these days seem to actually be
> > the result of a missing __noreturn annotation.  Add an explicit check
> > for that.
> > 
> > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> > ---
> >  tools/objtool/Documentation/objtool.txt |  6 ++++++
> >  tools/objtool/check.c                   | 14 +++++++++++++-
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt
> > index 5a69c207a10e..2cd1fa16ed08 100644
> > --- a/tools/objtool/Documentation/objtool.txt
> > +++ b/tools/objtool/Documentation/objtool.txt
> > @@ -303,6 +303,12 @@ the objtool maintainers.
> >     If it's not actually in a callable function (e.g. kernel entry code),
> >     change ENDPROC to END.
> >  
> > +3. file.o: warning: objtool: foo+0x48c: bar() is missing a __noreturn annotation
> > +
> > +   The call from foo() to bar() doesn't return, but bar() is missing the
> > +   __noreturn annotation.  NOTE: In addition to adding the __noreturn
> > +   annotation, the function name also needs to be added to
> > +   'global_noreturns' in tools/objtool/check.c.
> 
> Do we want something like the below (except perhaps less horrible) ?

Yeah, maybe.  Another possible way to do it:

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index cae6ac6ff246..a4e8ff9dabf1 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -192,39 +192,16 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
 	struct instruction *insn;
 	bool empty = true;
 
+#define NORETURN(func) "\"" __stringify(func) "\"",
+
 	/*
 	 * Unfortunately these have to be hard coded because the noreturn
 	 * attribute isn't provided in ELF data. Keep 'em sorted.
 	 */
 	static const char * const global_noreturns[] = {
-		"__invalid_creds",
-		"__module_put_and_kthread_exit",
-		"__reiserfs_panic",
-		"__stack_chk_fail",
-		"__ubsan_handle_builtin_unreachable",
-		"arch_cpu_idle_dead",
-		"cpu_bringup_and_idle",
-		"cpu_startup_entry",
-		"do_exit",
-		"do_group_exit",
-		"do_task_dead",
-		"ex_handler_msr_mce",
-		"fortify_panic",
-		"kthread_complete_and_exit",
-		"kthread_exit",
-		"kunit_try_catch_throw",
-		"lbug_with_loc",
-		"machine_real_restart",
-		"make_task_dead",
-		"panic",
-		"rewind_stack_and_make_dead",
-		"sev_es_terminate",
-		"snp_abort",
-		"stop_this_cpu",
-		"usercopy_abort",
-		"xen_cpu_bringup_again",
-		"xen_start_kernel",
+#include "noreturns.h"
 	};
+#undef NORETURN
 
 	if (!func)
 		return false;
diff --git a/tools/objtool/noreturns.h b/tools/objtool/noreturns.h
new file mode 100644
index 000000000000..0f5e53fd9e7a
--- /dev/null
+++ b/tools/objtool/noreturns.h
@@ -0,0 +1,27 @@
+NORETURN(__invalid_creds)
+NORETURN(__module_put_and_kthread_exit)
+NORETURN(__reiserfs_panic)
+NORETURN(__stack_chk_fail)
+NORETURN(__ubsan_handle_builtin_unreachable)
+NORETURN(arch_cpu_idle_dead)
+NORETURN(cpu_bringup_and_idle)
+NORETURN(cpu_startup_entry)
+NORETURN(do_exit)
+NORETURN(do_group_exit)
+NORETURN(do_task_dead)
+NORETURN(ex_handler_msr_mce)
+NORETURN(fortify_panic)
+NORETURN(kthread_complete_and_exit)
+NORETURN(kthread_exit)
+NORETURN(kunit_try_catch_throw)
+NORETURN(lbug_with_loc)
+NORETURN(machine_real_restart)
+NORETURN(make_task_dead)
+NORETURN(panic)
+NORETURN(rewind_stack_and_make_dead)
+NORETURN(sev_es_terminate)
+NORETURN(snp_abort)
+NORETURN(stop_this_cpu)
+NORETURN(usercopy_abort)
+NORETURN(xen_cpu_bringup_again)
+NORETURN(xen_start_kernel)
  

On Thu, Apr 13, 2023 at 04:19:10PM +0200, Miroslav Benes wrote:
> > --- a/tools/objtool/check.c
> > +++ b/tools/objtool/check.c
> > @@ -4485,7 +4485,8 @@ static int validate_sls(struct objtool_file *file)
> >  
> >  static int validate_reachable_instructions(struct objtool_file *file)
> >  {
> > -	struct instruction *insn;
> > +	struct instruction *insn, *prev_insn;
> > +	struct symbol *call_dest;
> >  	int warnings = 0;
> >  
> >  	if (file->ignore_unreachables)
> > @@ -4495,6 +4496,17 @@ static int validate_reachable_instructions(struct objtool_file *file)
> >  		if (insn->visited || ignore_unreachable_insn(file, insn))
> >  			continue;
> >  
> > +		prev_insn = prev_insn_same_sec(file, insn);
> > +		if (prev_insn && prev_insn->dead_end) {
> > +			call_dest = insn_call_dest(prev_insn);
> > +			if (call_dest) {
> > +				WARN_INSN(insn, "%s() is missing a __noreturn annotation",
> > +					  call_dest->name);
> > +				warnings++;
> > +				continue;
> 
> A nit but this and
> 
> > +			}
> > +		}
> > +
> >  		WARN_INSN(insn, "unreachable instruction");
> >  		warnings++;
> 
> this makes me thinking. Wouldn't it be confusing to anyone that there is 
> no correspondence between warnings and a number of actual reported 
> warnings through WARN_INSN()? In the future when there would be a usage 
> for warnings. It does not really matter now.

True, maybe we need WARN_INSN_ONCE_PER_FUNC() or so ;-)
  

> diff --git a/tools/objtool/noreturns.h b/tools/objtool/noreturns.h
> new file mode 100644
> index 000000000000..0f5e53fd9e7a
> --- /dev/null
> +++ b/tools/objtool/noreturns.h
> @@ -0,0 +1,27 @@
> +NORETURN(__invalid_creds)
> +NORETURN(__module_put_and_kthread_exit)
> +NORETURN(__reiserfs_panic)
> +NORETURN(__stack_chk_fail)
> +NORETURN(__ubsan_handle_builtin_unreachable)
> +NORETURN(arch_cpu_idle_dead)
> +NORETURN(cpu_bringup_and_idle)
> +NORETURN(cpu_startup_entry)
> +NORETURN(do_exit)
> +NORETURN(do_group_exit)
> +NORETURN(do_task_dead)
> +NORETURN(ex_handler_msr_mce)
> +NORETURN(fortify_panic)
> +NORETURN(kthread_complete_and_exit)
> +NORETURN(kthread_exit)
> +NORETURN(kunit_try_catch_throw)
> +NORETURN(lbug_with_loc)
> +NORETURN(machine_real_restart)
> +NORETURN(make_task_dead)
> +NORETURN(panic)
> +NORETURN(rewind_stack_and_make_dead)
> +NORETURN(sev_es_terminate)
> +NORETURN(snp_abort)
> +NORETURN(stop_this_cpu)
> +NORETURN(usercopy_abort)
> +NORETURN(xen_cpu_bringup_again)
> +NORETURN(xen_start_kernel)

Not as convenient to edit, but much easier to use. A bit of a toss up I
suppose.