lkml.org 
[lkml]   [2016]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 2/7] perf annotate: Add cross arch annotate support
From
Date


On Friday 19 August 2016 04:18 PM, Russell King - ARM Linux wrote:
> 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...
>

Yes Russell, Fair point. Will send a next series.

-Ravi

\
 
 \ /
  Last update: 2016-09-17 09:57    [W:0.134 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site