Messages in this thread Patches in this message | | | Date | Wed, 17 Jan 2018 10:19:45 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has() |
| |
On Tue, Jan 16, 2018 at 05:02:56PM -0600, Josh Poimboeuf wrote: > On Tue, Jan 16, 2018 at 03:28:31PM +0100, Peter Zijlstra wrote: > > + if (special_alt->static_feat) > > s/static_feat/static_cpu_has/ ?
done.
> > @@ -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.
> > + 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.
> > --- a/tools/objtool/special.c > > +++ b/tools/objtool/special.c > > @@ -40,6 +40,11 @@ > > #define ALT_FEATURE_OFFSET 8 > > #define ALT_ORIG_LEN_OFFSET 10 > > #define ALT_NEW_LEN_OFFSET 11 > > +#define ALT_PADDING_OFFSET 12 > > +#define ALT_TYPE_OFFSET 13 > > We don't need the ALT_PADDING_OFFSET define (notice we have gaps > already, there are only defines for the ones we use).
Over zealous I suppose, I've taken it out again.
> > + > > +#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;
| |