Re: [PATCH v5 2/7] perf annotate: Add cross arch annotate support

[Russell King - ARM Linux]

On Fri, Aug 19, 2016 at 04:09:51PM +0530, Ravi Bangoria wrote:
> Thanks Russell for reviewing.
> 
> On Friday 19 August 2016 01:20 PM, Russell King - ARM Linux wrote:
> > On Fri, Aug 19, 2016 at 10:59:01AM +0530, Ravi Bangoria wrote:
> >> -static struct ins instructions[] = {
> >> +static struct ins instructions_x86[] = {
> >>  	{ .name = "add",   .ops  = &mov_ops, },
> >>  	{ .name = "addl",  .ops  = &mov_ops, },
> >>  	{ .name = "addq",  .ops  = &mov_ops, },
> >>  	{ .name = "addw",  .ops  = &mov_ops, },
> >>  	{ .name = "and",   .ops  = &mov_ops, },
> >> -#ifdef __arm__
> >> -	{ .name = "b",     .ops  = &jump_ops, }, // might also be a call
> >> -	{ .name = "bcc",   .ops  = &jump_ops, },
> >> -	{ .name = "bcs",   .ops  = &jump_ops, },
> >> -	{ .name = "beq",   .ops  = &jump_ops, },
> >> -	{ .name = "bge",   .ops  = &jump_ops, },
> >> -	{ .name = "bgt",   .ops  = &jump_ops, },
> >> -	{ .name = "bhi",   .ops  = &jump_ops, },
> >> -	{ .name = "bl",    .ops  = &call_ops, },
> >> -	{ .name = "bls",   .ops  = &jump_ops, },
> >> -	{ .name = "blt",   .ops  = &jump_ops, },
> >> -	{ .name = "blx",   .ops  = &call_ops, },
> >> -	{ .name = "bne",   .ops  = &jump_ops, },
> >> -#endif
> > Notice that ARM includes a lot of other instructions from this table,
> > not just those above.
> >
> >>  	{ .name = "bts",   .ops  = &mov_ops, },
> >>  	{ .name = "call",  .ops  = &call_ops, },
> >>  	{ .name = "callq", .ops  = &call_ops, },
> >> @@ -456,6 +444,21 @@ static struct ins instructions[] = {
> >>  	{ .name = "retq",  .ops  = &ret_ops, },
> >>  };
> >>  
> >> +static struct ins instructions_arm[] = {
> >> +	{ .name = "b",     .ops  = &jump_ops, }, /* might also be a call */
> >> +	{ .name = "bcc",   .ops  = &jump_ops, },
> >> +	{ .name = "bcs",   .ops  = &jump_ops, },
> >> +	{ .name = "beq",   .ops  = &jump_ops, },
> >> +	{ .name = "bge",   .ops  = &jump_ops, },
> >> +	{ .name = "bgt",   .ops  = &jump_ops, },
> >> +	{ .name = "bhi",   .ops  = &jump_ops, },
> >> +	{ .name = "bl",    .ops  = &call_ops, },
> >> +	{ .name = "bls",   .ops  = &jump_ops, },
> >> +	{ .name = "blt",   .ops  = &jump_ops, },
> >> +	{ .name = "blx",   .ops  = &call_ops, },
> >> +	{ .name = "bne",   .ops  = &jump_ops, },
> >> +};
> >> +
> > ...
> >> +	if (!strcmp(norm_arch, NORM_X86)) {
> >> +		instructions = instructions_x86;
> >> +		nmemb = ARRAY_SIZE(instructions_x86);
> >> +	} else if (!strcmp(norm_arch, NORM_ARM)) {
> >> +		instructions = instructions_arm;
> >> +		nmemb = ARRAY_SIZE(instructions_arm);
> > But these changes result in _only_ the ones that were in the #if __arm__
> > being matched.  This is wrong.
> >
> > If we want to go that way, we need to add _all_ arm instructions to
> > instructions_arm, not just those within the #if.
> 
> Yes, I've mentioned same in cover letter as well.
> 
> Can I add all x86 instructions for arm as well? If not, can you please
> provide a list of arm instructions that needs to be added here.

If it were me doing a change like this, I'd be trying to preserve the
current behaviour to avoid causing regressions, which would mean
ensuring that all the instructions that were visible before the change
remain visible after the change, even those which are obviously x86
specific but were still in the table anyway.  It then becomes a cleanup
matter later to remove those which aren't relevent, rather than having
to chase around wondering why the tool broke.

I'm afraid I don't have time to look at this (I'm chasing regressions
and bugs in the kernel) so I'd suggest you try to avoid causing
regressions in this tool...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.