All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] objtool: warning improvements
@ 2023-03-27 16:00 Josh Poimboeuf
  2023-03-27 16:00 ` [PATCH 1/5] objtool: Add '--verbose' option for disassembling affected functions Josh Poimboeuf
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Josh Poimboeuf @ 2023-03-27 16:00 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel, Peter Zijlstra

Add some user-friendliness to the warnings:

- Add OBJTOOL_ARGS="--verbose" option for making it easier to debug
  objtool warnings over email

- Remove per-file rate limiting (doesn't make sense for vmlinux.o)

- Add "missing __noreturn" warning

Josh Poimboeuf (5):
  objtool: Add '--verbose' option for disassembling affected functions
  objtool: Combine '--backtrace' with '--verbose'
  objtool: Remove superfluous dead_end_function() check
  objtool: Add per-function rate limiting for unreachable warnings
  objtool: Add "missing __noreturn" warning

 tools/objtool/Documentation/objtool.txt | 12 ++++++
 tools/objtool/builtin-check.c           |  2 +-
 tools/objtool/check.c                   | 45 ++++++++++++---------
 tools/objtool/include/objtool/builtin.h |  2 +-
 tools/objtool/include/objtool/elf.h     |  1 +
 tools/objtool/include/objtool/warn.h    | 52 ++++++++++++++++++++++---
 6 files changed, 88 insertions(+), 26 deletions(-)

-- 
2.39.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/5] objtool: Add '--verbose' option for disassembling affected functions
  2023-03-27 16:00 [PATCH 0/5] objtool: warning improvements Josh Poimboeuf
@ 2023-03-27 16:00 ` Josh Poimboeuf
  2023-03-28  8:47   ` Miroslav Benes
  2023-03-27 16:00 ` [PATCH 2/5] objtool: Combine '--backtrace' with '--verbose' Josh Poimboeuf
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Josh Poimboeuf @ 2023-03-27 16:00 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel, Peter Zijlstra

When a warning is associated with a function, add an option to
disassemble that function.

This makes it easier for reporters to submit the information needed to
diagnose objtool warnings.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 tools/objtool/Documentation/objtool.txt |  5 ++++
 tools/objtool/builtin-check.c           |  1 +
 tools/objtool/include/objtool/builtin.h |  1 +
 tools/objtool/include/objtool/warn.h    | 38 +++++++++++++++++++++++++
 4 files changed, 45 insertions(+)

diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt
index 8e53fc6735ef..7c1a46af322f 100644
--- a/tools/objtool/Documentation/objtool.txt
+++ b/tools/objtool/Documentation/objtool.txt
@@ -244,6 +244,11 @@ To achieve the validation, objtool enforces the following rules:
 Objtool warnings
 ----------------
 
+NOTE: When requesting help with an objtool warning, please re-run the
+kernel build with `OBJTOOL_ARGS="--verbose" make <whatever>` and send
+the full warning output (including any function disassembly below the
+warning) to the objtool maintainers.
+
 For asm files, if you're getting an error which doesn't make sense,
 first make sure that the affected code follows the above rules.
 
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 7c175198d09f..b8de42f6778e 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -93,6 +93,7 @@ static const struct option check_options[] = {
 	OPT_BOOLEAN(0, "no-unreachable", &opts.no_unreachable, "skip 'unreachable instruction' warnings"),
 	OPT_BOOLEAN(0, "sec-address", &opts.sec_address, "print section addresses in warnings"),
 	OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
+	OPT_BOOLEAN('v', "verbose", &opts.verbose, "verbose warnings"),
 
 	OPT_END(),
 };
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index 2a108e648b7a..fcca6662c8b4 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -37,6 +37,7 @@ struct opts {
 	bool no_unreachable;
 	bool sec_address;
 	bool stats;
+	bool verbose;
 };
 
 extern struct opts opts;
diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index a3e79ae75f2e..b85aa440ee1f 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -11,6 +11,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
+#include <unistd.h>
 #include <objtool/builtin.h>
 #include <objtool/elf.h>
 
@@ -41,6 +42,41 @@ static inline char *offstr(struct section *sec, unsigned long offset)
 	return str;
 }
 
+static inline void objdump_func(struct section *sec, unsigned long offset)
+{
+	struct symbol *sym = find_func_containing(sec, offset);
+	const char *script_relative = "scripts/objdump-func";
+	bool is_text = (sec->sh.sh_flags & SHF_EXECINSTR);
+	char *cmd, *srctree, *script;
+
+	if (is_text)
+		sym = find_func_containing(sec, offset);
+	if (!sym)
+		sym = find_symbol_containing(sec, offset);
+	if (!sym)
+		return;
+
+	srctree = getenv("abs_srctree");
+	if (!srctree)
+		return;
+
+	script = malloc(strlen(srctree) + strlen(script_relative) + 2);
+	if (!script)
+		return;
+
+	sprintf(script, "%s/%s", srctree, script_relative);
+
+	if (access(script, X_OK))
+		return;
+
+	cmd = malloc(strlen(script) + strlen(objname) + strlen(sym->name) + 10);
+	if (!cmd)
+		return;
+
+	sprintf(cmd, "%s %s %s 1>&2", script, objname, sym->name);
+	system(cmd);
+}
+
 #define WARN(format, ...)				\
 	fprintf(stderr,					\
 		"%s: warning: objtool: " format "\n",	\
@@ -51,6 +87,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
 	char *_str = offstr(sec, offset);		\
 	WARN("%s: " format, _str, ##__VA_ARGS__);	\
 	free(_str);					\
+	if (opts.verbose)				\
+		objdump_func(sec, offset);		\
 })
 
 #define BT_FUNC(format, insn, ...)			\
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/5] objtool: Combine '--backtrace' with '--verbose'
  2023-03-27 16:00 [PATCH 0/5] objtool: warning improvements Josh Poimboeuf
  2023-03-27 16:00 ` [PATCH 1/5] objtool: Add '--verbose' option for disassembling affected functions Josh Poimboeuf
@ 2023-03-27 16:00 ` Josh Poimboeuf
  2023-03-28  8:07   ` Peter Zijlstra
  2023-03-27 16:00 ` [PATCH 3/5] objtool: Remove superfluous dead_end_function() check Josh Poimboeuf
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Josh Poimboeuf @ 2023-03-27 16:00 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel, Peter Zijlstra

Get rid of the '--backtrace' option, instead including that
functionality in '--verbose'.  This makes it easy to gather all the
information needed for diagnosing objtool warnings.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 tools/objtool/Documentation/objtool.txt |  4 ++--
 tools/objtool/builtin-check.c           |  1 -
 tools/objtool/check.c                   | 22 ++++++++--------------
 tools/objtool/include/objtool/builtin.h |  1 -
 tools/objtool/include/objtool/warn.h    | 14 ++++++++------
 5 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt
index 7c1a46af322f..ec6f82fb414c 100644
--- a/tools/objtool/Documentation/objtool.txt
+++ b/tools/objtool/Documentation/objtool.txt
@@ -246,8 +246,8 @@ Objtool warnings
 
 NOTE: When requesting help with an objtool warning, please re-run the
 kernel build with `OBJTOOL_ARGS="--verbose" make <whatever>` and send
-the full warning output (including any function disassembly below the
-warning) to the objtool maintainers.
+the full warning output (including any function disassembly or objtool
+backtrace below the warning) to the objtool maintainers.
 
 For asm files, if you're getting an error which doesn't make sense,
 first make sure that the affected code follows the above rules.
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index b8de42f6778e..937ba5d78e08 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -84,7 +84,6 @@ static const struct option check_options[] = {
 	OPT_CALLBACK_OPTARG(0, "dump", NULL, NULL, "orc", "dump metadata", parse_dump),
 
 	OPT_GROUP("Options:"),
-	OPT_BOOLEAN(0, "backtrace", &opts.backtrace, "unwind on error"),
 	OPT_BOOLEAN(0, "backup", &opts.backup, "create .orig files before modification"),
 	OPT_BOOLEAN(0, "dry-run", &opts.dryrun, "don't write modifications"),
 	OPT_BOOLEAN(0, "link", &opts.link, "object is a linked object"),
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index cae6ac6ff246..a652b9e5c805 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3708,8 +3708,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 
 				ret = validate_branch(file, func, alt->insn, state);
 				if (ret) {
-					if (opts.backtrace)
-						BT_FUNC("(alt)", insn);
+					BT_FUNC("(alt)", insn);
 					return ret;
 				}
 			}
@@ -3755,8 +3754,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 				ret = validate_branch(file, func,
 						      insn->jump_dest, state);
 				if (ret) {
-					if (opts.backtrace)
-						BT_FUNC("(branch)", insn);
+					BT_FUNC("(branch)", insn);
 					return ret;
 				}
 			}
@@ -3855,7 +3853,7 @@ static int validate_unwind_hint(struct objtool_file *file,
 {
 	if (insn->hint && !insn->visited && !insn->ignore) {
 		int ret = validate_branch(file, insn_func(insn), insn, *state);
-		if (ret && opts.backtrace)
+		if (ret)
 			BT_FUNC("<=== (hint)", insn);
 		return ret;
 	}
@@ -3914,8 +3912,7 @@ static int validate_unret(struct objtool_file *file, struct instruction *insn)
 
 				ret = validate_unret(file, alt->insn);
 				if (ret) {
-				        if (opts.backtrace)
-						BT_FUNC("(alt)", insn);
+					BT_FUNC("(alt)", insn);
 					return ret;
 				}
 			}
@@ -3942,10 +3939,8 @@ static int validate_unret(struct objtool_file *file, struct instruction *insn)
 				}
 				ret = validate_unret(file, insn->jump_dest);
 				if (ret) {
-					if (opts.backtrace) {
-						BT_FUNC("(branch%s)", insn,
-							insn->type == INSN_JUMP_CONDITIONAL ? "-cond" : "");
-					}
+					BT_FUNC("(branch%s)", insn,
+						insn->type == INSN_JUMP_CONDITIONAL ? "-cond" : "");
 					return ret;
 				}
 
@@ -3967,8 +3962,7 @@ static int validate_unret(struct objtool_file *file, struct instruction *insn)
 
 			ret = validate_unret(file, dest);
 			if (ret) {
-				if (opts.backtrace)
-					BT_FUNC("(call)", insn);
+				BT_FUNC("(call)", insn);
 				return ret;
 			}
 			/*
@@ -4254,7 +4248,7 @@ static int validate_symbol(struct objtool_file *file, struct section *sec,
 	state->uaccess = sym->uaccess_safe;
 
 	ret = validate_branch(file, insn_func(insn), insn, *state);
-	if (ret && opts.backtrace)
+	if (ret)
 		BT_FUNC("<=== (sym)", insn);
 	return ret;
 }
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index fcca6662c8b4..38aef760465e 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -28,7 +28,6 @@ struct opts {
 	bool cfi;
 
 	/* options: */
-	bool backtrace;
 	bool backup;
 	bool dryrun;
 	bool link;
diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index b85aa440ee1f..a9ec1ed6a2e3 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -91,12 +91,14 @@ static inline void objdump_func(struct section *sec, unsigned long offset)
 		objdump_func(sec, offset);		\
 })
 
-#define BT_FUNC(format, insn, ...)			\
-({							\
-	struct instruction *_insn = (insn);		\
-	char *_str = offstr(_insn->sec, _insn->offset); \
-	WARN("  %s: " format, _str, ##__VA_ARGS__);	\
-	free(_str);					\
+#define BT_FUNC(format, insn, ...)				\
+({								\
+	if (opts.verbose) {					\
+		struct instruction *_insn = (insn);		\
+		char *_str = offstr(_insn->sec, _insn->offset); \
+		WARN("  %s: " format, _str, ##__VA_ARGS__);	\
+		free(_str);					\
+	}							\
 })
 
 #define WARN_ELF(format, ...)				\
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/5] objtool: Remove superfluous dead_end_function() check
  2023-03-27 16:00 [PATCH 0/5] objtool: warning improvements Josh Poimboeuf
  2023-03-27 16:00 ` [PATCH 1/5] objtool: Add '--verbose' option for disassembling affected functions Josh Poimboeuf
  2023-03-27 16:00 ` [PATCH 2/5] objtool: Combine '--backtrace' with '--verbose' Josh Poimboeuf
@ 2023-03-27 16:00 ` Josh Poimboeuf
  2023-03-27 16:00 ` [PATCH 4/5] objtool: Add per-function rate limiting for unreachable warnings Josh Poimboeuf
  2023-03-27 16:00 ` [PATCH 5/5] objtool: Add "missing __noreturn" warning Josh Poimboeuf
  4 siblings, 0 replies; 16+ messages in thread
From: Josh Poimboeuf @ 2023-03-27 16:00 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel, Peter Zijlstra

annotate_call_site() already sets 'insn->dead_end' for calls to dead end
functions.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 tools/objtool/check.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index a652b9e5c805..73dd091c0075 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4140,8 +4140,7 @@ static bool ignore_unreachable_insn(struct objtool_file *file, struct instructio
 	 * It may also insert a UD2 after calling a __noreturn function.
 	 */
 	prev_insn = prev_insn_same_sec(file, insn);
-	if ((prev_insn->dead_end ||
-	     dead_end_function(file, insn_call_dest(prev_insn))) &&
+	if (prev_insn->dead_end &&
 	    (insn->type == INSN_BUG ||
 	     (insn->type == INSN_JUMP_UNCONDITIONAL &&
 	      insn->jump_dest && insn->jump_dest->type == INSN_BUG)))
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/5] objtool: Add per-function rate limiting for unreachable warnings
  2023-03-27 16:00 [PATCH 0/5] objtool: warning improvements Josh Poimboeuf
                   ` (2 preceding siblings ...)
  2023-03-27 16:00 ` [PATCH 3/5] objtool: Remove superfluous dead_end_function() check Josh Poimboeuf
@ 2023-03-27 16:00 ` Josh Poimboeuf
  2023-03-28  8:11   ` Peter Zijlstra
  2023-03-28  9:07   ` Miroslav Benes
  2023-03-27 16:00 ` [PATCH 5/5] objtool: Add "missing __noreturn" warning Josh Poimboeuf
  4 siblings, 2 replies; 16+ messages in thread
From: Josh Poimboeuf @ 2023-03-27 16:00 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel, Peter Zijlstra

Unreachable instruction warnings are rate limited to once per object
file.  That no longer makes sense for vmlinux validation, which might
have other unreachable instructions lurking in other places.  Change it
to once per function.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 tools/objtool/check.c               | 4 ++++
 tools/objtool/include/objtool/elf.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 73dd091c0075..67a684225702 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4557,6 +4557,10 @@ static int validate_reachable_instructions(struct objtool_file *file)
 		if (insn->visited || ignore_unreachable_insn(file, insn))
 			continue;
 
+		if (insn->sym->warned)
+			continue;
+		insn->sym->warned = 1;
+
 		WARN_FUNC("unreachable instruction", insn->sec, insn->offset);
 		return 1;
 	}
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index ad0024da262b..a668173a5869 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -61,6 +61,7 @@ struct symbol {
 	u8 return_thunk      : 1;
 	u8 fentry            : 1;
 	u8 profiling_func    : 1;
+	u8 warned	     : 1;
 	struct list_head pv_target;
 	struct list_head reloc_list;
 };
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/5] objtool: Add "missing __noreturn" warning
  2023-03-27 16:00 [PATCH 0/5] objtool: warning improvements Josh Poimboeuf
                   ` (3 preceding siblings ...)
  2023-03-27 16:00 ` [PATCH 4/5] objtool: Add per-function rate limiting for unreachable warnings Josh Poimboeuf
@ 2023-03-27 16:00 ` Josh Poimboeuf
  2023-03-28  9:10   ` Miroslav Benes
  4 siblings, 1 reply; 16+ messages in thread
From: Josh Poimboeuf @ 2023-03-27 16:00 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel, Peter Zijlstra

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 |  7 +++++++
 tools/objtool/check.c                   | 16 ++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt
index ec6f82fb414c..e04d16490c3b 100644
--- a/tools/objtool/Documentation/objtool.txt
+++ b/tools/objtool/Documentation/objtool.txt
@@ -423,6 +423,13 @@ the objtool maintainers.
    names and does not use module_init() / module_exit() macros to create
    them.
 
+13. file.o: warning: func(): missing __noreturn
+
+   Objtool has detected that the function doesn't return, but 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.
+
 
 If the error doesn't seem to make sense, it could be a bug in objtool.
 Feel free to ask the objtool maintainer for help.
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 67a684225702..1ed3024af2b1 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4548,7 +4548,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;
 
 	if (file->ignore_unreachables)
 		return 0;
@@ -4561,8 +4562,19 @@ static int validate_reachable_instructions(struct objtool_file *file)
 			continue;
 		insn->sym->warned = 1;
 
+		prev_insn = prev_insn_same_sec(file, insn);
+		if (prev_insn)
+			call_dest = insn_call_dest(prev_insn);
+		if (prev_insn && prev_insn->dead_end && call_dest) {
+			if (call_dest->warned)
+				continue;
+			call_dest->warned = 1;
+
+			WARN("%s(): missing __noreturn", call_dest->name);
+			continue;
+		}
+
 		WARN_FUNC("unreachable instruction", insn->sec, insn->offset);
-		return 1;
 	}
 
 	return 0;
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] objtool: Combine '--backtrace' with '--verbose'
  2023-03-27 16:00 ` [PATCH 2/5] objtool: Combine '--backtrace' with '--verbose' Josh Poimboeuf
@ 2023-03-28  8:07   ` Peter Zijlstra
  2023-03-28 20:19     ` Josh Poimboeuf
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Zijlstra @ 2023-03-28  8:07 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: x86, linux-kernel

On Mon, Mar 27, 2023 at 09:00:45AM -0700, Josh Poimboeuf wrote:
> Get rid of the '--backtrace' option, instead including that
> functionality in '--verbose'.  This makes it easy to gather all the
> information needed for diagnosing objtool warnings.

Hurmm.. can't we have verbose imply backtrace but keep the separate
option? I'm not sure if I always want the objdump thing -- esp with
multiple warnings on vmlinux that's going to be really slow -- better to
dump the whole of vmlinux.o once at the end.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] objtool: Add per-function rate limiting for unreachable warnings
  2023-03-27 16:00 ` [PATCH 4/5] objtool: Add per-function rate limiting for unreachable warnings Josh Poimboeuf
@ 2023-03-28  8:11   ` Peter Zijlstra
  2023-03-28 20:22     ` Josh Poimboeuf
  2023-03-28  9:07   ` Miroslav Benes
  1 sibling, 1 reply; 16+ messages in thread
From: Peter Zijlstra @ 2023-03-28  8:11 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: x86, linux-kernel

On Mon, Mar 27, 2023 at 09:00:47AM -0700, Josh Poimboeuf wrote:
> Unreachable instruction warnings are rate limited to once per object
> file.  That no longer makes sense for vmlinux validation, which might
> have other unreachable instructions lurking in other places.  Change it
> to once per function.

Do we want a negative option to disable this? --no-ratelimit or such?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] objtool: Add '--verbose' option for disassembling affected functions
  2023-03-27 16:00 ` [PATCH 1/5] objtool: Add '--verbose' option for disassembling affected functions Josh Poimboeuf
@ 2023-03-28  8:47   ` Miroslav Benes
  0 siblings, 0 replies; 16+ messages in thread
From: Miroslav Benes @ 2023-03-28  8:47 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: x86, linux-kernel, Peter Zijlstra

> +static inline void objdump_func(struct section *sec, unsigned long offset)
> +{
> +	struct symbol *sym = find_func_containing(sec, offset);

Unnecessary assignment?

> +	const char *script_relative = "scripts/objdump-func";
> +	bool is_text = (sec->sh.sh_flags & SHF_EXECINSTR);
> +	char *cmd, *srctree, *script;
> +
> +	if (is_text)
> +		sym = find_func_containing(sec, offset);
> +	if (!sym)
> +		sym = find_symbol_containing(sec, offset);
> +	if (!sym)
> +		return;

Miroslav

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] objtool: Add per-function rate limiting for unreachable warnings
  2023-03-27 16:00 ` [PATCH 4/5] objtool: Add per-function rate limiting for unreachable warnings Josh Poimboeuf
  2023-03-28  8:11   ` Peter Zijlstra
@ 2023-03-28  9:07   ` Miroslav Benes
  1 sibling, 0 replies; 16+ messages in thread
From: Miroslav Benes @ 2023-03-28  9:07 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: x86, linux-kernel, Peter Zijlstra

On Mon, 27 Mar 2023, Josh Poimboeuf wrote:

> Unreachable instruction warnings are rate limited to once per object
> file.  That no longer makes sense for vmlinux validation, which might
> have other unreachable instructions lurking in other places.  Change it
> to once per function.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
>  tools/objtool/check.c               | 4 ++++
>  tools/objtool/include/objtool/elf.h | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 73dd091c0075..67a684225702 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -4557,6 +4557,10 @@ static int validate_reachable_instructions(struct objtool_file *file)
>  		if (insn->visited || ignore_unreachable_insn(file, insn))
>  			continue;
>  
> +		if (insn->sym->warned)
> +			continue;
> +		insn->sym->warned = 1;
> +

Ok

>  		WARN_FUNC("unreachable instruction", insn->sec, insn->offset);
>  		return 1;

But we still return here when an unreachable instruction is encountered 
and warned about. Or maybe I am just misunderstanding the purpose.

If not, would it be better to just collect the number of warnings per 
object as we do elsewhere?

  warnings++;

and then at the end

  return warnings;

Miroslav

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 5/5] objtool: Add "missing __noreturn" warning
  2023-03-27 16:00 ` [PATCH 5/5] objtool: Add "missing __noreturn" warning Josh Poimboeuf
@ 2023-03-28  9:10   ` Miroslav Benes
  2023-03-29 16:24     ` Josh Poimboeuf
  0 siblings, 1 reply; 16+ messages in thread
From: Miroslav Benes @ 2023-03-28  9:10 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: x86, linux-kernel, Peter Zijlstra

>  		WARN_FUNC("unreachable instruction", insn->sec, insn->offset);
> -		return 1;

I knew I should have read the whole set first...

Miroslav

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] objtool: Combine '--backtrace' with '--verbose'
  2023-03-28  8:07   ` Peter Zijlstra
@ 2023-03-28 20:19     ` Josh Poimboeuf
  2023-03-29  7:25       ` Peter Zijlstra
  0 siblings, 1 reply; 16+ messages in thread
From: Josh Poimboeuf @ 2023-03-28 20:19 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes

On Tue, Mar 28, 2023 at 10:07:43AM +0200, Peter Zijlstra wrote:
> On Mon, Mar 27, 2023 at 09:00:45AM -0700, Josh Poimboeuf wrote:
> > Get rid of the '--backtrace' option, instead including that
> > functionality in '--verbose'.  This makes it easy to gather all the
> > information needed for diagnosing objtool warnings.
> 
> Hurmm.. can't we have verbose imply backtrace but keep the separate
> option? I'm not sure if I always want the objdump thing -- esp with
> multiple warnings on vmlinux that's going to be really slow -- better to
> dump the whole of vmlinux.o once at the end.

That's a good point, vmlinux would be unbearable for multiple warnings.

We could accumulate a list of affected functions and then supply that to
objdump-func.sh at the end and dump them all at the same time.

objdump-func.sh would need to be changed to look for multiple funcs at
once.

If I do that, do you still want the separate backtrace option?

-- 
Josh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] objtool: Add per-function rate limiting for unreachable warnings
  2023-03-28  8:11   ` Peter Zijlstra
@ 2023-03-28 20:22     ` Josh Poimboeuf
  2023-03-29  7:26       ` Peter Zijlstra
  0 siblings, 1 reply; 16+ messages in thread
From: Josh Poimboeuf @ 2023-03-28 20:22 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: x86, linux-kernel

On Tue, Mar 28, 2023 at 10:11:05AM +0200, Peter Zijlstra wrote:
> On Mon, Mar 27, 2023 at 09:00:47AM -0700, Josh Poimboeuf wrote:
> > Unreachable instruction warnings are rate limited to once per object
> > file.  That no longer makes sense for vmlinux validation, which might
> > have other unreachable instructions lurking in other places.  Change it
> > to once per function.
> 
> Do we want a negative option to disable this? --no-ratelimit or such?

Per-function rate-limiting is almost always the right thing, personally
I don't envision needing to disable it.

-- 
Josh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] objtool: Combine '--backtrace' with '--verbose'
  2023-03-28 20:19     ` Josh Poimboeuf
@ 2023-03-29  7:25       ` Peter Zijlstra
  0 siblings, 0 replies; 16+ messages in thread
From: Peter Zijlstra @ 2023-03-29  7:25 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes

On Tue, Mar 28, 2023 at 01:19:21PM -0700, Josh Poimboeuf wrote:
> On Tue, Mar 28, 2023 at 10:07:43AM +0200, Peter Zijlstra wrote:
> > On Mon, Mar 27, 2023 at 09:00:45AM -0700, Josh Poimboeuf wrote:
> > > Get rid of the '--backtrace' option, instead including that
> > > functionality in '--verbose'.  This makes it easy to gather all the
> > > information needed for diagnosing objtool warnings.
> > 
> > Hurmm.. can't we have verbose imply backtrace but keep the separate
> > option? I'm not sure if I always want the objdump thing -- esp with
> > multiple warnings on vmlinux that's going to be really slow -- better to
> > dump the whole of vmlinux.o once at the end.
> 
> That's a good point, vmlinux would be unbearable for multiple warnings.
> 
> We could accumulate a list of affected functions and then supply that to
> objdump-func.sh at the end and dump them all at the same time.
> 
> objdump-func.sh would need to be changed to look for multiple funcs at
> once.
> 
> If I do that, do you still want the separate backtrace option?

For now, lets keep the orthogonal options -- we can always remove it
later if we find they go unused. This automagic stuff is going to take a
bit of getting used to I recon ;-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] objtool: Add per-function rate limiting for unreachable warnings
  2023-03-28 20:22     ` Josh Poimboeuf
@ 2023-03-29  7:26       ` Peter Zijlstra
  0 siblings, 0 replies; 16+ messages in thread
From: Peter Zijlstra @ 2023-03-29  7:26 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: x86, linux-kernel

On Tue, Mar 28, 2023 at 01:22:05PM -0700, Josh Poimboeuf wrote:
> On Tue, Mar 28, 2023 at 10:11:05AM +0200, Peter Zijlstra wrote:
> > On Mon, Mar 27, 2023 at 09:00:47AM -0700, Josh Poimboeuf wrote:
> > > Unreachable instruction warnings are rate limited to once per object
> > > file.  That no longer makes sense for vmlinux validation, which might
> > > have other unreachable instructions lurking in other places.  Change it
> > > to once per function.
> > 
> > Do we want a negative option to disable this? --no-ratelimit or such?
> 
> Per-function rate-limiting is almost always the right thing, personally
> I don't envision needing to disable it.

Ok, fair enough. I'll let you know if I ever come across the need to
disable it :-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 5/5] objtool: Add "missing __noreturn" warning
  2023-03-28  9:10   ` Miroslav Benes
@ 2023-03-29 16:24     ` Josh Poimboeuf
  0 siblings, 0 replies; 16+ messages in thread
From: Josh Poimboeuf @ 2023-03-29 16:24 UTC (permalink / raw)
  To: Miroslav Benes; +Cc: x86, linux-kernel, Peter Zijlstra

On Tue, Mar 28, 2023 at 11:10:48AM +0200, Miroslav Benes wrote:
> >  		WARN_FUNC("unreachable instruction", insn->sec, insn->offset);
> > -		return 1;
> 
> I knew I should have read the whole set first...

Oops, guess that is in the wrong patch.

-- 
Josh

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2023-03-29 16:24 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-27 16:00 [PATCH 0/5] objtool: warning improvements Josh Poimboeuf
2023-03-27 16:00 ` [PATCH 1/5] objtool: Add '--verbose' option for disassembling affected functions Josh Poimboeuf
2023-03-28  8:47   ` Miroslav Benes
2023-03-27 16:00 ` [PATCH 2/5] objtool: Combine '--backtrace' with '--verbose' Josh Poimboeuf
2023-03-28  8:07   ` Peter Zijlstra
2023-03-28 20:19     ` Josh Poimboeuf
2023-03-29  7:25       ` Peter Zijlstra
2023-03-27 16:00 ` [PATCH 3/5] objtool: Remove superfluous dead_end_function() check Josh Poimboeuf
2023-03-27 16:00 ` [PATCH 4/5] objtool: Add per-function rate limiting for unreachable warnings Josh Poimboeuf
2023-03-28  8:11   ` Peter Zijlstra
2023-03-28 20:22     ` Josh Poimboeuf
2023-03-29  7:26       ` Peter Zijlstra
2023-03-28  9:07   ` Miroslav Benes
2023-03-27 16:00 ` [PATCH 5/5] objtool: Add "missing __noreturn" warning Josh Poimboeuf
2023-03-28  9:10   ` Miroslav Benes
2023-03-29 16:24     ` Josh Poimboeuf

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.