| Date | Tue, 24 Mar 2020 16:40:06 -0500 | From | Josh Poimboeuf <> | Subject | Re: [PATCH v3 18/26] objtool: Fix !CFI insn_state propagation |
| |
On Tue, Mar 24, 2020 at 04:31:31PM +0100, Peter Zijlstra wrote: > Objtool keeps per instruction CFI state in struct insn_state and will > save/restore this where required. However, insn_state has grown some > !CFI state, and this must not be saved/restored (and thus lost). > > Fix this by explicitly preserving the !CFI state and clarify by > restucturing the code and adding a comment. > > XXX, the insn==first condition is not handled right. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > tools/objtool/check.c | 95 +++++++++++++++++++++++++++++--------------------- > tools/objtool/check.h | 8 ++++ > 2 files changed, 64 insertions(+), 39 deletions(-) > > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -2033,6 +2033,59 @@ static int validate_return(struct symbol > return 0; > } > > +static int apply_insn_hint(struct objtool_file *file, struct section *sec, > + struct symbol *func, struct instruction *first, > + struct instruction *insn, struct insn_state *state) > +{ > + struct insn_state old = *state; > + > + if (insn->restore) { > + struct instruction *save_insn, *i; > + > + i = insn; > + save_insn = NULL; > + sym_for_each_insn_continue_reverse(file, func, i) { > + if (i->save) { > + save_insn = i; > + break; > + } > + } > + > + if (!save_insn) { > + WARN_FUNC("no corresponding CFI save for CFI restore", > + sec, insn->offset); > + return 1; > + } > + > + if (!save_insn->visited) { > + /* > + * Oops, no state to copy yet. > + * Hopefully we can reach this > + * instruction from another branch > + * after the save insn has been > + * visited. > + */ > + if (insn == first) > + return 0; // XXX
Yeah, moving this code out to apply_insn_hint() seems like a nice idea, but it wouldn't be worth it if it breaks this case. TBH I don't remember if this check was for a real-world case. Might be worth looking at... If this case doesn't exist in reality then we could just remove this check altogether.
> + > + WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo", > + sec, insn->offset); > + return 1; > + } > + > + insn->state = save_insn->state; > + } > + > + *state = insn->state;
This would have been easier to review if apply_insn_hint() were added in a separate patch.
> + > + /* restore !CFI state */ > + state->df = old.df; > + state->uaccess = old.uaccess; > + state->uaccess_stack = old.uaccess_stack;
Maybe we should just move the CFI stuff into a state->cfi substruct. That would remove the need for these bits and probably also the comment above the insn_state declaration.
-- Josh
|