| Message ID | 20200109160300.26150-13-jthierry@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | objtool: Add support for arm64 | expand |
On Thu, Jan 09, 2020 at 04:02:15PM +0000, Julien Thierry wrote: > Alternatives can contain instructions that jump to another instruction > in the same alternative group. This is actually a common pattern on > arm64. LL/SC I bet...
On Mon, Jan 20, 2020 at 03:56:56PM +0100, Peter Zijlstra wrote: > On Thu, Jan 09, 2020 at 04:02:15PM +0000, Julien Thierry wrote: > > Alternatives can contain instructions that jump to another instruction > > in the same alternative group. This is actually a common pattern on > > arm64. > > LL/SC I bet... I think there are some nasty errata workarounds that need it too. WIll
On Thu, Jan 09, 2020 at 04:02:15PM +0000, Julien Thierry wrote: > Alternatives can contain instructions that jump to another instruction > in the same alternative group. This is actually a common pattern on > arm64. > > Keep track of instructions jumping within their own alternative group > and carry on validating such branches. > > Signed-off-by: Julien Thierry <jthierry@redhat.com> > --- > tools/objtool/check.c | 48 ++++++++++++++++++++++++++++++++++--------- > tools/objtool/check.h | 1 + > 2 files changed, 39 insertions(+), 10 deletions(-) > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > index 8f2ff030936d..c7b3f1e2a628 100644 > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -722,6 +722,14 @@ static int handle_group_alt(struct objtool_file *file, > sec_for_each_insn_from(file, insn) { > if (insn->offset >= special_alt->orig_off + special_alt->orig_len) > break; > + /* Is insn a jump to an instruction within the alt_group */ > + if (insn->jump_dest && insn->sec == insn->jump_dest->sec && > + (insn->type == INSN_JUMP_CONDITIONAL || > + insn->type == INSN_JUMP_UNCONDITIONAL)) { > + dest_off = insn->jump_dest->offset; > + insn->intra_group_jump = special_alt->orig_off <= dest_off && > + dest_off < special_alt->orig_off + special_alt->orig_len; > + } This patch adds some complexity, just so we can keep the "don't know how to handle branch to middle of alternative instruction group" warning for the case where code from outside an alternative insn group is branching to inside the group. But I've never actually seen that case in practice, and I get the feeling that warning isn't very useful or realistic. How about we just remove the warning?
On 1/21/20 5:33 PM, Josh Poimboeuf wrote: > On Thu, Jan 09, 2020 at 04:02:15PM +0000, Julien Thierry wrote: >> Alternatives can contain instructions that jump to another instruction >> in the same alternative group. This is actually a common pattern on >> arm64. >> >> Keep track of instructions jumping within their own alternative group >> and carry on validating such branches. >> >> Signed-off-by: Julien Thierry <jthierry@redhat.com> >> --- >> tools/objtool/check.c | 48 ++++++++++++++++++++++++++++++++++--------- >> tools/objtool/check.h | 1 + >> 2 files changed, 39 insertions(+), 10 deletions(-) >> >> diff --git a/tools/objtool/check.c b/tools/objtool/check.c >> index 8f2ff030936d..c7b3f1e2a628 100644 >> --- a/tools/objtool/check.c >> +++ b/tools/objtool/check.c >> @@ -722,6 +722,14 @@ static int handle_group_alt(struct objtool_file *file, >> sec_for_each_insn_from(file, insn) { >> if (insn->offset >= special_alt->orig_off + special_alt->orig_len) >> break; >> + /* Is insn a jump to an instruction within the alt_group */ >> + if (insn->jump_dest && insn->sec == insn->jump_dest->sec && >> + (insn->type == INSN_JUMP_CONDITIONAL || >> + insn->type == INSN_JUMP_UNCONDITIONAL)) { >> + dest_off = insn->jump_dest->offset; >> + insn->intra_group_jump = special_alt->orig_off <= dest_off && >> + dest_off < special_alt->orig_off + special_alt->orig_len; >> + } > > This patch adds some complexity, just so we can keep the > > "don't know how to handle branch to middle of alternative instruction group" > > warning for the case where code from outside an alternative insn group > is branching to inside the group. But I've never actually seen that > case in practice, and I get the feeling that warning isn't very useful > or realistic. > > How about we just remove the warning? > I'm happy to remove it. I was trying to look for a less intrusive place to only check for instructions jumping into alternatives. add_jump_destinations() could be an option, but it would require to duplicate some stuff done in add_special_section_alts(). So maybe just ignoring for now can be fine. Thanks,
diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 8f2ff030936d..c7b3f1e2a628 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -722,6 +722,14 @@ static int handle_group_alt(struct objtool_file *file, sec_for_each_insn_from(file, insn) { if (insn->offset >= special_alt->orig_off + special_alt->orig_len) break; + /* Is insn a jump to an instruction within the alt_group */ + if (insn->jump_dest && insn->sec == insn->jump_dest->sec && + (insn->type == INSN_JUMP_CONDITIONAL || + insn->type == INSN_JUMP_UNCONDITIONAL)) { + dest_off = insn->jump_dest->offset; + insn->intra_group_jump = special_alt->orig_off <= dest_off && + dest_off < special_alt->orig_off + special_alt->orig_len; + } insn->alt_group = true; last_orig_insn = insn; @@ -1853,14 +1861,33 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st return validate_call(insn, state); } +static int validate_branch_alt_safe(struct objtool_file *file, + struct symbol *func, + struct instruction *first, + struct insn_state state); + +static int validate_branch(struct objtool_file *file, struct symbol *func, + struct instruction *first, struct insn_state state) +{ + if (first->alt_group && list_empty(&first->alts)) { + WARN_FUNC("don't know how to handle branch to middle of alternative instruction group", + first->sec, first->offset); + return 1; + } + + return validate_branch_alt_safe(file, func, first, state); +} + /* * Follow the branch starting at the given instruction, and recursively follow * any other branches (jumps). Meanwhile, track the frame pointer state at * each instruction and validate all the rules described in * tools/objtool/Documentation/stack-validation.txt. */ -static int validate_branch(struct objtool_file *file, struct symbol *func, - struct instruction *first, struct insn_state state) +static int validate_branch_alt_safe(struct objtool_file *file, + struct symbol *func, + struct instruction *first, + struct insn_state state) { struct alternative *alt; struct instruction *insn, *next_insn; @@ -1871,12 +1898,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, insn = first; 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; - } - while (1) { next_insn = next_insn_same_sec(file, insn); @@ -2023,8 +2044,15 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, return ret; } else if (insn->jump_dest) { - ret = validate_branch(file, func, - insn->jump_dest, state); + if (insn->intra_group_jump) + ret = validate_branch_alt_safe(file, + func, + insn->jump_dest, + state); + else + ret = validate_branch(file, func, + insn->jump_dest, + state); if (ret) { if (backtrace) BT_FUNC("(branch)", insn); diff --git a/tools/objtool/check.h b/tools/objtool/check.h index af87b55db454..d13ee02f28a4 100644 --- a/tools/objtool/check.h +++ b/tools/objtool/check.h @@ -46,6 +46,7 @@ struct instruction { struct stack_op stack_op; struct insn_state state; struct orc_entry orc; + bool intra_group_jump; }; struct objtool_file {
Alternatives can contain instructions that jump to another instruction in the same alternative group. This is actually a common pattern on arm64. Keep track of instructions jumping within their own alternative group and carry on validating such branches. Signed-off-by: Julien Thierry <jthierry@redhat.com> --- tools/objtool/check.c | 48 ++++++++++++++++++++++++++++++++++--------- tools/objtool/check.h | 1 + 2 files changed, 39 insertions(+), 10 deletions(-)