* [PATCH v2 0/9] objtool: warning improvements
@ 2023-04-12 19:03 Josh Poimboeuf
2023-04-12 19:03 ` [PATCH v2 1/9] scripts/objdump-func: Support multiple functions Josh Poimboeuf
` (8 more replies)
0 siblings, 9 replies; 28+ messages in thread
From: Josh Poimboeuf @ 2023-04-12 19:03 UTC (permalink / raw)
To: x86; +Cc: linux-kernel, Peter Zijlstra, Miroslav Benes
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
^ permalink raw reply [flat|nested] 28+ messages in thread* [PATCH v2 1/9] scripts/objdump-func: Support multiple functions 2023-04-12 19:03 [PATCH v2 0/9] objtool: warning improvements Josh Poimboeuf @ 2023-04-12 19:03 ` Josh Poimboeuf 2023-04-14 14:47 ` [tip: objtool/core] " tip-bot2 for Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 2/9] objtool: Add WARN_INSN() Josh Poimboeuf ` (7 subsequent siblings) 8 siblings, 1 reply; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-12 19:03 UTC (permalink / raw) To: x86; +Cc: linux-kernel, Peter Zijlstra, Miroslav Benes 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 <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 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [tip: objtool/core] scripts/objdump-func: Support multiple functions 2023-04-12 19:03 ` [PATCH v2 1/9] scripts/objdump-func: Support multiple functions Josh Poimboeuf @ 2023-04-14 14:47 ` tip-bot2 for Josh Poimboeuf 0 siblings, 0 replies; 28+ messages in thread From: tip-bot2 for Josh Poimboeuf @ 2023-04-14 14:47 UTC (permalink / raw) To: linux-tip-commits Cc: Josh Poimboeuf, Peter Zijlstra (Intel), x86, linux-kernel 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 <jpoimboe@kernel.org> AuthorDate: Wed, 12 Apr 2023 12:03:16 -07:00 Committer: Peter Zijlstra <peterz@infradead.org> 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 <jpoimboe@kernel.org> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> 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; + } + }' ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 2/9] objtool: Add WARN_INSN() 2023-04-12 19:03 [PATCH v2 0/9] objtool: warning improvements Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 1/9] scripts/objdump-func: Support multiple functions Josh Poimboeuf @ 2023-04-12 19:03 ` Josh Poimboeuf 2023-04-14 14:47 ` [tip: objtool/core] " tip-bot2 for Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 3/9] objtool: Limit unreachable warnings to once per function Josh Poimboeuf ` (6 subsequent siblings) 8 siblings, 1 reply; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-12 19:03 UTC (permalink / raw) To: x86; +Cc: linux-kernel, Peter Zijlstra, Miroslav Benes 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 <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 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [tip: objtool/core] objtool: Add WARN_INSN() 2023-04-12 19:03 ` [PATCH v2 2/9] objtool: Add WARN_INSN() Josh Poimboeuf @ 2023-04-14 14:47 ` tip-bot2 for Josh Poimboeuf 0 siblings, 0 replies; 28+ messages in thread From: tip-bot2 for Josh Poimboeuf @ 2023-04-14 14:47 UTC (permalink / raw) To: linux-tip-commits Cc: Josh Poimboeuf, Peter Zijlstra (Intel), x86, linux-kernel 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 <jpoimboe@kernel.org> AuthorDate: Wed, 12 Apr 2023 12:03:17 -07:00 Committer: Peter Zijlstra <peterz@infradead.org> 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 <jpoimboe@kernel.org> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> 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; } ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 3/9] objtool: Limit unreachable warnings to once per function 2023-04-12 19:03 [PATCH v2 0/9] objtool: warning improvements Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 1/9] scripts/objdump-func: Support multiple functions Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 2/9] objtool: Add WARN_INSN() Josh Poimboeuf @ 2023-04-12 19:03 ` Josh Poimboeuf 2023-04-13 8:01 ` Peter Zijlstra 2023-04-12 19:03 ` [PATCH v2 4/9] objtool: Add symbol iteration helpers Josh Poimboeuf ` (5 subsequent siblings) 8 siblings, 1 reply; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-12 19:03 UTC (permalink / raw) To: x86; +Cc: linux-kernel, Peter Zijlstra, Miroslav Benes 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 <jpoimboe@kernel.org> --- 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/9] objtool: Limit unreachable warnings to once per function 2023-04-12 19:03 ` [PATCH v2 3/9] objtool: Limit unreachable warnings to once per function Josh Poimboeuf @ 2023-04-13 8:01 ` Peter Zijlstra 2023-04-13 15:03 ` Josh Poimboeuf 0 siblings, 1 reply; 28+ messages in thread From: Peter Zijlstra @ 2023-04-13 8:01 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes 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 ? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/9] objtool: Limit unreachable warnings to once per function 2023-04-13 8:01 ` Peter Zijlstra @ 2023-04-13 15:03 ` Josh Poimboeuf 0 siblings, 0 replies; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-13 15:03 UTC (permalink / raw) To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 4/9] objtool: Add symbol iteration helpers 2023-04-12 19:03 [PATCH v2 0/9] objtool: warning improvements Josh Poimboeuf ` (2 preceding siblings ...) 2023-04-12 19:03 ` [PATCH v2 3/9] objtool: Limit unreachable warnings to once per function Josh Poimboeuf @ 2023-04-12 19:03 ` Josh Poimboeuf 2023-04-14 14:47 ` [tip: objtool/core] " tip-bot2 for Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions Josh Poimboeuf ` (4 subsequent siblings) 8 siblings, 1 reply; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-12 19:03 UTC (permalink / raw) To: x86; +Cc: linux-kernel, Peter Zijlstra, Miroslav Benes Add [sec_]for_each_sym() and use them. Signed-off-by: Josh Poimboeuf <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 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [tip: objtool/core] objtool: Add symbol iteration helpers 2023-04-12 19:03 ` [PATCH v2 4/9] objtool: Add symbol iteration helpers Josh Poimboeuf @ 2023-04-14 14:47 ` tip-bot2 for Josh Poimboeuf 0 siblings, 0 replies; 28+ messages in thread From: tip-bot2 for Josh Poimboeuf @ 2023-04-14 14:47 UTC (permalink / raw) To: linux-tip-commits Cc: Josh Poimboeuf, Peter Zijlstra (Intel), x86, linux-kernel 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 <jpoimboe@kernel.org> AuthorDate: Wed, 12 Apr 2023 12:03:19 -07:00 Committer: Peter Zijlstra <peterz@infradead.org> 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 <jpoimboe@kernel.org> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> 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 */ ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions 2023-04-12 19:03 [PATCH v2 0/9] objtool: warning improvements Josh Poimboeuf ` (3 preceding siblings ...) 2023-04-12 19:03 ` [PATCH v2 4/9] objtool: Add symbol iteration helpers Josh Poimboeuf @ 2023-04-12 19:03 ` Josh Poimboeuf 2023-04-13 8:05 ` Peter Zijlstra 2023-04-13 8:08 ` Peter Zijlstra 2023-04-12 19:03 ` [PATCH v2 6/9] objtool: Include backtrace in verbose mode Josh Poimboeuf ` (3 subsequent siblings) 8 siblings, 2 replies; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-12 19:03 UTC (permalink / raw) To: x86; +Cc: linux-kernel, Peter Zijlstra, Miroslav Benes When a warning is associated with a function, add an option to disassemble that function. This makes it easier for reporters to submit the information needed to diagnose objtool warnings. Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- tools/objtool/Documentation/objtool.txt | 5 ++ tools/objtool/builtin-check.c | 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions 2023-04-12 19:03 ` [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions Josh Poimboeuf @ 2023-04-13 8:05 ` Peter Zijlstra 2023-04-13 15:05 ` Josh Poimboeuf 2023-04-13 8:08 ` Peter Zijlstra 1 sibling, 1 reply; 28+ messages in thread From: Peter Zijlstra @ 2023-04-13 8:05 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes 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 <jpoimboe@kernel.org> > --- > 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions 2023-04-13 8:05 ` Peter Zijlstra @ 2023-04-13 15:05 ` Josh Poimboeuf 0 siblings, 0 replies; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-13 15:05 UTC (permalink / raw) To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions 2023-04-12 19:03 ` [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions Josh Poimboeuf 2023-04-13 8:05 ` Peter Zijlstra @ 2023-04-13 8:08 ` Peter Zijlstra 2023-04-13 14:03 ` Miroslav Benes 1 sibling, 1 reply; 28+ messages in thread From: Peter Zijlstra @ 2023-04-13 8:08 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes 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? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions 2023-04-13 8:08 ` Peter Zijlstra @ 2023-04-13 14:03 ` Miroslav Benes 2023-04-13 15:09 ` Josh Poimboeuf 0 siblings, 1 reply; 28+ messages in thread From: Miroslav Benes @ 2023-04-13 14:03 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Josh Poimboeuf, x86, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions 2023-04-13 14:03 ` Miroslav Benes @ 2023-04-13 15:09 ` Josh Poimboeuf 0 siblings, 0 replies; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-13 15:09 UTC (permalink / raw) To: Miroslav Benes; +Cc: Peter Zijlstra, x86, linux-kernel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 6/9] objtool: Include backtrace in verbose mode 2023-04-12 19:03 [PATCH v2 0/9] objtool: warning improvements Josh Poimboeuf ` (4 preceding siblings ...) 2023-04-12 19:03 ` [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions Josh Poimboeuf @ 2023-04-12 19:03 ` Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 7/9] objtool: Remove superfluous dead_end_function() check Josh Poimboeuf ` (2 subsequent siblings) 8 siblings, 0 replies; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-12 19:03 UTC (permalink / raw) To: x86; +Cc: linux-kernel, Peter Zijlstra, Miroslav Benes Include backtrace in verbose mode. This makes it easy to gather all the information needed for diagnosing objtool warnings. Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- tools/objtool/Documentation/objtool.txt | 4 ++-- tools/objtool/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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 7/9] objtool: Remove superfluous dead_end_function() check 2023-04-12 19:03 [PATCH v2 0/9] objtool: warning improvements Josh Poimboeuf ` (5 preceding siblings ...) 2023-04-12 19:03 ` [PATCH v2 6/9] objtool: Include backtrace in verbose mode Josh Poimboeuf @ 2023-04-12 19:03 ` Josh Poimboeuf 2023-04-14 14:47 ` [tip: objtool/core] " tip-bot2 for Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 8/9] objtool: Detect missing __noreturn annotations Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 9/9] objtool: Ignore exc_double_fault() __noreturn warnings Josh Poimboeuf 8 siblings, 1 reply; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-12 19:03 UTC (permalink / raw) To: x86; +Cc: linux-kernel, Peter Zijlstra, Miroslav Benes annotate_call_site() already sets 'insn->dead_end' for calls to dead end functions. Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- tools/objtool/check.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [tip: objtool/core] objtool: Remove superfluous dead_end_function() check 2023-04-12 19:03 ` [PATCH v2 7/9] objtool: Remove superfluous dead_end_function() check Josh Poimboeuf @ 2023-04-14 14:47 ` tip-bot2 for Josh Poimboeuf 0 siblings, 0 replies; 28+ messages in thread From: tip-bot2 for Josh Poimboeuf @ 2023-04-14 14:47 UTC (permalink / raw) To: linux-tip-commits Cc: Josh Poimboeuf, Peter Zijlstra (Intel), x86, linux-kernel 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 <jpoimboe@kernel.org> AuthorDate: Wed, 12 Apr 2023 12:03:22 -07:00 Committer: Peter Zijlstra <peterz@infradead.org> 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 <jpoimboe@kernel.org> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> 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))) ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 8/9] objtool: Detect missing __noreturn annotations 2023-04-12 19:03 [PATCH v2 0/9] objtool: warning improvements Josh Poimboeuf ` (6 preceding siblings ...) 2023-04-12 19:03 ` [PATCH v2 7/9] objtool: Remove superfluous dead_end_function() check Josh Poimboeuf @ 2023-04-12 19:03 ` Josh Poimboeuf 2023-04-13 8:48 ` Peter Zijlstra 2023-04-13 14:19 ` Miroslav Benes 2023-04-12 19:03 ` [PATCH v2 9/9] objtool: Ignore exc_double_fault() __noreturn warnings Josh Poimboeuf 8 siblings, 2 replies; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-12 19:03 UTC (permalink / raw) To: x86; +Cc: linux-kernel, Peter Zijlstra, Miroslav Benes Most "unreachable instruction" warnings these days seem to actually be the result of a missing __noreturn annotation. Add an explicit check for that. Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- tools/objtool/Documentation/objtool.txt | 6 ++++++ tools/objtool/check.c | 14 +++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt index 5a69c207a10e..2cd1fa16ed08 100644 --- a/tools/objtool/Documentation/objtool.txt +++ b/tools/objtool/Documentation/objtool.txt @@ -303,6 +303,12 @@ the objtool maintainers. If it's not actually in a callable function (e.g. kernel entry code), change ENDPROC to END. +3. file.o: warning: objtool: foo+0x48c: bar() is missing a __noreturn annotation + + The call from foo() to bar() doesn't return, but bar() is missing the + __noreturn annotation. NOTE: In addition to adding the __noreturn + annotation, the function name also needs to be added to + 'global_noreturns' in tools/objtool/check.c. 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 8/9] objtool: Detect missing __noreturn annotations 2023-04-12 19:03 ` [PATCH v2 8/9] objtool: Detect missing __noreturn annotations Josh Poimboeuf @ 2023-04-13 8:48 ` Peter Zijlstra 2023-04-13 15:19 ` Josh Poimboeuf 2023-04-13 14:19 ` Miroslav Benes 1 sibling, 1 reply; 28+ messages in thread From: Peter Zijlstra @ 2023-04-13 8:48 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes On Wed, Apr 12, 2023 at 12:03:23PM -0700, Josh Poimboeuf wrote: > Most "unreachable instruction" warnings these days seem to actually be > the result of a missing __noreturn annotation. Add an explicit check > for that. > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > --- > tools/objtool/Documentation/objtool.txt | 6 ++++++ > tools/objtool/check.c | 14 +++++++++++++- > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt > index 5a69c207a10e..2cd1fa16ed08 100644 > --- a/tools/objtool/Documentation/objtool.txt > +++ b/tools/objtool/Documentation/objtool.txt > @@ -303,6 +303,12 @@ the objtool maintainers. > If it's not actually in a callable function (e.g. kernel entry code), > change ENDPROC to END. > > +3. file.o: warning: objtool: foo+0x48c: bar() is missing a __noreturn annotation > + > + The call from foo() to bar() doesn't return, but bar() is missing the > + __noreturn annotation. NOTE: In addition to adding the __noreturn > + annotation, the function name also needs to be added to > + 'global_noreturns' in tools/objtool/check.c. Do we want something like the below (except perhaps less horrible) ? --- tools/objtool/Makefile | 1 + tools/objtool/check.c | 27 +-------------------------- tools/objtool/noreturns | 26 ++++++++++++++++++++++++++ tools/objtool/noreturns.sh | 7 +++++++ 4 files changed, 35 insertions(+), 26 deletions(-) diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile index 83b100c1e7f6..50b6cd241571 100644 --- a/tools/objtool/Makefile +++ b/tools/objtool/Makefile @@ -63,6 +63,7 @@ include $(srctree)/tools/build/Makefile.include $(OBJTOOL_IN): fixdep $(LIBSUBCMD) FORCE $(Q)$(CONFIG_SHELL) ./sync-check.sh + $(Q)$(CONFIG_SHELL) ./noreturns.sh $(Q)$(MAKE) $(build)=objtool $(HOST_OVERRIDES) CFLAGS="$(OBJTOOL_CFLAGS)" \ LDFLAGS="$(OBJTOOL_LDFLAGS)" diff --git a/tools/objtool/check.c b/tools/objtool/check.c index f937be1afe65..f558730c27b6 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -197,32 +197,7 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func, * attribute isn't provided in ELF data. Keep 'em sorted. */ static const char * const global_noreturns[] = { - "__invalid_creds", - "__module_put_and_kthread_exit", - "__reiserfs_panic", - "__stack_chk_fail", - "__ubsan_handle_builtin_unreachable", - "cpu_bringup_and_idle", - "cpu_startup_entry", - "do_exit", - "do_group_exit", - "do_task_dead", - "ex_handler_msr_mce", - "fortify_panic", - "kthread_complete_and_exit", - "kthread_exit", - "kunit_try_catch_throw", - "lbug_with_loc", - "machine_real_restart", - "make_task_dead", - "panic", - "rewind_stack_and_make_dead", - "sev_es_terminate", - "snp_abort", - "stop_this_cpu", - "usercopy_abort", - "xen_cpu_bringup_again", - "xen_start_kernel", +#include "noreturns.h" }; if (!func) diff --git a/tools/objtool/noreturns b/tools/objtool/noreturns new file mode 100644 index 000000000000..75f35fbb34d1 --- /dev/null +++ b/tools/objtool/noreturns @@ -0,0 +1,26 @@ +__invalid_creds +__module_put_and_kthread_exit +__reiserfs_panic +__stack_chk_fail +__ubsan_handle_builtin_unreachable +cpu_bringup_and_idle +cpu_startup_entry +do_exit +do_group_exit +do_task_dead +ex_handler_msr_mce +fortify_panic +kthread_complete_and_exit +kthread_exit +kunit_try_catch_throw +lbug_with_loc +machine_real_restart +make_task_dead +panic +rewind_stack_and_make_dead +sev_es_terminate +snp_abort +stop_this_cpu +usercopy_abort +xen_cpu_bringup_again +xen_start_kernel diff --git a/tools/objtool/noreturns.sh b/tools/objtool/noreturns.sh new file mode 100755 index 000000000000..f728cb61e665 --- /dev/null +++ b/tools/objtool/noreturns.sh @@ -0,0 +1,7 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 + +cat noreturns | while read func +do + echo \"${func}\", +done > noreturns.h ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 8/9] objtool: Detect missing __noreturn annotations 2023-04-13 8:48 ` Peter Zijlstra @ 2023-04-13 15:19 ` Josh Poimboeuf 2023-04-13 19:17 ` Peter Zijlstra 0 siblings, 1 reply; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-13 15:19 UTC (permalink / raw) To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes On Thu, Apr 13, 2023 at 10:48:01AM +0200, Peter Zijlstra wrote: > On Wed, Apr 12, 2023 at 12:03:23PM -0700, Josh Poimboeuf wrote: > > Most "unreachable instruction" warnings these days seem to actually be > > the result of a missing __noreturn annotation. Add an explicit check > > for that. > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > > --- > > tools/objtool/Documentation/objtool.txt | 6 ++++++ > > tools/objtool/check.c | 14 +++++++++++++- > > 2 files changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt > > index 5a69c207a10e..2cd1fa16ed08 100644 > > --- a/tools/objtool/Documentation/objtool.txt > > +++ b/tools/objtool/Documentation/objtool.txt > > @@ -303,6 +303,12 @@ the objtool maintainers. > > If it's not actually in a callable function (e.g. kernel entry code), > > change ENDPROC to END. > > > > +3. file.o: warning: objtool: foo+0x48c: bar() is missing a __noreturn annotation > > + > > + The call from foo() to bar() doesn't return, but bar() is missing the > > + __noreturn annotation. NOTE: In addition to adding the __noreturn > > + annotation, the function name also needs to be added to > > + 'global_noreturns' in tools/objtool/check.c. > > Do we want something like the below (except perhaps less horrible) ? Yeah, maybe. Another possible way to do it: diff --git a/tools/objtool/check.c b/tools/objtool/check.c index cae6ac6ff246..a4e8ff9dabf1 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -192,39 +192,16 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func, struct instruction *insn; bool empty = true; +#define NORETURN(func) "\"" __stringify(func) "\"", + /* * Unfortunately these have to be hard coded because the noreturn * attribute isn't provided in ELF data. Keep 'em sorted. */ static const char * const global_noreturns[] = { - "__invalid_creds", - "__module_put_and_kthread_exit", - "__reiserfs_panic", - "__stack_chk_fail", - "__ubsan_handle_builtin_unreachable", - "arch_cpu_idle_dead", - "cpu_bringup_and_idle", - "cpu_startup_entry", - "do_exit", - "do_group_exit", - "do_task_dead", - "ex_handler_msr_mce", - "fortify_panic", - "kthread_complete_and_exit", - "kthread_exit", - "kunit_try_catch_throw", - "lbug_with_loc", - "machine_real_restart", - "make_task_dead", - "panic", - "rewind_stack_and_make_dead", - "sev_es_terminate", - "snp_abort", - "stop_this_cpu", - "usercopy_abort", - "xen_cpu_bringup_again", - "xen_start_kernel", +#include "noreturns.h" }; +#undef NORETURN if (!func) return false; diff --git a/tools/objtool/noreturns.h b/tools/objtool/noreturns.h new file mode 100644 index 000000000000..0f5e53fd9e7a --- /dev/null +++ b/tools/objtool/noreturns.h @@ -0,0 +1,27 @@ +NORETURN(__invalid_creds) +NORETURN(__module_put_and_kthread_exit) +NORETURN(__reiserfs_panic) +NORETURN(__stack_chk_fail) +NORETURN(__ubsan_handle_builtin_unreachable) +NORETURN(arch_cpu_idle_dead) +NORETURN(cpu_bringup_and_idle) +NORETURN(cpu_startup_entry) +NORETURN(do_exit) +NORETURN(do_group_exit) +NORETURN(do_task_dead) +NORETURN(ex_handler_msr_mce) +NORETURN(fortify_panic) +NORETURN(kthread_complete_and_exit) +NORETURN(kthread_exit) +NORETURN(kunit_try_catch_throw) +NORETURN(lbug_with_loc) +NORETURN(machine_real_restart) +NORETURN(make_task_dead) +NORETURN(panic) +NORETURN(rewind_stack_and_make_dead) +NORETURN(sev_es_terminate) +NORETURN(snp_abort) +NORETURN(stop_this_cpu) +NORETURN(usercopy_abort) +NORETURN(xen_cpu_bringup_again) +NORETURN(xen_start_kernel) ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 8/9] objtool: Detect missing __noreturn annotations 2023-04-13 15:19 ` Josh Poimboeuf @ 2023-04-13 19:17 ` Peter Zijlstra 0 siblings, 0 replies; 28+ messages in thread From: Peter Zijlstra @ 2023-04-13 19:17 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes > 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 8/9] objtool: Detect missing __noreturn annotations 2023-04-12 19:03 ` [PATCH v2 8/9] objtool: Detect missing __noreturn annotations Josh Poimboeuf 2023-04-13 8:48 ` Peter Zijlstra @ 2023-04-13 14:19 ` Miroslav Benes 2023-04-13 15:32 ` Josh Poimboeuf 1 sibling, 1 reply; 28+ messages in thread From: Miroslav Benes @ 2023-04-13 14:19 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: x86, linux-kernel, Peter Zijlstra > --- 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 8/9] objtool: Detect missing __noreturn annotations 2023-04-13 14:19 ` Miroslav Benes @ 2023-04-13 15:32 ` Josh Poimboeuf 0 siblings, 0 replies; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-13 15:32 UTC (permalink / raw) To: Miroslav Benes; +Cc: x86, linux-kernel, Peter Zijlstra 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 9/9] objtool: Ignore exc_double_fault() __noreturn warnings 2023-04-12 19:03 [PATCH v2 0/9] objtool: warning improvements Josh Poimboeuf ` (7 preceding siblings ...) 2023-04-12 19:03 ` [PATCH v2 8/9] objtool: Detect missing __noreturn annotations Josh Poimboeuf @ 2023-04-12 19:03 ` Josh Poimboeuf 2023-04-13 9:24 ` Peter Zijlstra 8 siblings, 1 reply; 28+ messages in thread From: Josh Poimboeuf @ 2023-04-12 19:03 UTC (permalink / raw) To: x86; +Cc: linux-kernel, Peter Zijlstra, Miroslav Benes 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 <jpoimboe@kernel.org> --- 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 9/9] objtool: Ignore exc_double_fault() __noreturn warnings 2023-04-12 19:03 ` [PATCH v2 9/9] objtool: Ignore exc_double_fault() __noreturn warnings Josh Poimboeuf @ 2023-04-13 9:24 ` Peter Zijlstra 2023-04-14 21:16 ` Nick Desaulniers 0 siblings, 1 reply; 28+ messages in thread From: Peter Zijlstra @ 2023-04-13 9:24 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes, Nick Desaulniers 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 9/9] objtool: Ignore exc_double_fault() __noreturn warnings 2023-04-13 9:24 ` Peter Zijlstra @ 2023-04-14 21:16 ` Nick Desaulniers 0 siblings, 0 replies; 28+ messages in thread From: Nick Desaulniers @ 2023-04-14 21:16 UTC (permalink / raw) To: Peter Zijlstra Cc: Josh Poimboeuf, x86, linux-kernel, Miroslav Benes, linux-toolchains On Thu, Apr 13, 2023 at 2:24 AM Peter Zijlstra <peterz@infradead.org> 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2023-04-14 21:17 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-04-12 19:03 [PATCH v2 0/9] objtool: warning improvements Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 1/9] scripts/objdump-func: Support multiple functions Josh Poimboeuf 2023-04-14 14:47 ` [tip: objtool/core] " tip-bot2 for Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 2/9] objtool: Add WARN_INSN() Josh Poimboeuf 2023-04-14 14:47 ` [tip: objtool/core] " tip-bot2 for Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 3/9] objtool: Limit unreachable warnings to once per function Josh Poimboeuf 2023-04-13 8:01 ` Peter Zijlstra 2023-04-13 15:03 ` Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 4/9] objtool: Add symbol iteration helpers Josh Poimboeuf 2023-04-14 14:47 ` [tip: objtool/core] " tip-bot2 for Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 5/9] objtool: Add verbose option for disassembling affected functions Josh Poimboeuf 2023-04-13 8:05 ` Peter Zijlstra 2023-04-13 15:05 ` Josh Poimboeuf 2023-04-13 8:08 ` Peter Zijlstra 2023-04-13 14:03 ` Miroslav Benes 2023-04-13 15:09 ` Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 6/9] objtool: Include backtrace in verbose mode Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 7/9] objtool: Remove superfluous dead_end_function() check Josh Poimboeuf 2023-04-14 14:47 ` [tip: objtool/core] " tip-bot2 for Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 8/9] objtool: Detect missing __noreturn annotations Josh Poimboeuf 2023-04-13 8:48 ` Peter Zijlstra 2023-04-13 15:19 ` Josh Poimboeuf 2023-04-13 19:17 ` Peter Zijlstra 2023-04-13 14:19 ` Miroslav Benes 2023-04-13 15:32 ` Josh Poimboeuf 2023-04-12 19:03 ` [PATCH v2 9/9] objtool: Ignore exc_double_fault() __noreturn warnings Josh Poimboeuf 2023-04-13 9:24 ` Peter Zijlstra 2023-04-14 21:16 ` Nick Desaulniers
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.