2023-04-12 19:10:25

by Josh Poimboeuf

Subject: [PATCH v2 0/9] objtool: warning improvements

v2:
- keep --backtrace
- hard-code awk logic (don't use objdump-func script)
- add OBJTOOL_VERBOSE=1
- add WARN_INSN()
- several other improvements/fixes - best to review from scratch ;-)

Josh Poimboeuf (9):
scripts/objdump-func: Support multiple functions
objtool: Add WARN_INSN()
objtool: Limit unreachable warnings to once per function
objtool: Add symbol iteration helpers
objtool: Add verbose option for disassembling affected functions
objtool: Include backtrace in verbose mode
objtool: Remove superfluous dead_end_function() check
objtool: Detect missing __noreturn annotations
objtool: Ignore exc_double_fault() __noreturn warnings

scripts/objdump-func | 34 +-
tools/objtool/Documentation/objtool.txt | 11 +
tools/objtool/builtin-check.c | 5 +
tools/objtool/check.c | 421 +++++++++++++-----------
tools/objtool/elf.c | 2 +-
tools/objtool/include/objtool/builtin.h | 1 +
tools/objtool/include/objtool/elf.h | 10 +
tools/objtool/include/objtool/warn.h | 22 +-
tools/objtool/orc_gen.c | 9 +-
9 files changed, 307 insertions(+), 208 deletions(-)

--
2.39.2

2023-04-12 19:10:34

by Josh Poimboeuf

Subject: [PATCH v2 1/9] scripts/objdump-func: Support multiple functions

Allow specifying multiple functions on the cmdline. Note this removes
the secret EXTRA_ARGS feature.

While at it, spread out the awk to make it more readable.

Signed-off-by: Josh Poimboeuf <[email protected]>
---
scripts/objdump-func | 34 +++++++++++++++++++++++++---------
1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/scripts/objdump-func b/scripts/objdump-func
index 4eb463dd9f52..7b15b873d0e2 100755
--- a/scripts/objdump-func
+++ b/scripts/objdump-func
@@ -3,7 +3,7 @@
#
# Disassemble a single function.
#
-# usage: objdump-func <file> <func>
+# usage: objdump-func <file> <func> [<func> ...]

set -o errexit
set -o nounset
@@ -13,17 +13,33 @@ OBJDUMP="${CROSS_COMPILE:-}objdump"
command -v gawk >/dev/null 2>&1 || die "gawk isn't installed"

usage() {
- echo "usage: objdump-func <file> <func>" >&2
+ echo "usage: objdump-func <file> <func> [<func> ...]" >&2
exit 1
}

[[ $# -lt 2 ]] && usage

OBJ=$1; shift
-FUNC=$1; shift
-
-# Secret feature to allow adding extra objdump args at the end
-EXTRA_ARGS=$@
-
-# Note this also matches compiler-added suffixes like ".cold", etc
-${OBJDUMP} -wdr $EXTRA_ARGS $OBJ | gawk -M -v f=$FUNC '/^$/ { P=0; } $0 ~ "<" f "(\\..*)?>:" { P=1; O=strtonum("0x" $1); } { if (P) { o=strtonum("0x" $1); printf("%04x ", o-O); print $0; } }'
+FUNCS=("$@")
+
+${OBJDUMP} -wdr $OBJ | gawk -M -v _funcs="${FUNCS[*]}" '
+ BEGIN { split(_funcs, funcs); }
+ /^$/ { func_match=0; }
+ /<.*>:/ {
+ f = gensub(/.*<(.*)>:/, "\\1", 1);
+ for (i in funcs) {
+ # match compiler-added suffixes like ".cold", etc
+ if (f ~ "^" funcs[i] "(\\..*)?") {
+ func_match = 1;
+ base = strtonum("0x" $1);
+ break;
+ }
+ }
+ }
+ {
+ if (func_match) {
+ addr = strtonum("0x" $1);
+ printf("%04x ", addr - base);
+ print;
+ }
+ }'
--
2.39.2

2023-04-12 19:11:19

by Josh Poimboeuf

Subject: [PATCH v2 3/9] objtool: Limit unreachable warnings to once per function

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

Note this affects some other (much rarer) non-fatal warnings as well.
In general I think one-warning-per-function makes sense, as related
warnings can accumulate quickly and we want to eventually get back to
failing the build with -Werror anyway.

Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 5 +++--
tools/objtool/include/objtool/elf.h | 1 +
tools/objtool/include/objtool/warn.h | 5 ++++-
3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e85440db7a5f..de0d0234527d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4509,6 +4509,7 @@ static int validate_sls(struct objtool_file *file)
static int validate_reachable_instructions(struct objtool_file *file)
{
struct instruction *insn;
+ int warnings = 0;

if (file->ignore_unreachables)
return 0;
@@ -4518,10 +4519,10 @@ static int validate_reachable_instructions(struct objtool_file *file)
continue;

WARN_INSN(insn, "unreachable instruction");
- return 1;
+ warnings++;
}

- return 0;
+ return warnings;
}

int check(struct objtool_file *file)
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;
};
diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index b1c920dc9516..4ef9b278e5fd 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -55,7 +55,10 @@ static inline char *offstr(struct section *sec, unsigned long offset)

#define WARN_INSN(insn, format, ...) \
({ \
- WARN_FUNC(format, insn->sec, insn->offset, ##__VA_ARGS__); \
+ if (!insn->sym || !insn->sym->warned) \
+ WARN_FUNC(format, insn->sec, insn->offset, ##__VA_ARGS__);\
+ if (insn->sym) \
+ insn->sym->warned = 1; \
})

#define BT_FUNC(format, insn, ...) \
--
2.39.2

2023-04-12 19:12:59

by Josh Poimboeuf

Subject: [PATCH v2 4/9] objtool: Add symbol iteration helpers

Add [sec_]for_each_sym() and use them.

Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 98 ++++++++++++-----------------
tools/objtool/elf.c | 2 +-
tools/objtool/include/objtool/elf.h | 9 +++
3 files changed, 51 insertions(+), 58 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index de0d0234527d..d1d47baa252c 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -470,7 +470,7 @@ static int decode_instructions(struct objtool_file *file)

// printf("%s: last chunk used: %d\n", sec->name, (int)idx);

- list_for_each_entry(func, &sec->symbol_list, list) {
+ sec_for_each_sym(sec, func) {
if (func->type != STT_NOTYPE && func->type != STT_FUNC)
continue;

@@ -924,7 +924,7 @@ static int create_ibt_endbr_seal_sections(struct objtool_file *file)

static int create_cfi_sections(struct objtool_file *file)
{
- struct section *sec, *s;
+ struct section *sec;
struct symbol *sym;
unsigned int *loc;
int idx;
@@ -937,19 +937,14 @@ static int create_cfi_sections(struct objtool_file *file)
}

idx = 0;
- for_each_sec(file, s) {
- if (!s->text)
+ for_each_sym(file, sym) {
+ if (sym->type != STT_FUNC)
continue;

- list_for_each_entry(sym, &s->symbol_list, list) {
- if (sym->type != STT_FUNC)
- continue;
-
- if (strncmp(sym->name, "__cfi_", 6))
- continue;
+ if (strncmp(sym->name, "__cfi_", 6))
+ continue;

- idx++;
- }
+ idx++;
}

sec = elf_create_section(file->elf, ".cfi_sites", 0, sizeof(unsigned int), idx);
@@ -957,28 +952,23 @@ static int create_cfi_sections(struct objtool_file *file)
return -1;

idx = 0;
- for_each_sec(file, s) {
- if (!s->text)
+ for_each_sym(file, sym) {
+ if (sym->type != STT_FUNC)
continue;

- list_for_each_entry(sym, &s->symbol_list, list) {
- if (sym->type != STT_FUNC)
- continue;
-
- if (strncmp(sym->name, "__cfi_", 6))
- continue;
+ if (strncmp(sym->name, "__cfi_", 6))
+ continue;

- loc = (unsigned int *)sec->data->d_buf + idx;
- memset(loc, 0, sizeof(unsigned int));
+ loc = (unsigned int *)sec->data->d_buf + idx;
+ memset(loc, 0, sizeof(unsigned int));

- if (elf_add_reloc_to_insn(file->elf, sec,
- idx * sizeof(unsigned int),
- R_X86_64_PC32,
- s, sym->offset))
- return -1;
+ if (elf_add_reloc_to_insn(file->elf, sec,
+ idx * sizeof(unsigned int),
+ R_X86_64_PC32,
+ sym->sec, sym->offset))
+ return -1;

- idx++;
- }
+ idx++;
}

return 0;
@@ -2205,23 +2195,20 @@ static int add_func_jump_tables(struct objtool_file *file,
*/
static int add_jump_table_alts(struct objtool_file *file)
{
- struct section *sec;
struct symbol *func;
int ret;

if (!file->rodata)
return 0;

- for_each_sec(file, sec) {
- list_for_each_entry(func, &sec->symbol_list, list) {
- if (func->type != STT_FUNC)
- continue;
+ for_each_sym(file, func) {
+ if (func->type != STT_FUNC)
+ continue;

- mark_func_jump_tables(file, func);
- ret = add_func_jump_tables(file, func);
- if (ret)
- return ret;
- }
+ mark_func_jump_tables(file, func);
+ ret = add_func_jump_tables(file, func);
+ if (ret)
+ return ret;
}

return 0;
@@ -2533,30 +2520,27 @@ static bool is_profiling_func(const char *name)

static int classify_symbols(struct objtool_file *file)
{
- struct section *sec;
struct symbol *func;

- for_each_sec(file, sec) {
- list_for_each_entry(func, &sec->symbol_list, list) {
- if (func->bind != STB_GLOBAL)
- continue;
+ for_each_sym(file, func) {
+ if (func->bind != STB_GLOBAL)
+ continue;

- if (!strncmp(func->name, STATIC_CALL_TRAMP_PREFIX_STR,
- strlen(STATIC_CALL_TRAMP_PREFIX_STR)))
- func->static_call_tramp = true;
+ if (!strncmp(func->name, STATIC_CALL_TRAMP_PREFIX_STR,
+ strlen(STATIC_CALL_TRAMP_PREFIX_STR)))
+ func->static_call_tramp = true;

- if (arch_is_retpoline(func))
- func->retpoline_thunk = true;
+ if (arch_is_retpoline(func))
+ func->retpoline_thunk = true;

- if (arch_is_rethunk(func))
- func->return_thunk = true;
+ if (arch_is_rethunk(func))
+ func->return_thunk = true;

- if (arch_ftrace_match(func->name))
- func->fentry = true;
+ if (arch_ftrace_match(func->name))
+ func->fentry = true;

- if (is_profiling_func(func->name))
- func->profiling_func = true;
- }
+ if (is_profiling_func(func->name))
+ func->profiling_func = true;
}

return 0;
@@ -4222,7 +4206,7 @@ static int validate_section(struct objtool_file *file, struct section *sec)
struct symbol *func;
int warnings = 0;

- list_for_each_entry(func, &sec->symbol_list, list) {
+ sec_for_each_sym(sec, func) {
if (func->type != STT_FUNC)
continue;

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 6806ce01d933..500e92979a31 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -474,7 +474,7 @@ static int read_symbols(struct elf *elf)

/* Create parent/child links for any cold subfunctions */
list_for_each_entry(sec, &elf->sections, list) {
- list_for_each_entry(sym, &sec->symbol_list, list) {
+ sec_for_each_sym(sec, sym) {
char pname[MAX_NAME_LEN + 1];
size_t pnamelen;
if (sym->type != STT_FUNC)
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index a668173a5869..78e2d0fc21ca 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -189,4 +189,13 @@ struct symbol *find_func_containing(struct section *sec, unsigned long offset);
#define for_each_sec(file, sec) \
list_for_each_entry(sec, &file->elf->sections, list)

+#define sec_for_each_sym(sec, sym) \
+ list_for_each_entry(sym, &sec->symbol_list, list)
+
+#define for_each_sym(file, sym) \
+ for (struct section *__sec, *__fake = (struct section *)1; \
+ __fake; __fake = NULL) \
+ for_each_sec(file, __sec) \
+ sec_for_each_sym(__sec, sym)
+
#endif /* _OBJTOOL_ELF_H */
--
2.39.2

2023-04-12 19:13:15

by Josh Poimboeuf

Subject: [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions

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 <[email protected]>
---
tools/objtool/Documentation/objtool.txt | 5 ++
tools/objtool/builtin-check.c | 5 ++
tools/objtool/check.c | 77 +++++++++++++++++++++++++
tools/objtool/include/objtool/builtin.h | 1 +
4 files changed, 88 insertions(+)

diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt
index 8e53fc6735ef..4d6c5acde7a3 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 recreate with
+OBJTOOL_VERBOSE=1 (e.g., "make OBJTOOL_VERBOSE=1") and send the full
+output, including any 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..5e21cfb7661d 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(),
};
@@ -118,6 +119,10 @@ int cmd_parse_options(int argc, const char **argv, const char * const usage[])
parse_options(envc, envv, check_options, env_usage, 0);
}

+ env = getenv("OBJTOOL_VERBOSE");
+ if (env && !strcmp(env, "1"))
+ opts.verbose = true;
+
argc = parse_options(argc, argv, check_options, usage, 0);
if (argc != 1)
usage_with_options(usage, check_options);
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index d1d47baa252c..bc9dd69c9a45 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4509,6 +4509,81 @@ static int validate_reachable_instructions(struct objtool_file *file)
return warnings;
}

+/* 'funcs' is a space-separated list of function names */
+static int disas_funcs(const char *funcs)
+{
+ const char *objdump_str, *cross_compile;
+ int size, ret;
+ char *cmd;
+
+ cross_compile = getenv("CROSS_COMPILE");
+
+ objdump_str = "%sobjdump -wdr %s | gawk -M -v _funcs='%s' '"
+ "BEGIN { split(_funcs, funcs); }"
+ "/^$/ { func_match = 0; }"
+ "/<.*>:/ { "
+ "f = gensub(/.*<(.*)>:/, \"\\\\1\", 1);"
+ "for (i in funcs) {"
+ "if (funcs[i] == f) {"
+ "func_match = 1;"
+ "base = strtonum(\"0x\" $1);"
+ "break;"
+ "}"
+ "}"
+ "}"
+ "{"
+ "if (func_match) {"
+ "addr = strtonum(\"0x\" $1);"
+ "printf(\"%%04x \", addr - base);"
+ "print;"
+ "}"
+ "}' 1>&2";
+
+ /* fake snprintf() to calculate the size */
+ size = snprintf(NULL, 0, objdump_str, cross_compile, objname, funcs) + 1;
+ if (size <= 0) {
+ WARN("objdump string size calculation failed");
+ return -1;
+ }
+
+ cmd = malloc(size);
+
+ /* real snprintf() */
+ snprintf(cmd, size, objdump_str, cross_compile, objname, funcs);
+ ret = system(cmd);
+ if (ret) {
+ WARN("disassembly failed: %d", ret);
+ return -1;
+ }
+
+ return 0;
+}
+
+static int disas_warned_funcs(struct objtool_file *file)
+{
+ struct symbol *sym;
+ char *funcs = NULL, *tmp;
+
+ for_each_sym(file, sym) {
+ if (sym->warned) {
+ if (!funcs) {
+ funcs = malloc(strlen(sym->name) + 1);
+ strcpy(funcs, sym->name);
+ } else {
+ tmp = malloc(strlen(funcs) + strlen(sym->name) + 2);
+ sprintf(tmp, "%s %s", funcs, sym->name);
+ free(funcs);
+ funcs = tmp;
+ }
+ }
+ }
+
+ if (funcs)
+ disas_funcs(funcs);
+
+ return 0;
+}
+
int check(struct objtool_file *file)
{
int ret, warnings = 0;
@@ -4646,6 +4721,8 @@ int check(struct objtool_file *file)
warnings += ret;
}

+ if (opts.verbose)
+ disas_warned_funcs(file);

if (opts.stats) {
printf("nr_insns_visited: %ld\n", nr_insns_visited);
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;
--
2.39.2

2023-04-12 19:14:49

by Josh Poimboeuf

Subject: [PATCH v2 7/9] objtool: Remove superfluous dead_end_function() check

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

Signed-off-by: Josh Poimboeuf <[email protected]>
---
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 b9e5e0e9c1ee..5e7d3c62fb9d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4081,8 +4081,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

2023-04-12 19:14:49

by Josh Poimboeuf

Subject: [PATCH v2 6/9] objtool: Include backtrace in verbose mode

Include backtrace in verbose mode. This makes it easy to gather all the
information needed for diagnosing objtool warnings.

Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/Documentation/objtool.txt | 4 ++--
tools/objtool/check.c | 26 ++++++++++---------------
tools/objtool/include/objtool/warn.h | 14 +++++++------
3 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt
index 4d6c5acde7a3..5a69c207a10e 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 recreate with
OBJTOOL_VERBOSE=1 (e.g., "make OBJTOOL_VERBOSE=1") and send the full
-output, including any disassembly below the warning, to the objtool
-maintainers.
+output, including any disassembly or 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/check.c b/tools/objtool/check.c
index bc9dd69c9a45..b9e5e0e9c1ee 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3654,8 +3654,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_INSN(insn, "(alt)");
return ret;
}
}
@@ -3700,8 +3699,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_INSN(insn, "(branch)");
return ret;
}
}
@@ -3799,8 +3797,8 @@ 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)
- BT_FUNC("<=== (hint)", insn);
+ if (ret)
+ BT_INSN(insn, "<=== (hint)");
return ret;
}

@@ -3858,8 +3856,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_INSN(insn, "(alt)");
return ret;
}
}
@@ -3885,10 +3882,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_INSN(insn, "(branch%s)",
+ insn->type == INSN_JUMP_CONDITIONAL ? "-cond" : "");
return ret;
}

@@ -3910,8 +3905,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_INSN(insn, "(call)");
return ret;
}
/*
@@ -4195,8 +4189,8 @@ 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)
- BT_FUNC("<=== (sym)", insn);
+ if (ret)
+ BT_INSN(insn, "<=== (sym)");
return ret;
}

diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index 4ef9b278e5fd..856ea8bc10d7 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -61,12 +61,14 @@ static inline char *offstr(struct section *sec, unsigned long offset)
insn->sym->warned = 1; \
})

-#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_INSN(insn, format, ...) \
+({ \
+ if (opts.verbose || opts.backtrace) { \
+ 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

2023-04-12 19:15:19

by Josh Poimboeuf

Subject: [PATCH 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 <[email protected]>
---
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.

4. file.o: warning: objtool: func(): can't find starting instruction
or
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 5e7d3c62fb9d..60f2d649f19f 100644
--- 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;
+ }
+ }
+
WARN_INSN(insn, "unreachable instruction");
warnings++;
}
--
2.39.2

2023-04-12 19:15:31

by Josh Poimboeuf

Subject: [PATCH v2 2/9] objtool: Add WARN_INSN()

It's easier to use and also gives easy access to the instruction's
containing function, which is useful for printing that function's
symbol. It will also be useful in the future for rate-limiting and
disassembly of warned functions.

Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 171 ++++++++++-----------------
tools/objtool/include/objtool/warn.h | 5 +
tools/objtool/orc_gen.c | 9 +-
3 files changed, 70 insertions(+), 115 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index cae6ac6ff246..e85440db7a5f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1444,7 +1444,7 @@ static void annotate_call_site(struct objtool_file *file,

if (opts.mcount && sym->fentry) {
if (sibling)
- WARN_FUNC("Tail call to __fentry__ !?!?", insn->sec, insn->offset);
+ WARN_INSN(insn, "tail call to __fentry__ !?!?");
if (opts.mnop) {
if (reloc) {
reloc->type = R_NONE;
@@ -1646,9 +1646,8 @@ static int add_jump_destinations(struct objtool_file *file)
continue;
}

- WARN_FUNC("can't find jump dest instruction at %s+0x%lx",
- insn->sec, insn->offset, dest_sec->name,
- dest_off);
+ WARN_INSN(insn, "can't find jump dest instruction at %s+0x%lx",
+ dest_sec->name, dest_off);
return -1;
}

@@ -1731,13 +1730,12 @@ static int add_call_destinations(struct objtool_file *file)
continue;

if (!insn_call_dest(insn)) {
- WARN_FUNC("unannotated intra-function call", insn->sec, insn->offset);
+ WARN_INSN(insn, "unannotated intra-function call");
return -1;
}

if (insn_func(insn) && insn_call_dest(insn)->type != STT_FUNC) {
- WARN_FUNC("unsupported call to non-function",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unsupported call to non-function");
return -1;
}

@@ -1745,10 +1743,8 @@ static int add_call_destinations(struct objtool_file *file)
dest_off = arch_dest_reloc_offset(reloc->addend);
dest = find_call_destination(reloc->sym->sec, dest_off);
if (!dest) {
- WARN_FUNC("can't find call dest symbol at %s+0x%lx",
- insn->sec, insn->offset,
- reloc->sym->sec->name,
- dest_off);
+ WARN_INSN(insn, "can't find call dest symbol at %s+0x%lx",
+ reloc->sym->sec->name, dest_off);
return -1;
}

@@ -1808,8 +1804,7 @@ static int handle_group_alt(struct objtool_file *file,
} else {
if (orig_alt_group->last_insn->offset + orig_alt_group->last_insn->len -
orig_alt_group->first_insn->offset != special_alt->orig_len) {
- WARN_FUNC("weirdly overlapping alternative! %ld != %d",
- orig_insn->sec, orig_insn->offset,
+ WARN_INSN(orig_insn, "weirdly overlapping alternative! %ld != %d",
orig_alt_group->last_insn->offset +
orig_alt_group->last_insn->len -
orig_alt_group->first_insn->offset,
@@ -1878,8 +1873,7 @@ static int handle_group_alt(struct objtool_file *file,
if (alt_reloc && arch_pc_relative_reloc(alt_reloc) &&
!arch_support_alt_relocation(special_alt, insn, alt_reloc)) {

- WARN_FUNC("unsupported relocation in alternatives section",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unsupported relocation in alternatives section");
return -1;
}

@@ -1893,8 +1887,7 @@ static int handle_group_alt(struct objtool_file *file,
if (dest_off == special_alt->new_off + special_alt->new_len) {
insn->jump_dest = next_insn_same_sec(file, orig_alt_group->last_insn);
if (!insn->jump_dest) {
- WARN_FUNC("can't find alternative jump destination",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "can't find alternative jump destination");
return -1;
}
}
@@ -1928,8 +1921,7 @@ static int handle_jump_alt(struct objtool_file *file,
if (orig_insn->type != INSN_JUMP_UNCONDITIONAL &&
orig_insn->type != INSN_NOP) {

- WARN_FUNC("unsupported instruction at jump label",
- orig_insn->sec, orig_insn->offset);
+ WARN_INSN(orig_insn, "unsupported instruction at jump label");
return -1;
}

@@ -2008,8 +2000,7 @@ static int add_special_section_alts(struct objtool_file *file)

if (special_alt->group) {
if (!special_alt->orig_len) {
- WARN_FUNC("empty alternative entry",
- orig_insn->sec, orig_insn->offset);
+ WARN_INSN(orig_insn, "empty alternative entry");
continue;
}

@@ -2100,8 +2091,7 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
}

if (!prev_offset) {
- WARN_FUNC("can't find switch jump table",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "can't find switch jump table");
return -1;
}

@@ -2305,8 +2295,7 @@ static int read_unwind_hints(struct objtool_file *file)

if (sym && sym->bind == STB_GLOBAL) {
if (opts.ibt && insn->type != INSN_ENDBR && !insn->noendbr) {
- WARN_FUNC("UNWIND_HINT_IRET_REGS without ENDBR",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "UNWIND_HINT_IRET_REGS without ENDBR");
}
}
}
@@ -2320,8 +2309,7 @@ static int read_unwind_hints(struct objtool_file *file)
cfi = *(insn->cfi);

if (arch_decode_hint_reg(hint->sp_reg, &cfi.cfa.base)) {
- WARN_FUNC("unsupported unwind_hint sp base reg %d",
- insn->sec, insn->offset, hint->sp_reg);
+ WARN_INSN(insn, "unsupported unwind_hint sp base reg %d", hint->sp_reg);
return -1;
}

@@ -2384,8 +2372,7 @@ static int read_retpoline_hints(struct objtool_file *file)
insn->type != INSN_CALL_DYNAMIC &&
insn->type != INSN_RETURN &&
insn->type != INSN_NOP) {
- WARN_FUNC("retpoline_safe hint not an indirect jump/call/ret/nop",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "retpoline_safe hint not an indirect jump/call/ret/nop");
return -1;
}

@@ -2496,8 +2483,7 @@ static int read_intra_function_calls(struct objtool_file *file)
}

if (insn->type != INSN_CALL) {
- WARN_FUNC("intra_function_call not a direct call",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "intra_function_call not a direct call");
return -1;
}

@@ -2511,8 +2497,7 @@ static int read_intra_function_calls(struct objtool_file *file)
dest_off = arch_jump_destination(insn);
insn->jump_dest = find_insn(file, insn->sec, dest_off);
if (!insn->jump_dest) {
- WARN_FUNC("can't find call dest at %s+0x%lx",
- insn->sec, insn->offset,
+ WARN_INSN(insn, "can't find call dest at %s+0x%lx",
insn->sec->name, dest_off);
return -1;
}
@@ -2853,7 +2838,7 @@ static int update_cfi_state(struct instruction *insn,
/* stack operations don't make sense with an undefined CFA */
if (cfa->base == CFI_UNDEFINED) {
if (insn_func(insn)) {
- WARN_FUNC("undefined stack state", insn->sec, insn->offset);
+ WARN_INSN(insn, "undefined stack state");
return -1;
}
return 0;
@@ -3047,8 +3032,7 @@ static int update_cfi_state(struct instruction *insn,
}

if (op->dest.reg == cfi->cfa.base && !(next_insn && next_insn->hint)) {
- WARN_FUNC("unsupported stack register modification",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unsupported stack register modification");
return -1;
}

@@ -3058,8 +3042,7 @@ static int update_cfi_state(struct instruction *insn,
if (op->dest.reg != CFI_SP ||
(cfi->drap_reg != CFI_UNDEFINED && cfa->base != CFI_SP) ||
(cfi->drap_reg == CFI_UNDEFINED && cfa->base != CFI_BP)) {
- WARN_FUNC("unsupported stack pointer realignment",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unsupported stack pointer realignment");
return -1;
}

@@ -3154,8 +3137,7 @@ static int update_cfi_state(struct instruction *insn,
break;

default:
- WARN_FUNC("unknown stack-related instruction",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unknown stack-related instruction");
return -1;
}

@@ -3244,8 +3226,7 @@ static int update_cfi_state(struct instruction *insn,

case OP_DEST_MEM:
if (op->src.type != OP_SRC_POP && op->src.type != OP_SRC_POPF) {
- WARN_FUNC("unknown stack-related memory operation",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unknown stack-related memory operation");
return -1;
}

@@ -3257,8 +3238,7 @@ static int update_cfi_state(struct instruction *insn,
break;

default:
- WARN_FUNC("unknown stack-related instruction",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unknown stack-related instruction");
return -1;
}

@@ -3297,8 +3277,7 @@ static int propagate_alt_cfi(struct objtool_file *file, struct instruction *insn
struct alt_group *orig_group = insn->alt_group->orig_group ?: insn->alt_group;
struct instruction *orig = orig_group->first_insn;
char *where = offstr(insn->sec, insn->offset);
- WARN_FUNC("stack layout conflict in alternatives: %s",
- orig->sec, orig->offset, where);
+ WARN_INSN(orig, "stack layout conflict in alternatives: %s", where);
free(where);
return -1;
}
@@ -3325,8 +3304,7 @@ static int handle_insn_ops(struct instruction *insn,
if (!state->uaccess_stack) {
state->uaccess_stack = 1;
} else if (state->uaccess_stack >> 31) {
- WARN_FUNC("PUSHF stack exhausted",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "PUSHF stack exhausted");
return 1;
}
state->uaccess_stack <<= 1;
@@ -3358,8 +3336,7 @@ static bool insn_cfi_match(struct instruction *insn, struct cfi_state *cfi2)

if (memcmp(&cfi1->cfa, &cfi2->cfa, sizeof(cfi1->cfa))) {

- WARN_FUNC("stack state mismatch: cfa1=%d%+d cfa2=%d%+d",
- insn->sec, insn->offset,
+ WARN_INSN(insn, "stack state mismatch: cfa1=%d%+d cfa2=%d%+d",
cfi1->cfa.base, cfi1->cfa.offset,
cfi2->cfa.base, cfi2->cfa.offset);

@@ -3369,8 +3346,7 @@ static bool insn_cfi_match(struct instruction *insn, struct cfi_state *cfi2)
sizeof(struct cfi_reg)))
continue;

- WARN_FUNC("stack state mismatch: reg1[%d]=%d%+d reg2[%d]=%d%+d",
- insn->sec, insn->offset,
+ WARN_INSN(insn, "stack state mismatch: reg1[%d]=%d%+d reg2[%d]=%d%+d",
i, cfi1->regs[i].base, cfi1->regs[i].offset,
i, cfi2->regs[i].base, cfi2->regs[i].offset);
break;
@@ -3378,15 +3354,14 @@ static bool insn_cfi_match(struct instruction *insn, struct cfi_state *cfi2)

} else if (cfi1->type != cfi2->type) {

- WARN_FUNC("stack state mismatch: type1=%d type2=%d",
- insn->sec, insn->offset, cfi1->type, cfi2->type);
+ WARN_INSN(insn, "stack state mismatch: type1=%d type2=%d",
+ cfi1->type, cfi2->type);

} else if (cfi1->drap != cfi2->drap ||
(cfi1->drap && cfi1->drap_reg != cfi2->drap_reg) ||
(cfi1->drap && cfi1->drap_offset != cfi2->drap_offset)) {

- WARN_FUNC("stack state mismatch: drap1=%d(%d,%d) drap2=%d(%d,%d)",
- insn->sec, insn->offset,
+ WARN_INSN(insn, "stack state mismatch: drap1=%d(%d,%d) drap2=%d(%d,%d)",
cfi1->drap, cfi1->drap_reg, cfi1->drap_offset,
cfi2->drap, cfi2->drap_reg, cfi2->drap_offset);

@@ -3494,20 +3469,17 @@ static int validate_call(struct objtool_file *file,
{
if (state->noinstr && state->instr <= 0 &&
!noinstr_call_dest(file, insn, insn_call_dest(insn))) {
- WARN_FUNC("call to %s() leaves .noinstr.text section",
- insn->sec, insn->offset, call_dest_name(insn));
+ WARN_INSN(insn, "call to %s() leaves .noinstr.text section", call_dest_name(insn));
return 1;
}

if (state->uaccess && !func_uaccess_safe(insn_call_dest(insn))) {
- WARN_FUNC("call to %s() with UACCESS enabled",
- insn->sec, insn->offset, call_dest_name(insn));
+ WARN_INSN(insn, "call to %s() with UACCESS enabled", call_dest_name(insn));
return 1;
}

if (state->df) {
- WARN_FUNC("call to %s() with DF set",
- insn->sec, insn->offset, call_dest_name(insn));
+ WARN_INSN(insn, "call to %s() with DF set", call_dest_name(insn));
return 1;
}

@@ -3519,8 +3491,7 @@ static int validate_sibling_call(struct objtool_file *file,
struct insn_state *state)
{
if (insn_func(insn) && has_modified_stack_frame(insn, state)) {
- WARN_FUNC("sibling call from callable instruction with modified stack frame",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "sibling call from callable instruction with modified stack frame");
return 1;
}

@@ -3530,38 +3501,32 @@ static int validate_sibling_call(struct objtool_file *file,
static int validate_return(struct symbol *func, struct instruction *insn, struct insn_state *state)
{
if (state->noinstr && state->instr > 0) {
- WARN_FUNC("return with instrumentation enabled",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "return with instrumentation enabled");
return 1;
}

if (state->uaccess && !func_uaccess_safe(func)) {
- WARN_FUNC("return with UACCESS enabled",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "return with UACCESS enabled");
return 1;
}

if (!state->uaccess && func_uaccess_safe(func)) {
- WARN_FUNC("return with UACCESS disabled from a UACCESS-safe function",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "return with UACCESS disabled from a UACCESS-safe function");
return 1;
}

if (state->df) {
- WARN_FUNC("return with DF set",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "return with DF set");
return 1;
}

if (func && has_modified_stack_frame(insn, state)) {
- WARN_FUNC("return with modified stack frame",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "return with modified stack frame");
return 1;
}

if (state->cfi.bp_scratch) {
- WARN_FUNC("BP used as a scratch register",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "BP used as a scratch register");
return 1;
}

@@ -3633,8 +3598,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
}

if (func && insn->ignore) {
- WARN_FUNC("BUG: why am I validating an ignored function?",
- sec, insn->offset);
+ WARN_INSN(insn, "BUG: why am I validating an ignored function?");
return 1;
}

@@ -3667,14 +3631,12 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
}

if (!save_insn) {
- WARN_FUNC("no corresponding CFI save for CFI restore",
- sec, insn->offset);
+ WARN_INSN(insn, "no corresponding CFI save for CFI restore");
return 1;
}

if (!save_insn->visited) {
- WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
- sec, insn->offset);
+ WARN_INSN(insn, "objtool isn't smart enough to handle this CFI save/restore combo");
return 1;
}

@@ -3734,8 +3696,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,

if (opts.stackval && func && !is_fentry_call(insn) &&
!has_valid_stack_frame(&state)) {
- WARN_FUNC("call without frame pointer save/setup",
- sec, insn->offset);
+ WARN_INSN(insn, "call without frame pointer save/setup");
return 1;
}

@@ -3781,15 +3742,14 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,

case INSN_CONTEXT_SWITCH:
if (func && (!next_insn || !next_insn->hint)) {
- WARN_FUNC("unsupported instruction in callable function",
- sec, insn->offset);
+ WARN_INSN(insn, "unsupported instruction in callable function");
return 1;
}
return 0;

case INSN_STAC:
if (state.uaccess) {
- WARN_FUNC("recursive UACCESS enable", sec, insn->offset);
+ WARN_INSN(insn, "recursive UACCESS enable");
return 1;
}

@@ -3798,12 +3758,12 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,

case INSN_CLAC:
if (!state.uaccess && func) {
- WARN_FUNC("redundant UACCESS disable", sec, insn->offset);
+ WARN_INSN(insn, "redundant UACCESS disable");
return 1;
}

if (func_uaccess_safe(func) && !state.uaccess_stack) {
- WARN_FUNC("UACCESS-safe disables UACCESS", sec, insn->offset);
+ WARN_INSN(insn, "UACCESS-safe disables UACCESS");
return 1;
}

@@ -3812,7 +3772,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,

case INSN_STD:
if (state.df) {
- WARN_FUNC("recursive STD", sec, insn->offset);
+ WARN_INSN(insn, "recursive STD");
return 1;
}

@@ -3821,7 +3781,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,

case INSN_CLD:
if (!state.df && func) {
- WARN_FUNC("redundant CLD", sec, insn->offset);
+ WARN_INSN(insn, "redundant CLD");
return 1;
}

@@ -3929,15 +3889,14 @@ static int validate_unret(struct objtool_file *file, struct instruction *insn)
case INSN_CALL_DYNAMIC:
case INSN_JUMP_DYNAMIC:
case INSN_JUMP_DYNAMIC_CONDITIONAL:
- WARN_FUNC("early indirect call", insn->sec, insn->offset);
+ WARN_INSN(insn, "early indirect call");
return 1;

case INSN_JUMP_UNCONDITIONAL:
case INSN_JUMP_CONDITIONAL:
if (!is_sibling_call(insn)) {
if (!insn->jump_dest) {
- WARN_FUNC("unresolved jump target after linking?!?",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unresolved jump target after linking?!?");
return -1;
}
ret = validate_unret(file, insn->jump_dest);
@@ -3978,7 +3937,7 @@ static int validate_unret(struct objtool_file *file, struct instruction *insn)
return 0;

case INSN_RETURN:
- WARN_FUNC("RET before UNTRAIN", insn->sec, insn->offset);
+ WARN_INSN(insn, "RET before UNTRAIN");
return 1;

case INSN_NOP:
@@ -3991,7 +3950,7 @@ static int validate_unret(struct objtool_file *file, struct instruction *insn)
}

if (!next) {
- WARN_FUNC("teh end!", insn->sec, insn->offset);
+ WARN_INSN(insn, "teh end!");
return -1;
}
insn = next;
@@ -4015,7 +3974,7 @@ static int validate_unrets(struct objtool_file *file)

ret = validate_unret(file, insn);
if (ret < 0) {
- WARN_FUNC("Failed UNRET validation", insn->sec, insn->offset);
+ WARN_INSN(insn, "Failed UNRET validation");
return ret;
}
warnings += ret;
@@ -4043,13 +4002,11 @@ static int validate_retpoline(struct objtool_file *file)

if (insn->type == INSN_RETURN) {
if (opts.rethunk) {
- WARN_FUNC("'naked' return found in RETHUNK build",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "'naked' return found in RETHUNK build");
} else
continue;
} else {
- WARN_FUNC("indirect %s found in RETPOLINE build",
- insn->sec, insn->offset,
+ WARN_INSN(insn, "indirect %s found in RETPOLINE build",
insn->type == INSN_JUMP_DYNAMIC ? "jump" : "call");
}

@@ -4428,9 +4385,7 @@ static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn
if (noendbr_range(file, dest))
continue;

- WARN_FUNC("relocation to !ENDBR: %s",
- insn->sec, insn->offset,
- offstr(dest->sec, dest->offset));
+ WARN_INSN(insn, "relocation to !ENDBR: %s", offstr(dest->sec, dest->offset));

warnings++;
}
@@ -4532,16 +4487,14 @@ static int validate_sls(struct objtool_file *file)
switch (insn->type) {
case INSN_RETURN:
if (!next_insn || next_insn->type != INSN_TRAP) {
- WARN_FUNC("missing int3 after ret",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "missing int3 after ret");
warnings++;
}

break;
case INSN_JUMP_DYNAMIC:
if (!next_insn || next_insn->type != INSN_TRAP) {
- WARN_FUNC("missing int3 after indirect jump",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "missing int3 after indirect jump");
warnings++;
}
break;
@@ -4564,7 +4517,7 @@ static int validate_reachable_instructions(struct objtool_file *file)
if (insn->visited || ignore_unreachable_insn(file, insn))
continue;

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

diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index a3e79ae75f2e..b1c920dc9516 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -53,6 +53,11 @@ static inline char *offstr(struct section *sec, unsigned long offset)
free(_str); \
})

+#define WARN_INSN(insn, format, ...) \
+({ \
+ WARN_FUNC(format, insn->sec, insn->offset, ##__VA_ARGS__); \
+})
+
#define BT_FUNC(format, insn, ...) \
({ \
struct instruction *_insn = (insn); \
diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index b327f9ccfe73..48efd1e2f00d 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -47,8 +47,7 @@ static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi,
orc->type = ORC_TYPE_REGS_PARTIAL;
break;
default:
- WARN_FUNC("unknown unwind hint type %d",
- insn->sec, insn->offset, cfi->type);
+ WARN_INSN(insn, "unknown unwind hint type %d", cfi->type);
return -1;
}

@@ -80,8 +79,7 @@ static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi,
orc->sp_reg = ORC_REG_DX;
break;
default:
- WARN_FUNC("unknown CFA base reg %d",
- insn->sec, insn->offset, cfi->cfa.base);
+ WARN_INSN(insn, "unknown CFA base reg %d", cfi->cfa.base);
return -1;
}

@@ -96,8 +94,7 @@ static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi,
orc->bp_reg = ORC_REG_BP;
break;
default:
- WARN_FUNC("unknown BP base reg %d",
- insn->sec, insn->offset, bp->base);
+ WARN_INSN(insn, "unknown BP base reg %d", bp->base);
return -1;
}

--
2.39.2

2023-04-12 19:16:20

by Josh Poimboeuf

Subject: [PATCH v2 9/9] objtool: Ignore exc_double_fault() __noreturn warnings

This is a hack, but it works for now.

Problem is, exc_double_fault() may or may not return, depending on
whether CONFIG_X86_ESPFIX64 is set. But objtool has no visibility to
the kernel config.

"Fix" it by silencing the exc_double_fault() __noreturn warning.

This removes the following warning:

vmlinux.o: warning: objtool: xenpv_exc_double_fault+0xd: exc_double_fault() is missing a __noreturn annotation

Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 60f2d649f19f..7641e818db7d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4483,6 +4483,35 @@ static int validate_sls(struct objtool_file *file)
return warnings;
}

+static bool ignore_noreturn_call(struct instruction *insn)
+{
+ struct symbol *call_dest = insn_call_dest(insn);
+
+ /*
+ * This is a hack, but it works for now.
+ *
+ * Problem is, exc_double_fault() may or may not return, depending on
+ * whether CONFIG_X86_ESPFIX64 is set. But objtool has no visibility
+ * to the kernel config.
+ *
+ * Other potential ways to fix it:
+ *
+ * - remove CONFIG_X86_ESPFIX64
+ * - read the .config file
+ * - add a cmdline option
+ * - create a generic objtool annotation format (vs a bunch of custom
+ * formats) and annotate it
+ * - have compiler communicate __noreturn functions somehow
+ */
+ if (!strcmp(call_dest->name, "exc_double_fault")) {
+ /* prevent further unreachable warnings for the caller */
+ insn->sym->warned = 1;
+ return true;
+ }
+
+ return false;
+}
+
static int validate_reachable_instructions(struct objtool_file *file)
{
struct instruction *insn, *prev_insn;
@@ -4499,7 +4528,7 @@ static int validate_reachable_instructions(struct objtool_file *file)
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) {
+ if (call_dest && !ignore_noreturn_call(prev_insn)) {
WARN_INSN(insn, "%s() is missing a __noreturn annotation",
call_dest->name);
warnings++;
--
2.39.2

2023-04-13 08:07:39

by Peter Zijlstra

Subject: Re: [PATCH v2 3/9] objtool: Limit unreachable warnings to once per function

On Wed, Apr 12, 2023 at 12:03:18PM -0700, Josh Poimboeuf wrote:

> diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
> index b1c920dc9516..4ef9b278e5fd 100644
> --- a/tools/objtool/include/objtool/warn.h
> +++ b/tools/objtool/include/objtool/warn.h
> @@ -55,7 +55,10 @@ static inline char *offstr(struct section *sec, unsigned long offset)
>
> #define WARN_INSN(insn, format, ...) \
> ({ \
> - WARN_FUNC(format, insn->sec, insn->offset, ##__VA_ARGS__); \
> + if (!insn->sym || !insn->sym->warned) \
> + WARN_FUNC(format, insn->sec, insn->offset, ##__VA_ARGS__);\
> + if (insn->sym) \
> + insn->sym->warned = 1; \
> })

Do we want to write that like:

#define WARN_INSN(insn, format, ...) \
({ \
struct instruction *_insn = (insn); \
if (!_insn->sym || !_insn->sym->warned) \
WARN_FUNC(format, _insn->sec, _insn->offset, ##__VA_ARGS__);\
if (_insn->sym) \
_insn->sym->warned = 1; \
})

instead ?

2023-04-13 08:09:59

by Peter Zijlstra

Subject: Re: [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions

On Wed, Apr 12, 2023 at 12:03:20PM -0700, Josh Poimboeuf wrote:
> 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 <[email protected]>
> ---
> tools/objtool/Documentation/objtool.txt | 5 ++
> tools/objtool/builtin-check.c | 5 ++
> tools/objtool/check.c | 77 +++++++++++++++++++++++++
> tools/objtool/include/objtool/builtin.h | 1 +
> 4 files changed, 88 insertions(+)
>
> diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt
> index 8e53fc6735ef..4d6c5acde7a3 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 recreate with
> +OBJTOOL_VERBOSE=1 (e.g., "make OBJTOOL_VERBOSE=1") and send the full
> +output, including any 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..5e21cfb7661d 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(),
> };
> @@ -118,6 +119,10 @@ int cmd_parse_options(int argc, const char **argv, const char * const usage[])
> parse_options(envc, envv, check_options, env_usage, 0);
> }
>
> + env = getenv("OBJTOOL_VERBOSE");
> + if (env && !strcmp(env, "1"))
> + opts.verbose = true;
> +
> argc = parse_options(argc, argv, check_options, usage, 0);
> if (argc != 1)
> usage_with_options(usage, check_options);

No real objection; but I do feel obliged to point out that:
OBJTOOL_ARGS="-v" achieves much the same.

2023-04-13 08:13:01

by Peter Zijlstra

Subject: Re: [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions

On Wed, Apr 12, 2023 at 12:03:20PM -0700, Josh Poimboeuf wrote:
> + objdump_str = "%sobjdump -wdr %s | gawk -M -v _funcs='%s' '"
> + "BEGIN { split(_funcs, funcs); }"
> + "/^$/ { func_match = 0; }"
> + "/<.*>:/ { "
> + "f = gensub(/.*<(.*)>:/, \"\\\\1\", 1);"
> + "for (i in funcs) {"
> + "if (funcs[i] == f) {"
> + "func_match = 1;"
> + "base = strtonum(\"0x\" $1);"
> + "break;"
> + "}"
> + "}"
> + "}"
> + "{"
> + "if (func_match) {"
> + "addr = strtonum(\"0x\" $1);"
> + "printf(\"%%04x \", addr - base);"
> + "print;"
> + "}"
> + "}' 1>&2";

Do we want to have scripts/objdump-func.awk and use that to avoid the
duplication and eventual divergence of these awk thingies?

2023-04-13 08:53:50

by Peter Zijlstra

Subject: Re: [PATCH v2 8/9] objtool: Detect missing __noreturn annotations

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 <[email protected]>
> ---
> 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

2023-04-13 09:29:53

by Peter Zijlstra

Subject: Re: [PATCH v2 9/9] objtool: Ignore exc_double_fault() __noreturn warnings

On Wed, Apr 12, 2023 at 12:03:24PM -0700, Josh Poimboeuf wrote:
> + * - have compiler communicate __noreturn functions somehow

This, we're going to have to do that -- because keeping that (ever
growing) list in sync is going to be a pain in the backside.

2023-04-13 14:13:40

by Miroslav Benes

Subject: Re: [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions

On Thu, 13 Apr 2023, Peter Zijlstra wrote:

> On Wed, Apr 12, 2023 at 12:03:20PM -0700, Josh Poimboeuf wrote:
> > + objdump_str = "%sobjdump -wdr %s | gawk -M -v _funcs='%s' '"
> > + "BEGIN { split(_funcs, funcs); }"
> > + "/^$/ { func_match = 0; }"
> > + "/<.*>:/ { "
> > + "f = gensub(/.*<(.*)>:/, \"\\\\1\", 1);"
> > + "for (i in funcs) {"
> > + "if (funcs[i] == f) {"
> > + "func_match = 1;"
> > + "base = strtonum(\"0x\" $1);"
> > + "break;"
> > + "}"
> > + "}"
> > + "}"
> > + "{"
> > + "if (func_match) {"
> > + "addr = strtonum(\"0x\" $1);"
> > + "printf(\"%%04x \", addr - base);"
> > + "print;"
> > + "}"
> > + "}' 1>&2";
>
> Do we want to have scripts/objdump-func.awk and use that to avoid the
> duplication and eventual divergence of these awk thingies?

I vote for that as well. To keep everything in sync can be a nightmare.

Miroslav

2023-04-13 14:21:19

by Miroslav Benes

Subject: Re: [PATCH v2 8/9] objtool: Detect missing __noreturn annotations

> --- 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

2023-04-13 15:10:37

by Josh Poimboeuf

Subject: Re: [PATCH v2 3/9] objtool: Limit unreachable warnings to once per function

On Thu, Apr 13, 2023 at 10:01:12AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 12, 2023 at 12:03:18PM -0700, Josh Poimboeuf wrote:
>
> > diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
> > index b1c920dc9516..4ef9b278e5fd 100644
> > --- a/tools/objtool/include/objtool/warn.h
> > +++ b/tools/objtool/include/objtool/warn.h
> > @@ -55,7 +55,10 @@ static inline char *offstr(struct section *sec, unsigned long offset)
> >
> > #define WARN_INSN(insn, format, ...) \
> > ({ \
> > - WARN_FUNC(format, insn->sec, insn->offset, ##__VA_ARGS__); \
> > + if (!insn->sym || !insn->sym->warned) \
> > + WARN_FUNC(format, insn->sec, insn->offset, ##__VA_ARGS__);\
> > + if (insn->sym) \
> > + insn->sym->warned = 1; \
> > })
>
> Do we want to write that like:
>
> #define WARN_INSN(insn, format, ...) \
> ({ \
> struct instruction *_insn = (insn); \
> if (!_insn->sym || !_insn->sym->warned) \
> WARN_FUNC(format, _insn->sec, _insn->offset, ##__VA_ARGS__);\
> if (_insn->sym) \
> _insn->sym->warned = 1; \
> })
>
> instead ?

Yes indeed.

--
Josh

2023-04-13 15:11:19

by Josh Poimboeuf

Subject: Re: [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions

On Thu, Apr 13, 2023 at 10:05:27AM +0200, Peter Zijlstra wrote:
> > @@ -118,6 +119,10 @@ int cmd_parse_options(int argc, const char **argv, const char * const usage[])
> > parse_options(envc, envv, check_options, env_usage, 0);
> > }
> >
> > + env = getenv("OBJTOOL_VERBOSE");
> > + if (env && !strcmp(env, "1"))
> > + opts.verbose = true;
> > +
> > argc = parse_options(argc, argv, check_options, usage, 0);
> > if (argc != 1)
> > usage_with_options(usage, check_options);
>
> No real objection; but I do feel obliged to point out that:
> OBJTOOL_ARGS="-v" achieves much the same.

But OBJTOOL_VERBOSE=1 just rolls off the fingers :-)

--
Josh

2023-04-13 15:12:57

by Josh Poimboeuf

Subject: Re: [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions

On Thu, Apr 13, 2023 at 04:03:07PM +0200, Miroslav Benes wrote:
> On Thu, 13 Apr 2023, Peter Zijlstra wrote:
>
> > On Wed, Apr 12, 2023 at 12:03:20PM -0700, Josh Poimboeuf wrote:
> > > + objdump_str = "%sobjdump -wdr %s | gawk -M -v _funcs='%s' '"
> > > + "BEGIN { split(_funcs, funcs); }"
> > > + "/^$/ { func_match = 0; }"
> > > + "/<.*>:/ { "
> > > + "f = gensub(/.*<(.*)>:/, \"\\\\1\", 1);"
> > > + "for (i in funcs) {"
> > > + "if (funcs[i] == f) {"
> > > + "func_match = 1;"
> > > + "base = strtonum(\"0x\" $1);"
> > > + "break;"
> > > + "}"
> > > + "}"
> > > + "}"
> > > + "{"
> > > + "if (func_match) {"
> > > + "addr = strtonum(\"0x\" $1);"
> > > + "printf(\"%%04x \", addr - base);"
> > > + "print;"
> > > + "}"
> > > + "}' 1>&2";
> >
> > Do we want to have scripts/objdump-func.awk and use that to avoid the
> > duplication and eventual divergence of these awk thingies?
>
> I vote for that as well. To keep everything in sync can be a nightmare.

Reasons I did it this way for v2:

1) The awk implementation is slightly different (this one doesn't match
*.cold, etc)

2) Objtool is self-sufficient (doesn't need any other files)

3) Any changes to the objdump-func interface might break objtool

4) It's clearly bug-free and self-contained and I don't expect the code
to change much in the future

--
Josh

2023-04-13 15:24:25

by Josh Poimboeuf

Subject: Re: [PATCH v2 8/9] objtool: Detect missing __noreturn annotations

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 <[email protected]>
> > ---
> > 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)

2023-04-13 15:37:45

by Josh Poimboeuf

Subject: Re: [PATCH v2 8/9] objtool: Detect missing __noreturn annotations

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 ;-)

--
Josh

2023-04-13 19:18:56

by Peter Zijlstra

Subject: Re: [PATCH v2 8/9] objtool: Detect missing __noreturn annotations

> 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.

2023-04-14 14:54:32

by tip-bot2 for Linus Torvalds

Subject: [tip: objtool/core] objtool: Remove superfluous dead_end_function() check

The following commit has been merged into the objtool/core branch of tip:

Commit-ID: 6126ed5dfbc656374e851bfdfb128f3aa9e1263a
Gitweb: https://git.kernel.org/tip/6126ed5dfbc656374e851bfdfb128f3aa9e1263a
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Wed, 12 Apr 2023 12:03:22 -07:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Fri, 14 Apr 2023 16:08:29 +02:00

objtool: Remove superfluous dead_end_function() check

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

Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/5d603a301e9a8b1036b61503385907e154867ace.1681325924.git.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 9de3972..1cf2e28 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4078,8 +4078,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)))

2023-04-14 14:56:14

by tip-bot2 for Linus Torvalds

Subject: [tip: objtool/core] objtool: Add symbol iteration helpers

The following commit has been merged into the objtool/core branch of tip:

Commit-ID: 9290e772baccecec324ae9f2e0b470f870c097de
Gitweb: https://git.kernel.org/tip/9290e772baccecec324ae9f2e0b470f870c097de
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Wed, 12 Apr 2023 12:03:19 -07:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Fri, 14 Apr 2023 16:08:29 +02:00

objtool: Add symbol iteration helpers

Add [sec_]for_each_sym() and use them.

Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/59023e5886ab125aa30702e633be7732b1acaa7e.1681325924.git.jpoimboe@kernel.org
---
tools/objtool/check.c | 98 +++++++++++-----------------
tools/objtool/elf.c | 2 +-
tools/objtool/include/objtool/elf.h | 9 +++-
3 files changed, 51 insertions(+), 58 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 7d1a42b..9de3972 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -470,7 +470,7 @@ static int decode_instructions(struct objtool_file *file)

// printf("%s: last chunk used: %d\n", sec->name, (int)idx);

- list_for_each_entry(func, &sec->symbol_list, list) {
+ sec_for_each_sym(sec, func) {
if (func->type != STT_NOTYPE && func->type != STT_FUNC)
continue;

@@ -924,7 +924,7 @@ static int create_ibt_endbr_seal_sections(struct objtool_file *file)

static int create_cfi_sections(struct objtool_file *file)
{
- struct section *sec, *s;
+ struct section *sec;
struct symbol *sym;
unsigned int *loc;
int idx;
@@ -937,19 +937,14 @@ static int create_cfi_sections(struct objtool_file *file)
}

idx = 0;
- for_each_sec(file, s) {
- if (!s->text)
+ for_each_sym(file, sym) {
+ if (sym->type != STT_FUNC)
continue;

- list_for_each_entry(sym, &s->symbol_list, list) {
- if (sym->type != STT_FUNC)
- continue;
-
- if (strncmp(sym->name, "__cfi_", 6))
- continue;
+ if (strncmp(sym->name, "__cfi_", 6))
+ continue;

- idx++;
- }
+ idx++;
}

sec = elf_create_section(file->elf, ".cfi_sites", 0, sizeof(unsigned int), idx);
@@ -957,28 +952,23 @@ static int create_cfi_sections(struct objtool_file *file)
return -1;

idx = 0;
- for_each_sec(file, s) {
- if (!s->text)
+ for_each_sym(file, sym) {
+ if (sym->type != STT_FUNC)
continue;

- list_for_each_entry(sym, &s->symbol_list, list) {
- if (sym->type != STT_FUNC)
- continue;
-
- if (strncmp(sym->name, "__cfi_", 6))
- continue;
+ if (strncmp(sym->name, "__cfi_", 6))
+ continue;

- loc = (unsigned int *)sec->data->d_buf + idx;
- memset(loc, 0, sizeof(unsigned int));
+ loc = (unsigned int *)sec->data->d_buf + idx;
+ memset(loc, 0, sizeof(unsigned int));

- if (elf_add_reloc_to_insn(file->elf, sec,
- idx * sizeof(unsigned int),
- R_X86_64_PC32,
- s, sym->offset))
- return -1;
+ if (elf_add_reloc_to_insn(file->elf, sec,
+ idx * sizeof(unsigned int),
+ R_X86_64_PC32,
+ sym->sec, sym->offset))
+ return -1;

- idx++;
- }
+ idx++;
}

return 0;
@@ -2207,23 +2197,20 @@ static int add_func_jump_tables(struct objtool_file *file,
*/
static int add_jump_table_alts(struct objtool_file *file)
{
- struct section *sec;
struct symbol *func;
int ret;

if (!file->rodata)
return 0;

- for_each_sec(file, sec) {
- list_for_each_entry(func, &sec->symbol_list, list) {
- if (func->type != STT_FUNC)
- continue;
+ for_each_sym(file, func) {
+ if (func->type != STT_FUNC)
+ continue;

- mark_func_jump_tables(file, func);
- ret = add_func_jump_tables(file, func);
- if (ret)
- return ret;
- }
+ mark_func_jump_tables(file, func);
+ ret = add_func_jump_tables(file, func);
+ if (ret)
+ return ret;
}

return 0;
@@ -2535,30 +2522,27 @@ static bool is_profiling_func(const char *name)

static int classify_symbols(struct objtool_file *file)
{
- struct section *sec;
struct symbol *func;

- for_each_sec(file, sec) {
- list_for_each_entry(func, &sec->symbol_list, list) {
- if (func->bind != STB_GLOBAL)
- continue;
+ for_each_sym(file, func) {
+ if (func->bind != STB_GLOBAL)
+ continue;

- if (!strncmp(func->name, STATIC_CALL_TRAMP_PREFIX_STR,
- strlen(STATIC_CALL_TRAMP_PREFIX_STR)))
- func->static_call_tramp = true;
+ if (!strncmp(func->name, STATIC_CALL_TRAMP_PREFIX_STR,
+ strlen(STATIC_CALL_TRAMP_PREFIX_STR)))
+ func->static_call_tramp = true;

- if (arch_is_retpoline(func))
- func->retpoline_thunk = true;
+ if (arch_is_retpoline(func))
+ func->retpoline_thunk = true;

- if (arch_is_rethunk(func))
- func->return_thunk = true;
+ if (arch_is_rethunk(func))
+ func->return_thunk = true;

- if (arch_ftrace_match(func->name))
- func->fentry = true;
+ if (arch_ftrace_match(func->name))
+ func->fentry = true;

- if (is_profiling_func(func->name))
- func->profiling_func = true;
- }
+ if (is_profiling_func(func->name))
+ func->profiling_func = true;
}

return 0;
@@ -4213,7 +4197,7 @@ static int validate_section(struct objtool_file *file, struct section *sec)
struct symbol *func;
int warnings = 0;

- list_for_each_entry(func, &sec->symbol_list, list) {
+ sec_for_each_sym(sec, func) {
if (func->type != STT_FUNC)
continue;

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 6806ce0..500e929 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -474,7 +474,7 @@ static int read_symbols(struct elf *elf)

/* Create parent/child links for any cold subfunctions */
list_for_each_entry(sec, &elf->sections, list) {
- list_for_each_entry(sym, &sec->symbol_list, list) {
+ sec_for_each_sym(sec, sym) {
char pname[MAX_NAME_LEN + 1];
size_t pnamelen;
if (sym->type != STT_FUNC)
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index ad0024d..e1ca588 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -188,4 +188,13 @@ struct symbol *find_func_containing(struct section *sec, unsigned long offset);
#define for_each_sec(file, sec) \
list_for_each_entry(sec, &file->elf->sections, list)

+#define sec_for_each_sym(sec, sym) \
+ list_for_each_entry(sym, &sec->symbol_list, list)
+
+#define for_each_sym(file, sym) \
+ for (struct section *__sec, *__fake = (struct section *)1; \
+ __fake; __fake = NULL) \
+ for_each_sec(file, __sec) \
+ sec_for_each_sym(__sec, sym)
+
#endif /* _OBJTOOL_ELF_H */

2023-04-14 14:56:19

by tip-bot2 for Linus Torvalds

Subject: [tip: objtool/core] scripts/objdump-func: Support multiple functions

The following commit has been merged into the objtool/core branch of tip:

Commit-ID: 27d000d635ce48b579988e9b3240352a2a0306e0
Gitweb: https://git.kernel.org/tip/27d000d635ce48b579988e9b3240352a2a0306e0
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Wed, 12 Apr 2023 12:03:16 -07:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Fri, 14 Apr 2023 16:08:28 +02:00

scripts/objdump-func: Support multiple functions

Allow specifying multiple functions on the cmdline. Note this removes
the secret EXTRA_ARGS feature.

While at it, spread out the awk to make it more readable.

Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/0bf5f4f5978660985037b24c6db49b114374eb4d.1681325924.git.jpoimboe@kernel.org
---
scripts/objdump-func | 34 +++++++++++++++++++++++++---------
1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/scripts/objdump-func b/scripts/objdump-func
index 4eb463d..7b15b87 100755
--- a/scripts/objdump-func
+++ b/scripts/objdump-func
@@ -3,7 +3,7 @@
#
# Disassemble a single function.
#
-# usage: objdump-func <file> <func>
+# usage: objdump-func <file> <func> [<func> ...]

set -o errexit
set -o nounset
@@ -13,17 +13,33 @@ OBJDUMP="${CROSS_COMPILE:-}objdump"
command -v gawk >/dev/null 2>&1 || die "gawk isn't installed"

usage() {
- echo "usage: objdump-func <file> <func>" >&2
+ echo "usage: objdump-func <file> <func> [<func> ...]" >&2
exit 1
}

[[ $# -lt 2 ]] && usage

OBJ=$1; shift
-FUNC=$1; shift
-
-# Secret feature to allow adding extra objdump args at the end
-EXTRA_ARGS=$@
-
-# Note this also matches compiler-added suffixes like ".cold", etc
-${OBJDUMP} -wdr $EXTRA_ARGS $OBJ | gawk -M -v f=$FUNC '/^$/ { P=0; } $0 ~ "<" f "(\\..*)?>:" { P=1; O=strtonum("0x" $1); } { if (P) { o=strtonum("0x" $1); printf("%04x ", o-O); print $0; } }'
+FUNCS=("$@")
+
+${OBJDUMP} -wdr $OBJ | gawk -M -v _funcs="${FUNCS[*]}" '
+ BEGIN { split(_funcs, funcs); }
+ /^$/ { func_match=0; }
+ /<.*>:/ {
+ f = gensub(/.*<(.*)>:/, "\\1", 1);
+ for (i in funcs) {
+ # match compiler-added suffixes like ".cold", etc
+ if (f ~ "^" funcs[i] "(\\..*)?") {
+ func_match = 1;
+ base = strtonum("0x" $1);
+ break;
+ }
+ }
+ }
+ {
+ if (func_match) {
+ addr = strtonum("0x" $1);
+ printf("%04x ", addr - base);
+ print;
+ }
+ }'

2023-04-14 14:57:52

by tip-bot2 for Linus Torvalds

Subject: [tip: objtool/core] objtool: Add WARN_INSN()

The following commit has been merged into the objtool/core branch of tip:

Commit-ID: 246b2c85487a7bc5f6a09098e18a96506b1b55df
Gitweb: https://git.kernel.org/tip/246b2c85487a7bc5f6a09098e18a96506b1b55df
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Wed, 12 Apr 2023 12:03:17 -07:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Fri, 14 Apr 2023 16:08:28 +02:00

objtool: Add WARN_INSN()

It's easier to use and also gives easy access to the instruction's
containing function, which is useful for printing that function's
symbol. It will also be useful in the future for rate-limiting and
disassembly of warned functions.

Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/2eaa3155c90fba683d8723599f279c46025b75f3.1681325924.git.jpoimboe@kernel.org
---
tools/objtool/check.c | 171 +++++++++-----------------
tools/objtool/include/objtool/warn.h | 5 +-
tools/objtool/orc_gen.c | 9 +-
3 files changed, 70 insertions(+), 115 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 4c8ef81..7d1a42b 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1446,7 +1446,7 @@ static void annotate_call_site(struct objtool_file *file,

if (opts.mcount && sym->fentry) {
if (sibling)
- WARN_FUNC("Tail call to __fentry__ !?!?", insn->sec, insn->offset);
+ WARN_INSN(insn, "tail call to __fentry__ !?!?");
if (opts.mnop) {
if (reloc) {
reloc->type = R_NONE;
@@ -1648,9 +1648,8 @@ static int add_jump_destinations(struct objtool_file *file)
continue;
}

- WARN_FUNC("can't find jump dest instruction at %s+0x%lx",
- insn->sec, insn->offset, dest_sec->name,
- dest_off);
+ WARN_INSN(insn, "can't find jump dest instruction at %s+0x%lx",
+ dest_sec->name, dest_off);
return -1;
}

@@ -1733,13 +1732,12 @@ static int add_call_destinations(struct objtool_file *file)
continue;

if (!insn_call_dest(insn)) {
- WARN_FUNC("unannotated intra-function call", insn->sec, insn->offset);
+ WARN_INSN(insn, "unannotated intra-function call");
return -1;
}

if (insn_func(insn) && insn_call_dest(insn)->type != STT_FUNC) {
- WARN_FUNC("unsupported call to non-function",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unsupported call to non-function");
return -1;
}

@@ -1747,10 +1745,8 @@ static int add_call_destinations(struct objtool_file *file)
dest_off = arch_dest_reloc_offset(reloc->addend);
dest = find_call_destination(reloc->sym->sec, dest_off);
if (!dest) {
- WARN_FUNC("can't find call dest symbol at %s+0x%lx",
- insn->sec, insn->offset,
- reloc->sym->sec->name,
- dest_off);
+ WARN_INSN(insn, "can't find call dest symbol at %s+0x%lx",
+ reloc->sym->sec->name, dest_off);
return -1;
}

@@ -1810,8 +1806,7 @@ static int handle_group_alt(struct objtool_file *file,
} else {
if (orig_alt_group->last_insn->offset + orig_alt_group->last_insn->len -
orig_alt_group->first_insn->offset != special_alt->orig_len) {
- WARN_FUNC("weirdly overlapping alternative! %ld != %d",
- orig_insn->sec, orig_insn->offset,
+ WARN_INSN(orig_insn, "weirdly overlapping alternative! %ld != %d",
orig_alt_group->last_insn->offset +
orig_alt_group->last_insn->len -
orig_alt_group->first_insn->offset,
@@ -1880,8 +1875,7 @@ static int handle_group_alt(struct objtool_file *file,
if (alt_reloc && arch_pc_relative_reloc(alt_reloc) &&
!arch_support_alt_relocation(special_alt, insn, alt_reloc)) {

- WARN_FUNC("unsupported relocation in alternatives section",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unsupported relocation in alternatives section");
return -1;
}

@@ -1895,8 +1889,7 @@ static int handle_group_alt(struct objtool_file *file,
if (dest_off == special_alt->new_off + special_alt->new_len) {
insn->jump_dest = next_insn_same_sec(file, orig_alt_group->last_insn);
if (!insn->jump_dest) {
- WARN_FUNC("can't find alternative jump destination",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "can't find alternative jump destination");
return -1;
}
}
@@ -1930,8 +1923,7 @@ static int handle_jump_alt(struct objtool_file *file,
if (orig_insn->type != INSN_JUMP_UNCONDITIONAL &&
orig_insn->type != INSN_NOP) {

- WARN_FUNC("unsupported instruction at jump label",
- orig_insn->sec, orig_insn->offset);
+ WARN_INSN(orig_insn, "unsupported instruction at jump label");
return -1;
}

@@ -2010,8 +2002,7 @@ static int add_special_section_alts(struct objtool_file *file)

if (special_alt->group) {
if (!special_alt->orig_len) {
- WARN_FUNC("empty alternative entry",
- orig_insn->sec, orig_insn->offset);
+ WARN_INSN(orig_insn, "empty alternative entry");
continue;
}

@@ -2102,8 +2093,7 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
}

if (!prev_offset) {
- WARN_FUNC("can't find switch jump table",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "can't find switch jump table");
return -1;
}

@@ -2307,8 +2297,7 @@ static int read_unwind_hints(struct objtool_file *file)

if (sym && sym->bind == STB_GLOBAL) {
if (opts.ibt && insn->type != INSN_ENDBR && !insn->noendbr) {
- WARN_FUNC("UNWIND_HINT_IRET_REGS without ENDBR",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "UNWIND_HINT_IRET_REGS without ENDBR");
}
}
}
@@ -2322,8 +2311,7 @@ static int read_unwind_hints(struct objtool_file *file)
cfi = *(insn->cfi);

if (arch_decode_hint_reg(hint->sp_reg, &cfi.cfa.base)) {
- WARN_FUNC("unsupported unwind_hint sp base reg %d",
- insn->sec, insn->offset, hint->sp_reg);
+ WARN_INSN(insn, "unsupported unwind_hint sp base reg %d", hint->sp_reg);
return -1;
}

@@ -2386,8 +2374,7 @@ static int read_retpoline_hints(struct objtool_file *file)
insn->type != INSN_CALL_DYNAMIC &&
insn->type != INSN_RETURN &&
insn->type != INSN_NOP) {
- WARN_FUNC("retpoline_safe hint not an indirect jump/call/ret/nop",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "retpoline_safe hint not an indirect jump/call/ret/nop");
return -1;
}

@@ -2498,8 +2485,7 @@ static int read_intra_function_calls(struct objtool_file *file)
}

if (insn->type != INSN_CALL) {
- WARN_FUNC("intra_function_call not a direct call",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "intra_function_call not a direct call");
return -1;
}

@@ -2513,8 +2499,7 @@ static int read_intra_function_calls(struct objtool_file *file)
dest_off = arch_jump_destination(insn);
insn->jump_dest = find_insn(file, insn->sec, dest_off);
if (!insn->jump_dest) {
- WARN_FUNC("can't find call dest at %s+0x%lx",
- insn->sec, insn->offset,
+ WARN_INSN(insn, "can't find call dest at %s+0x%lx",
insn->sec->name, dest_off);
return -1;
}
@@ -2855,7 +2840,7 @@ static int update_cfi_state(struct instruction *insn,
/* stack operations don't make sense with an undefined CFA */
if (cfa->base == CFI_UNDEFINED) {
if (insn_func(insn)) {
- WARN_FUNC("undefined stack state", insn->sec, insn->offset);
+ WARN_INSN(insn, "undefined stack state");
return -1;
}
return 0;
@@ -3038,8 +3023,7 @@ static int update_cfi_state(struct instruction *insn,
}

if (op->dest.reg == cfi->cfa.base && !(next_insn && next_insn->hint)) {
- WARN_FUNC("unsupported stack register modification",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unsupported stack register modification");
return -1;
}

@@ -3049,8 +3033,7 @@ static int update_cfi_state(struct instruction *insn,
if (op->dest.reg != CFI_SP ||
(cfi->drap_reg != CFI_UNDEFINED && cfa->base != CFI_SP) ||
(cfi->drap_reg == CFI_UNDEFINED && cfa->base != CFI_BP)) {
- WARN_FUNC("unsupported stack pointer realignment",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unsupported stack pointer realignment");
return -1;
}

@@ -3145,8 +3128,7 @@ static int update_cfi_state(struct instruction *insn,
break;

default:
- WARN_FUNC("unknown stack-related instruction",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unknown stack-related instruction");
return -1;
}

@@ -3235,8 +3217,7 @@ static int update_cfi_state(struct instruction *insn,

case OP_DEST_MEM:
if (op->src.type != OP_SRC_POP && op->src.type != OP_SRC_POPF) {
- WARN_FUNC("unknown stack-related memory operation",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unknown stack-related memory operation");
return -1;
}

@@ -3248,8 +3229,7 @@ static int update_cfi_state(struct instruction *insn,
break;

default:
- WARN_FUNC("unknown stack-related instruction",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unknown stack-related instruction");
return -1;
}

@@ -3288,8 +3268,7 @@ static int propagate_alt_cfi(struct objtool_file *file, struct instruction *insn
struct alt_group *orig_group = insn->alt_group->orig_group ?: insn->alt_group;
struct instruction *orig = orig_group->first_insn;
char *where = offstr(insn->sec, insn->offset);
- WARN_FUNC("stack layout conflict in alternatives: %s",
- orig->sec, orig->offset, where);
+ WARN_INSN(orig, "stack layout conflict in alternatives: %s", where);
free(where);
return -1;
}
@@ -3316,8 +3295,7 @@ static int handle_insn_ops(struct instruction *insn,
if (!state->uaccess_stack) {
state->uaccess_stack = 1;
} else if (state->uaccess_stack >> 31) {
- WARN_FUNC("PUSHF stack exhausted",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "PUSHF stack exhausted");
return 1;
}
state->uaccess_stack <<= 1;
@@ -3349,8 +3327,7 @@ static bool insn_cfi_match(struct instruction *insn, struct cfi_state *cfi2)

if (memcmp(&cfi1->cfa, &cfi2->cfa, sizeof(cfi1->cfa))) {

- WARN_FUNC("stack state mismatch: cfa1=%d%+d cfa2=%d%+d",
- insn->sec, insn->offset,
+ WARN_INSN(insn, "stack state mismatch: cfa1=%d%+d cfa2=%d%+d",
cfi1->cfa.base, cfi1->cfa.offset,
cfi2->cfa.base, cfi2->cfa.offset);

@@ -3360,8 +3337,7 @@ static bool insn_cfi_match(struct instruction *insn, struct cfi_state *cfi2)
sizeof(struct cfi_reg)))
continue;

- WARN_FUNC("stack state mismatch: reg1[%d]=%d%+d reg2[%d]=%d%+d",
- insn->sec, insn->offset,
+ WARN_INSN(insn, "stack state mismatch: reg1[%d]=%d%+d reg2[%d]=%d%+d",
i, cfi1->regs[i].base, cfi1->regs[i].offset,
i, cfi2->regs[i].base, cfi2->regs[i].offset);
break;
@@ -3369,15 +3345,14 @@ static bool insn_cfi_match(struct instruction *insn, struct cfi_state *cfi2)

} else if (cfi1->type != cfi2->type) {

- WARN_FUNC("stack state mismatch: type1=%d type2=%d",
- insn->sec, insn->offset, cfi1->type, cfi2->type);
+ WARN_INSN(insn, "stack state mismatch: type1=%d type2=%d",
+ cfi1->type, cfi2->type);

} else if (cfi1->drap != cfi2->drap ||
(cfi1->drap && cfi1->drap_reg != cfi2->drap_reg) ||
(cfi1->drap && cfi1->drap_offset != cfi2->drap_offset)) {

- WARN_FUNC("stack state mismatch: drap1=%d(%d,%d) drap2=%d(%d,%d)",
- insn->sec, insn->offset,
+ WARN_INSN(insn, "stack state mismatch: drap1=%d(%d,%d) drap2=%d(%d,%d)",
cfi1->drap, cfi1->drap_reg, cfi1->drap_offset,
cfi2->drap, cfi2->drap_reg, cfi2->drap_offset);

@@ -3485,20 +3460,17 @@ static int validate_call(struct objtool_file *file,
{
if (state->noinstr && state->instr <= 0 &&
!noinstr_call_dest(file, insn, insn_call_dest(insn))) {
- WARN_FUNC("call to %s() leaves .noinstr.text section",
- insn->sec, insn->offset, call_dest_name(insn));
+ WARN_INSN(insn, "call to %s() leaves .noinstr.text section", call_dest_name(insn));
return 1;
}

if (state->uaccess && !func_uaccess_safe(insn_call_dest(insn))) {
- WARN_FUNC("call to %s() with UACCESS enabled",
- insn->sec, insn->offset, call_dest_name(insn));
+ WARN_INSN(insn, "call to %s() with UACCESS enabled", call_dest_name(insn));
return 1;
}

if (state->df) {
- WARN_FUNC("call to %s() with DF set",
- insn->sec, insn->offset, call_dest_name(insn));
+ WARN_INSN(insn, "call to %s() with DF set", call_dest_name(insn));
return 1;
}

@@ -3510,8 +3482,7 @@ static int validate_sibling_call(struct objtool_file *file,
struct insn_state *state)
{
if (insn_func(insn) && has_modified_stack_frame(insn, state)) {
- WARN_FUNC("sibling call from callable instruction with modified stack frame",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "sibling call from callable instruction with modified stack frame");
return 1;
}

@@ -3521,38 +3492,32 @@ static int validate_sibling_call(struct objtool_file *file,
static int validate_return(struct symbol *func, struct instruction *insn, struct insn_state *state)
{
if (state->noinstr && state->instr > 0) {
- WARN_FUNC("return with instrumentation enabled",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "return with instrumentation enabled");
return 1;
}

if (state->uaccess && !func_uaccess_safe(func)) {
- WARN_FUNC("return with UACCESS enabled",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "return with UACCESS enabled");
return 1;
}

if (!state->uaccess && func_uaccess_safe(func)) {
- WARN_FUNC("return with UACCESS disabled from a UACCESS-safe function",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "return with UACCESS disabled from a UACCESS-safe function");
return 1;
}

if (state->df) {
- WARN_FUNC("return with DF set",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "return with DF set");
return 1;
}

if (func && has_modified_stack_frame(insn, state)) {
- WARN_FUNC("return with modified stack frame",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "return with modified stack frame");
return 1;
}

if (state->cfi.bp_scratch) {
- WARN_FUNC("BP used as a scratch register",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "BP used as a scratch register");
return 1;
}

@@ -3624,8 +3589,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
}

if (func && insn->ignore) {
- WARN_FUNC("BUG: why am I validating an ignored function?",
- sec, insn->offset);
+ WARN_INSN(insn, "BUG: why am I validating an ignored function?");
return 1;
}

@@ -3658,14 +3622,12 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
}

if (!save_insn) {
- WARN_FUNC("no corresponding CFI save for CFI restore",
- sec, insn->offset);
+ WARN_INSN(insn, "no corresponding CFI save for CFI restore");
return 1;
}

if (!save_insn->visited) {
- WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
- sec, insn->offset);
+ WARN_INSN(insn, "objtool isn't smart enough to handle this CFI save/restore combo");
return 1;
}

@@ -3725,8 +3687,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,

if (opts.stackval && func && !is_fentry_call(insn) &&
!has_valid_stack_frame(&state)) {
- WARN_FUNC("call without frame pointer save/setup",
- sec, insn->offset);
+ WARN_INSN(insn, "call without frame pointer save/setup");
return 1;
}

@@ -3772,15 +3733,14 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,

case INSN_CONTEXT_SWITCH:
if (func && (!next_insn || !next_insn->hint)) {
- WARN_FUNC("unsupported instruction in callable function",
- sec, insn->offset);
+ WARN_INSN(insn, "unsupported instruction in callable function");
return 1;
}
return 0;

case INSN_STAC:
if (state.uaccess) {
- WARN_FUNC("recursive UACCESS enable", sec, insn->offset);
+ WARN_INSN(insn, "recursive UACCESS enable");
return 1;
}

@@ -3789,12 +3749,12 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,

case INSN_CLAC:
if (!state.uaccess && func) {
- WARN_FUNC("redundant UACCESS disable", sec, insn->offset);
+ WARN_INSN(insn, "redundant UACCESS disable");
return 1;
}

if (func_uaccess_safe(func) && !state.uaccess_stack) {
- WARN_FUNC("UACCESS-safe disables UACCESS", sec, insn->offset);
+ WARN_INSN(insn, "UACCESS-safe disables UACCESS");
return 1;
}

@@ -3803,7 +3763,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,

case INSN_STD:
if (state.df) {
- WARN_FUNC("recursive STD", sec, insn->offset);
+ WARN_INSN(insn, "recursive STD");
return 1;
}

@@ -3812,7 +3772,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,

case INSN_CLD:
if (!state.df && func) {
- WARN_FUNC("redundant CLD", sec, insn->offset);
+ WARN_INSN(insn, "redundant CLD");
return 1;
}

@@ -3920,15 +3880,14 @@ static int validate_unret(struct objtool_file *file, struct instruction *insn)
case INSN_CALL_DYNAMIC:
case INSN_JUMP_DYNAMIC:
case INSN_JUMP_DYNAMIC_CONDITIONAL:
- WARN_FUNC("early indirect call", insn->sec, insn->offset);
+ WARN_INSN(insn, "early indirect call");
return 1;

case INSN_JUMP_UNCONDITIONAL:
case INSN_JUMP_CONDITIONAL:
if (!is_sibling_call(insn)) {
if (!insn->jump_dest) {
- WARN_FUNC("unresolved jump target after linking?!?",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "unresolved jump target after linking?!?");
return -1;
}
ret = validate_unret(file, insn->jump_dest);
@@ -3969,7 +3928,7 @@ static int validate_unret(struct objtool_file *file, struct instruction *insn)
return 0;

case INSN_RETURN:
- WARN_FUNC("RET before UNTRAIN", insn->sec, insn->offset);
+ WARN_INSN(insn, "RET before UNTRAIN");
return 1;

case INSN_NOP:
@@ -3982,7 +3941,7 @@ static int validate_unret(struct objtool_file *file, struct instruction *insn)
}

if (!next) {
- WARN_FUNC("teh end!", insn->sec, insn->offset);
+ WARN_INSN(insn, "teh end!");
return -1;
}
insn = next;
@@ -4006,7 +3965,7 @@ static int validate_unrets(struct objtool_file *file)

ret = validate_unret(file, insn);
if (ret < 0) {
- WARN_FUNC("Failed UNRET validation", insn->sec, insn->offset);
+ WARN_INSN(insn, "Failed UNRET validation");
return ret;
}
warnings += ret;
@@ -4034,13 +3993,11 @@ static int validate_retpoline(struct objtool_file *file)

if (insn->type == INSN_RETURN) {
if (opts.rethunk) {
- WARN_FUNC("'naked' return found in RETHUNK build",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "'naked' return found in RETHUNK build");
} else
continue;
} else {
- WARN_FUNC("indirect %s found in RETPOLINE build",
- insn->sec, insn->offset,
+ WARN_INSN(insn, "indirect %s found in RETPOLINE build",
insn->type == INSN_JUMP_DYNAMIC ? "jump" : "call");
}

@@ -4419,9 +4376,7 @@ static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn
if (noendbr_range(file, dest))
continue;

- WARN_FUNC("relocation to !ENDBR: %s",
- insn->sec, insn->offset,
- offstr(dest->sec, dest->offset));
+ WARN_INSN(insn, "relocation to !ENDBR: %s", offstr(dest->sec, dest->offset));

warnings++;
}
@@ -4523,16 +4478,14 @@ static int validate_sls(struct objtool_file *file)
switch (insn->type) {
case INSN_RETURN:
if (!next_insn || next_insn->type != INSN_TRAP) {
- WARN_FUNC("missing int3 after ret",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "missing int3 after ret");
warnings++;
}

break;
case INSN_JUMP_DYNAMIC:
if (!next_insn || next_insn->type != INSN_TRAP) {
- WARN_FUNC("missing int3 after indirect jump",
- insn->sec, insn->offset);
+ WARN_INSN(insn, "missing int3 after indirect jump");
warnings++;
}
break;
@@ -4555,7 +4508,7 @@ static int validate_reachable_instructions(struct objtool_file *file)
if (insn->visited || ignore_unreachable_insn(file, insn))
continue;

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

diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index a3e79ae..b1c920d 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -53,6 +53,11 @@ static inline char *offstr(struct section *sec, unsigned long offset)
free(_str); \
})

+#define WARN_INSN(insn, format, ...) \
+({ \
+ WARN_FUNC(format, insn->sec, insn->offset, ##__VA_ARGS__); \
+})
+
#define BT_FUNC(format, insn, ...) \
({ \
struct instruction *_insn = (insn); \
diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index b327f9c..48efd1e 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -47,8 +47,7 @@ static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi,
orc->type = ORC_TYPE_REGS_PARTIAL;
break;
default:
- WARN_FUNC("unknown unwind hint type %d",
- insn->sec, insn->offset, cfi->type);
+ WARN_INSN(insn, "unknown unwind hint type %d", cfi->type);
return -1;
}

@@ -80,8 +79,7 @@ static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi,
orc->sp_reg = ORC_REG_DX;
break;
default:
- WARN_FUNC("unknown CFA base reg %d",
- insn->sec, insn->offset, cfi->cfa.base);
+ WARN_INSN(insn, "unknown CFA base reg %d", cfi->cfa.base);
return -1;
}

@@ -96,8 +94,7 @@ static int init_orc_entry(struct orc_entry *orc, struct cfi_state *cfi,
orc->bp_reg = ORC_REG_BP;
break;
default:
- WARN_FUNC("unknown BP base reg %d",
- insn->sec, insn->offset, bp->base);
+ WARN_INSN(insn, "unknown BP base reg %d", bp->base);
return -1;
}

2023-04-14 21:20:48

by Nick Desaulniers

Subject: Re: [PATCH v2 9/9] objtool: Ignore exc_double_fault() __noreturn warnings

On Thu, Apr 13, 2023 at 2:24 AM Peter Zijlstra <[email protected]> wrote:
>
> On Wed, Apr 12, 2023 at 12:03:24PM -0700, Josh Poimboeuf wrote:
> > + * - have compiler communicate __noreturn functions somehow

Could probably stuff these in some kind of "noreturn function symbol
table"/custom elf section; DWARF tags are nice but we don't always
want debug info. (I should ask colleagues if there's anything like
this in DWARF already, perhaps for DWARFv6).

>
> This, we're going to have to do that -- because keeping that (ever
> growing) list in sync is going to be a pain in the backside.
>


--
Thanks,
~Nick Desaulniers