Re: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has()

[Peter Zijlstra <peterz@infradead.org>]

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;