Messages in this thread | | | Date | Wed, 17 Jan 2018 08:27:59 -0600 | From | Josh Poimboeuf <> | Subject | Re: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has() |
| |
On Wed, Jan 17, 2018 at 10:19:45AM +0100, Peter Zijlstra wrote: > > > @@ -664,6 +670,21 @@ static int handle_group_alt(struct objto > > > insn->sec, insn->offset); > > > return -1; > > > } > > > + > > > + if (special_alt->static_feat) { > > > + if (insn->type != INSN_JUMP_UNCONDITIONAL) { > > > + WARN_FUNC("not an unconditional jump in _static_cpu_has()", > > > + insn->sec, insn->offset); > > > + } > > > > So I think this is trying to assert the fact that you're only expecting > > a single instruction which is an unconditional jump. > > Correct. > > > We already weeded > > out non-jumps earlier in the loop, so would it make sense to do this > > check before the INSN_JUMP_CONDITIONAL/INSN_JUMP_UNCONDITIONAL check a > > little higher up? > > This way we keep all the special_alt->static_cpu_has bits together.
But if the instruction is something other than a conditional jump, you won't get a warning.
> > > + if (insn->jump_dest == fake_jump) { > > > + WARN_FUNC("jump inside alternative for _static_cpu_has()", > > > + insn->sec, insn->offset); > > > + } > > > > The error message doesn't seem to match the condition, so I'm not sure > > which one you're trying to check, or why. > > > > IIRC, 'insn->jump_dest == fake_jump' means we reached the end of the > > alternative code block without hitting a jump. > > > > But based on the loop exit condition, I don't think it's ever possible > > for insn->jump_dest to ever point to the fake_jump at the end. > > Oof, now what was I thinking again.. So that fake_jump is inserted at > the end of the alternative and jumps to the code after where the > alternative will be patched in to simulate the code flow. > > If there is a jump inside the alternative that jumps to the end, it's > destination will be set to the fake jump, we have this clause for that: > > dest_off = insn->offset + insn->len + insn->immediate; > if (dest_off == special_alt->new_off + special_alt->new_len) > insn->jump_dest = fake_jump; > > if that happens for static_cpu_has(), bad things happened. > > So the only way for a jump to have fake_jump as destination is if the > jump is inside the alternative (but to the end) and we must assert this > didn't happen. > > Unlikely, yes, but I figured we want to know about it if it ever does > happen.
Ok.
> > > + > > > +#define ALT_TYPE_DEFAULT 0 > > > +#define ALT_TYPE_STATIC_CPU_HAS 1 > > > > > > #define X86_FEATURE_POPCNT (4*32+23) > > > > > > @@ -99,10 +104,13 @@ static int get_alt_entry(struct elf *elf > > > > > > if (entry->feature) { > > > > Since this block now uses more than entry->feature, and is now just a > > general alternatives thing, can you change it to > > > > if (entry->feature == ALT_FEATURE_OFFSET) > > > > so it's more clear and slightly more future proof? > > I've made it entry->group instead. How about the below on top? > > --- > > Subject: objtool: Introduce special_type > From: Peter Zijlstra <peterz@infradead.org> > Date: Wed Jan 17 10:16:44 CET 2018 > > Use an enum for the special_alt entries instead of a collection of > booleans. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > tools/objtool/check.c | 11 ++++++++--- > tools/objtool/special.c | 21 +++++++++------------ > tools/objtool/special.h | 10 ++++++++-- > 3 files changed, 25 insertions(+), 17 deletions(-) > > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -773,7 +773,7 @@ static int add_special_section_alts(stru > continue; > > new_insn = NULL; > - if (!special_alt->group || special_alt->new_len) { > + if (special_alt->type != alternative || special_alt->new_len) { > new_insn = find_insn(file, special_alt->new_sec, > special_alt->new_off); > if (!new_insn) { > @@ -785,16 +785,21 @@ static int add_special_section_alts(stru > } > } > > - if (special_alt->group) { > + switch(special_alt->type) { > + case alternative: > ret = handle_group_alt(file, special_alt, orig_insn, > &new_insn); > if (ret) > goto out; > - } else if (special_alt->jump_or_nop) { > + break; > + > + case jump_label: > ret = handle_jump_alt(file, special_alt, orig_insn, > &new_insn); > if (ret) > goto out; > + > + break; > } > > alt = malloc(sizeof(*alt)); > --- a/tools/objtool/special.c > +++ b/tools/objtool/special.c > @@ -49,7 +49,7 @@ > > struct special_entry { > const char *sec; > - bool group, jump_or_nop; > + enum special_type type; > unsigned char size, orig, new; > unsigned char orig_len, new_len; /* group only */ > unsigned char feature; /* ALTERNATIVE macro CPU feature */ > @@ -58,7 +58,7 @@ struct special_entry { > struct special_entry entries[] = { > { > .sec = ".altinstructions", > - .group = true, > + .type = alternative, > .size = ALT_ENTRY_SIZE, > .orig = ALT_ORIG_OFFSET, > .orig_len = ALT_ORIG_LEN_OFFSET, > @@ -68,13 +68,14 @@ struct special_entry entries[] = { > }, > { > .sec = "__jump_table", > - .jump_or_nop = true, > + .type = jump_label, > .size = JUMP_ENTRY_SIZE, > .orig = JUMP_ORIG_OFFSET, > .new = JUMP_NEW_OFFSET, > }, > { > .sec = "__ex_table", > + .type = exception, > .size = EX_ENTRY_SIZE, > .orig = EX_ORIG_OFFSET, > .new = EX_NEW_OFFSET, > @@ -92,18 +93,14 @@ static int get_alt_entry(struct elf *elf > offset = idx * entry->size; > data = sec->data->d_buf + offset; > > - alt->group = entry->group; > - alt->jump_or_nop = entry->jump_or_nop; > + alt->type = entry->type; > > - if (alt->group) { > - alt->orig_len = *(unsigned char *)(data + entry->orig_len); > - alt->new_len = *(unsigned char *)(data + entry->new_len); > - } > - > - if (entry->group) { > + if (alt->type == alternative) { > unsigned short feature; > unsigned char type; > > + alt->orig_len = *(unsigned char *)(data + entry->orig_len); > + alt->new_len = *(unsigned char *)(data + entry->new_len); > feature = *(unsigned short *)(data + ALT_FEATURE_OFFSET); > type = *(unsigned char *)(data + ALT_TYPE_OFFSET); > > @@ -133,7 +130,7 @@ static int get_alt_entry(struct elf *elf > alt->orig_sec = orig_rela->sym->sec; > alt->orig_off = orig_rela->addend; > > - if (!entry->group || alt->new_len) { > + if (entry->type != alternative || alt->new_len) { > new_rela = find_rela_by_dest(sec, offset + entry->new); > if (!new_rela) { > WARN_FUNC("can't find new rela", > --- a/tools/objtool/special.h > +++ b/tools/objtool/special.h > @@ -21,12 +21,18 @@ > #include <stdbool.h> > #include "elf.h" > > +enum special_type { > + alternative, > + jump_label, > + exception, > +}; > + > struct special_alt { > struct list_head list; > > - bool group; > + enum special_type type; > + > bool skip_orig; > - bool jump_or_nop; > bool static_cpu_has; > > struct section *orig_sec;
Yes, nice improvement on the existing code.
-- Josh
| |