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 <[email protected]>
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) <[email protected]>
---
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;
