The code to validate a branch loops through all instructions of the
branch and validate each instruction. Move the code to validate an
instruction to a separated function.
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
tools/objtool/check.c | 375 +++++++++++++++++++++++-------------------
1 file changed, 208 insertions(+), 167 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 2c73c8d3515d..36ec08b8d654 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3527,6 +3527,11 @@ static bool skip_alt_group(struct instruction *insn)
return alt_insn->type == INSN_CLAC || alt_insn->type == INSN_STAC;
}
+static int validate_insn(struct objtool_file *file, struct symbol *func,
+ struct instruction *insn, struct insn_state *statep,
+ struct instruction *prev_insn, struct instruction *next_insn,
+ bool *validate_nextp);
+
/*
* Follow the branch starting at the given instruction, and recursively follow
* any other branches (jumps). Meanwhile, track the frame pointer state at
@@ -3536,10 +3541,9 @@ static bool skip_alt_group(struct instruction *insn)
static int validate_branch(struct objtool_file *file, struct symbol *func,
struct instruction *insn, struct insn_state state)
{
- struct alternative *alt;
struct instruction *next_insn, *prev_insn = NULL;
struct section *sec;
- u8 visited;
+ bool validate_next;
int ret;
if (func && func->ignore)
@@ -3566,232 +3570,269 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
return 1;
}
- visited = VISITED_BRANCH << state.uaccess;
- if (insn->visited & VISITED_BRANCH_MASK) {
- if (!insn->hint && !insn_cfi_match(insn, &state.cfi))
- return 1;
+ ret = validate_insn(file, func, insn, &state,
+ prev_insn, next_insn,
+ &validate_next);
+ if (!validate_next)
+ break;
- if (insn->visited & visited)
+ if (!next_insn) {
+ if (state.cfi.cfa.base == CFI_UNDEFINED)
return 0;
- } else {
- nr_insns_visited++;
+ if (file->ignore_unreachables)
+ return 0;
+
+ WARN("%s%sunexpected end of section %s",
+ func ? func->name : "", func ? "(): " : "",
+ sec->name);
+ return 1;
}
- if (state.noinstr)
- state.instr += insn->instr;
+ prev_insn = insn;
+ insn = next_insn;
+ }
- if (insn->hint) {
- if (insn->restore) {
- struct instruction *save_insn, *i;
+ return ret;
+}
- i = insn;
- save_insn = NULL;
+static int validate_insn(struct objtool_file *file, struct symbol *func,
+ struct instruction *insn, struct insn_state *statep,
+ struct instruction *prev_insn, struct instruction *next_insn,
+ bool *validate_nextp)
+{
+ struct alternative *alt;
+ u8 visited;
+ int ret;
- sym_for_each_insn_continue_reverse(file, func, i) {
- if (i->save) {
- save_insn = i;
- break;
- }
- }
+ /*
+ * Indicate that, by default, the calling function should not
+ * validate the next instruction and validation should be
+ * stopped. That way this function can stop validation by just
+ * returning at any point before reaching the end of the function.
+ *
+ * If the end of this function is reached then that means that the
+ * validation should continue and the caller should validate the
+ * next instruction, so *validate_nextp will be set to true at
+ * that point.
+ */
+ *validate_nextp = false;
- if (!save_insn) {
- WARN_INSN(insn, "no corresponding CFI save for CFI restore");
- return 1;
- }
+ visited = VISITED_BRANCH << statep->uaccess;
+ if (insn->visited & VISITED_BRANCH_MASK) {
+ if (!insn->hint && !insn_cfi_match(insn, &statep->cfi))
+ return 1;
- if (!save_insn->visited) {
- /*
- * If the restore hint insn is at the
- * beginning of a basic block and was
- * branched to from elsewhere, and the
- * save insn hasn't been visited yet,
- * defer following this branch for now.
- * It will be seen later via the
- * straight-line path.
- */
- if (!prev_insn)
- return 0;
+ if (insn->visited & visited)
+ return 0;
+ } else {
+ nr_insns_visited++;
+ }
- WARN_INSN(insn, "objtool isn't smart enough to handle this CFI save/restore combo");
- return 1;
- }
+ if (statep->noinstr)
+ statep->instr += insn->instr;
- insn->cfi = save_insn->cfi;
- nr_cfi_reused++;
- }
+ if (insn->hint) {
+ if (insn->restore) {
+ struct instruction *save_insn, *i;
- state.cfi = *insn->cfi;
- } else {
- /* XXX track if we actually changed state.cfi */
+ i = insn;
+ save_insn = NULL;
- if (prev_insn && !cficmp(prev_insn->cfi, &state.cfi)) {
- insn->cfi = prev_insn->cfi;
- nr_cfi_reused++;
- } else {
- insn->cfi = cfi_hash_find_or_add(&state.cfi);
+ sym_for_each_insn_continue_reverse(file, func, i) {
+ if (i->save) {
+ save_insn = i;
+ break;
+ }
}
- }
- insn->visited |= visited;
+ if (!save_insn) {
+ WARN_INSN(insn, "no corresponding CFI save for CFI restore");
+ return 1;
+ }
- if (propagate_alt_cfi(file, insn))
- return 1;
+ if (!save_insn->visited) {
+ /*
+ * If the restore hint insn is at the
+ * beginning of a basic block and was
+ * branched to from elsewhere, and the
+ * save insn hasn't been visited yet,
+ * defer following this branch for now.
+ * It will be seen later via the
+ * straight-line path.
+ */
+ if (!prev_insn)
+ return 0;
- if (insn->alts) {
- for (alt = insn->alts; alt; alt = alt->next) {
- ret = validate_branch(file, func, alt->insn, state);
- if (ret) {
- BT_INSN(insn, "(alt)");
- return ret;
- }
+ WARN_INSN(insn, "objtool isn't smart enough to handle this CFI save/restore combo");
+ return 1;
}
+
+ insn->cfi = save_insn->cfi;
+ nr_cfi_reused++;
}
- if (skip_alt_group(insn))
- return 0;
+ statep->cfi = *insn->cfi;
+ } else {
+ /* XXX track if we actually changed statep->cfi */
- if (handle_insn_ops(insn, next_insn, &state))
- return 1;
+ if (prev_insn && !cficmp(prev_insn->cfi, &statep->cfi)) {
+ insn->cfi = prev_insn->cfi;
+ nr_cfi_reused++;
+ } else {
+ insn->cfi = cfi_hash_find_or_add(&statep->cfi);
+ }
+ }
- switch (insn->type) {
+ insn->visited |= visited;
- case INSN_RETURN:
- return validate_return(func, insn, &state);
+ if (propagate_alt_cfi(file, insn))
+ return 1;
- case INSN_CALL:
- case INSN_CALL_DYNAMIC:
- ret = validate_call(file, insn, &state);
- if (ret)
+ if (insn->alts) {
+ for (alt = insn->alts; alt; alt = alt->next) {
+ ret = validate_branch(file, func, alt->insn, *statep);
+ if (ret) {
+ BT_INSN(insn, "(alt)");
return ret;
-
- if (opts.stackval && func && !is_special_call(insn) &&
- !has_valid_stack_frame(&state)) {
- WARN_INSN(insn, "call without frame pointer save/setup");
- return 1;
}
+ }
+ }
- break;
-
- case INSN_JUMP_CONDITIONAL:
- case INSN_JUMP_UNCONDITIONAL:
- if (is_sibling_call(insn)) {
- ret = validate_sibling_call(file, insn, &state);
- if (ret)
- return ret;
-
- } else if (insn->jump_dest) {
- ret = validate_branch(file, func,
- insn->jump_dest, state);
- if (ret) {
- BT_INSN(insn, "(branch)");
- return ret;
- }
- }
+ if (skip_alt_group(insn))
+ return 0;
- if (insn->type == INSN_JUMP_UNCONDITIONAL)
- return 0;
+ if (handle_insn_ops(insn, next_insn, statep))
+ return 1;
- break;
+ switch (insn->type) {
- case INSN_JUMP_DYNAMIC:
- case INSN_JUMP_DYNAMIC_CONDITIONAL:
- if (is_sibling_call(insn)) {
- ret = validate_sibling_call(file, insn, &state);
- if (ret)
- return ret;
- }
+ case INSN_RETURN:
+ return validate_return(func, insn, statep);
- if (insn->type == INSN_JUMP_DYNAMIC)
- return 0;
+ case INSN_CALL:
+ case INSN_CALL_DYNAMIC:
+ ret = validate_call(file, insn, statep);
+ if (ret)
+ return ret;
- break;
+ if (opts.stackval && func && !is_special_call(insn) &&
+ !has_valid_stack_frame(statep)) {
+ WARN_INSN(insn, "call without frame pointer save/setup");
+ return 1;
+ }
- case INSN_SYSCALL:
- if (func && (!next_insn || !next_insn->hint)) {
- WARN_INSN(insn, "unsupported instruction in callable function");
- return 1;
- }
+ break;
- break;
+ case INSN_JUMP_CONDITIONAL:
+ case INSN_JUMP_UNCONDITIONAL:
+ if (is_sibling_call(insn)) {
+ ret = validate_sibling_call(file, insn, statep);
+ if (ret)
+ return ret;
- case INSN_SYSRET:
- if (func && (!next_insn || !next_insn->hint)) {
- WARN_INSN(insn, "unsupported instruction in callable function");
- return 1;
+ } else if (insn->jump_dest) {
+ ret = validate_branch(file, func,
+ insn->jump_dest, *statep);
+ if (ret) {
+ BT_INSN(insn, "(branch)");
+ return ret;
}
+ }
+ if (insn->type == INSN_JUMP_UNCONDITIONAL)
return 0;
- case INSN_STAC:
- if (!opts.uaccess)
- break;
+ break;
- if (state.uaccess) {
- WARN_INSN(insn, "recursive UACCESS enable");
- return 1;
- }
+ case INSN_JUMP_DYNAMIC:
+ case INSN_JUMP_DYNAMIC_CONDITIONAL:
+ if (is_sibling_call(insn)) {
+ ret = validate_sibling_call(file, insn, statep);
+ if (ret)
+ return ret;
+ }
- state.uaccess = true;
- break;
+ if (insn->type == INSN_JUMP_DYNAMIC)
+ return 0;
- case INSN_CLAC:
- if (!opts.uaccess)
- break;
+ break;
- if (!state.uaccess && func) {
- WARN_INSN(insn, "redundant UACCESS disable");
- return 1;
- }
+ case INSN_SYSCALL:
+ if (func && (!next_insn || !next_insn->hint)) {
+ WARN_INSN(insn, "unsupported instruction in callable function");
+ return 1;
+ }
- if (func_uaccess_safe(func) && !state.uaccess_stack) {
- WARN_INSN(insn, "UACCESS-safe disables UACCESS");
- return 1;
- }
+ break;
- state.uaccess = false;
- break;
+ case INSN_SYSRET:
+ if (func && (!next_insn || !next_insn->hint)) {
+ WARN_INSN(insn, "unsupported instruction in callable function");
+ return 1;
+ }
- case INSN_STD:
- if (state.df) {
- WARN_INSN(insn, "recursive STD");
- return 1;
- }
+ return 0;
- state.df = true;
+ case INSN_STAC:
+ if (!opts.uaccess)
break;
- case INSN_CLD:
- if (!state.df && func) {
- WARN_INSN(insn, "redundant CLD");
- return 1;
- }
+ if (statep->uaccess) {
+ WARN_INSN(insn, "recursive UACCESS enable");
+ return 1;
+ }
- state.df = false;
- break;
+ statep->uaccess = true;
+ break;
- default:
+ case INSN_CLAC:
+ if (!opts.uaccess)
break;
+
+ if (!statep->uaccess && func) {
+ WARN_INSN(insn, "redundant UACCESS disable");
+ return 1;
}
- if (insn->dead_end)
- return 0;
+ if (func_uaccess_safe(func) && !statep->uaccess_stack) {
+ WARN_INSN(insn, "UACCESS-safe disables UACCESS");
+ return 1;
+ }
- if (!next_insn) {
- if (state.cfi.cfa.base == CFI_UNDEFINED)
- return 0;
- if (file->ignore_unreachables)
- return 0;
+ statep->uaccess = false;
+ break;
- WARN("%s%sunexpected end of section %s",
- func ? func->name : "", func ? "(): " : "",
- sec->name);
+ case INSN_STD:
+ if (statep->df) {
+ WARN_INSN(insn, "recursive STD");
return 1;
}
- prev_insn = insn;
- insn = next_insn;
+ statep->df = true;
+ break;
+
+ case INSN_CLD:
+ if (!statep->df && func) {
+ WARN_INSN(insn, "redundant CLD");
+ return 1;
+ }
+
+ statep->df = false;
+ break;
+
+ default:
+ break;
}
+ if (insn->dead_end)
+ return 0;
+
+ /*
+ * Indicate that the caller should validate the next
+ * instruction and continue the validation.
+ */
+ *validate_nextp = true;
+
return 0;
}
--
2.43.5On Fri, Jun 06, 2025 at 05:34:34PM +0200, Alexandre Chartre wrote:
> The code to validate a branch loops through all instructions of the
> branch and validate each instruction. Move the code to validate an
> instruction to a separated function.
>
> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> ---
> tools/objtool/check.c | 375 +++++++++++++++++++++++-------------------
> 1 file changed, 208 insertions(+), 167 deletions(-)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 2c73c8d3515d..36ec08b8d654 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -3527,6 +3527,11 @@ static bool skip_alt_group(struct instruction *insn)
> return alt_insn->type == INSN_CLAC || alt_insn->type == INSN_STAC;
> }
>
> +static int validate_insn(struct objtool_file *file, struct symbol *func,
> + struct instruction *insn, struct insn_state *statep,
> + struct instruction *prev_insn, struct instruction *next_insn,
> + bool *validate_nextp);
Since validate_insn() is always called by validate_branch(), put the
validate_insn() implementation here above validate_branch().
> /*
> * Follow the branch starting at the given instruction, and recursively follow
> * any other branches (jumps). Meanwhile, track the frame pointer state at
> @@ -3536,10 +3541,9 @@ static bool skip_alt_group(struct instruction *insn)
> static int validate_branch(struct objtool_file *file, struct symbol *func,
> struct instruction *insn, struct insn_state state)
> {
> - struct alternative *alt;
> struct instruction *next_insn, *prev_insn = NULL;
> struct section *sec;
> - u8 visited;
> + bool validate_next;
I think it's clearer if we reverse the polarity and call it "dead_end".
> @@ -3566,232 +3570,269 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> return 1;
> }
>
> - visited = VISITED_BRANCH << state.uaccess;
> - if (insn->visited & VISITED_BRANCH_MASK) {
> - if (!insn->hint && !insn_cfi_match(insn, &state.cfi))
> - return 1;
> + ret = validate_insn(file, func, insn, &state,
> + prev_insn, next_insn,
> + &validate_next);
This can fit in two lines:
ret = validate_insn(file, func, insn, &state, prev_insn,
next_insn, &dead_end);
> +static int validate_insn(struct objtool_file *file, struct symbol *func,
> + struct instruction *insn, struct insn_state *statep,
> + struct instruction *prev_insn, struct instruction *next_insn,
> + bool *validate_nextp)
> +{
> + struct alternative *alt;
> + u8 visited;
> + int ret;
>
> - sym_for_each_insn_continue_reverse(file, func, i) {
> - if (i->save) {
> - save_insn = i;
> - break;
> - }
> - }
> + /*
> + * Indicate that, by default, the calling function should not
> + * validate the next instruction and validation should be
> + * stopped. That way this function can stop validation by just
> + * returning at any point before reaching the end of the function.
> + *
> + * If the end of this function is reached then that means that the
> + * validation should continue and the caller should validate the
> + * next instruction, so *validate_nextp will be set to true at
> + * that point.
> + */
> + *validate_nextp = false;
This can be much more succinct:
/*
* Any returns before the end of this function are effectively dead
* ends, i.e. validate_branch() has reached the end of the branch.
*/
*dead_end = true;
> + default:
> + break;
> }
>
> + if (insn->dead_end)
> + return 0;
> +
> + /*
> + * Indicate that the caller should validate the next
> + * instruction and continue the validation.
> + */
> + *validate_nextp = true;
> +
> return 0;
No comment needed here. Also this can be condensed:
*dead_end = insn->dead_end;
return 0;
--
Josh
© 2016 - 2026 Red Hat, Inc.