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

[Josh Poimboeuf]

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@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
> ---
>  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