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 <pet...@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) <pet...@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
