Messages in this thread Patch in this message | | | Date | Thu, 26 Mar 2020 20:00:01 -0500 | From | Josh Poimboeuf <> | Subject | Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case |
| |
On Thu, Mar 26, 2020 at 08:57:18PM +0100, Peter Zijlstra wrote: > On Thu, Mar 26, 2020 at 04:49:38PM +0100, Peter Zijlstra wrote: > > > The 'insn == first' check isn't ideal, but at least it works (I think?). > > > > It works, yes, for exactly this one case. > > How's this? Ignore the ignore_cfi bits, that's a 'failed' experiment.
It still seems complex to me.
What do you think about this? If we store save_insn in the state when we see insn->save, the restore logic becomes a lot easier. Then if we get a restore without a save, we can just ignore the restore hint in that path. Later, when we see the restore insn again from the save path, we can then compare the insn state with the saved state to make sure they match.
This assumes no crazy save/restore scenarios. It also means that the restore path has to be reachable from the save path, for which I had to make a change to make IRETQ *not* a dead end if there's a restore hint immediately after it.
diff --git a/tools/objtool/check.c b/tools/objtool/check.c index e637a4a38d2a..e9becd50f148 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1265,7 +1265,6 @@ static int read_unwind_hints(struct objtool_file *file) } else if (hint->type == UNWIND_HINT_TYPE_RESTORE) { insn->restore = true; - insn->hint = true; continue; } @@ -2003,7 +2002,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, struct instruction *first, struct insn_state state) { struct alternative *alt; - struct instruction *insn, *next_insn; + struct instruction *insn, *next_insn, *save_insn = NULL; struct section *sec; u8 visited; int ret; @@ -2034,54 +2033,32 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, visited = 1 << state.uaccess; if (insn->visited) { - if (!insn->hint && !insn_state_match(insn, &state)) + if (!insn->hint && !insn->restore && + !insn_state_match(insn, &state)) { return 1; + } + + if (insn->restore && save_insn) { + if (!insn_state_match(insn, &save_insn->state)) + return 1; + save_insn = NULL; + } if (insn->visited & visited) return 0; } - if (insn->hint) { - if (insn->restore) { - struct instruction *save_insn, *i; - - i = insn; - save_insn = NULL; - func_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; - - WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo", - sec, insn->offset); - return 1; - } + if (insn->save) + save_insn = insn; - insn->state = save_insn->state; - } + if (insn->restore && save_insn) { + insn->state = save_insn->state; + save_insn = NULL; + } + if (insn->hint) state = insn->state; - - } else + else insn->state = state; insn->visited |= visited; @@ -2191,12 +2168,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, break; case INSN_CONTEXT_SWITCH: - if (func && (!next_insn || !next_insn->hint)) { - WARN_FUNC("unsupported instruction in callable function", - sec, insn->offset); - return 1; + if (!next_insn || !next_insn->restore) { + if (func) { + WARN_FUNC("unsupported instruction in callable function", + sec, insn->offset); + return 1; + } + + return 0; } - return 0; + + break; case INSN_STACK: if (update_insn_state(insn, &state)) @@ -2293,7 +2275,7 @@ static int validate_unwind_hints(struct objtool_file *file) clear_insn_state(&state); for_each_insn(file, insn) { - if (insn->hint && !insn->visited) { + if ((insn->hint || insn->save || insn->restore) && !insn->visited) { ret = validate_branch(file, insn->func, insn, state); if (ret && backtrace) BT_FUNC("<=== (hint)", insn);
| |