On Tue, Apr 28, 2020 at 05:25:52PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 28, 2020 at 09:31:57AM -0500, Josh Poimboeuf wrote:
> > That's quite the monstrosity, and I still don't see the point. I
> > thought we decided to just disallow CFI changes in alternatives anyway?
> > That can be done much simpler.
>
> Something like so then ?
>
> ---
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 8443ec690051..d14d83e6edb0 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -940,6 +940,7 @@ static int handle_group_alt(struct objtool_file *file,
>
> last_new_insn = insn;
>
> + insn->alt_group = true;
> insn->ignore = orig_insn->ignore_alts;
> insn->func = orig_insn->func;
>
> @@ -2242,6 +2243,11 @@ static int handle_insn_ops(struct instruction *insn,
> struct insn_state *state)
> list_for_each_entry(op, &insn->stack_ops, list) {
> int res;
>
> + if (insn->alt_group) {
> + WARN_FUNC("alternative has CFI", insn->sec,
> insn->offset);
> + return 1;
> + }
> +
ACK (separate patch)
> res = update_cfi_state(insn, &state->cfi, op);
> if (res)
> return res;
> @@ -2439,12 +2445,6 @@ static int validate_branch(struct objtool_file *file,
> struct symbol *func,
>
> sec = insn->sec;
>
> - if (insn->alt_group && list_empty(&insn->alts)) {
> - WARN_FUNC("don't know how to handle branch to middle of
> alternative instruction group",
> - sec, insn->offset);
> - return 1;
> - }
> -
ACK (separate patch)
> while (1) {
> next_insn = next_insn_same_sec(file, insn);
>
> @@ -2494,8 +2494,16 @@ static int validate_branch(struct objtool_file *file,
> struct symbol *func,
> }
> }
>
> - if (skip_orig)
> + if (skip_orig) {
> + struct instruction *prev_insn = insn;
> + sec_for_each_insn_continue(file, insn) {
> + if (!insn->alt_group)
> + break;
> + if (!insn->visited)
> + insn->cfi = prev_insn->cfi;
> + }
> return 0;
> + }
NACK :-)
What happens if you have two alternatives adjacent to each other (which
can definitely happen in this scenario)?
I still like my patch, at least the hack is done before the validate
code, so validate_branch() itself is simpler.
--
Josh
