Re: [PATCH v3 3/4] perf annotate: add powerpc support

[Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>]

Hi Michael,

On Friday 01 July 2016 02:13 PM, Ravi Bangoria wrote:
> Thanks Michael for your suggestion.
>
> On Thursday 30 June 2016 11:51 AM, Michael Ellerman wrote:
>> On Thu, 2016-06-30 at 11:44 +0530, Ravi Bangoria wrote:
>>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>>> index 36a5825..b87eac7 100644
>>> --- a/tools/perf/util/annotate.c
>>> +++ b/tools/perf/util/annotate.c
>>> @@ -476,6 +481,125 @@ static int ins__cmp(const void *a, const void *b)
>> ...
>>> +
>>> +static struct ins *ins__find_powerpc(const char *name)
>>> +{
>>> +    int i;
>>> +    struct ins *ins;
>>> +    struct ins_ops *ops;
>>> +    static struct instructions_powerpc head;
>>> +    static bool list_initialized;
>>> +
>>> +    /*
>>> +     * - Interested only if instruction starts with 'b'.
>>> +     * - Few start with 'b', but aren't branch instructions.
>>> +     * - Let's also ignore instructions involving 'ctr' and
>>> +     *   'tar' since target branch addresses for those can't
>>> +     *   be determined statically.
>>> +     */
>>> +    if (name[0] != 'b'             ||
>>> +        !strncmp(name, "bcd", 3)   ||
>>> +        !strncmp(name, "brinc", 5) ||
>>> +        !strncmp(name, "bper", 4)  ||
>>> +        strstr(name, "ctr")        ||
>>> +        strstr(name, "tar"))
>>> +        return NULL;
>> It would be good if 'bctr' was at least recognised as a branch, even 
>> if we
>> can't determine the target. They are very common.
>
> We can not show arrow for this since we don't know the target location.
> can you please suggest how you intends perf to display bctr?
>
> bctr can be classified into two variants -- 'bctr' and 'bctrl'.
>
> 'bctr' will be considered as jump instruction but jump__parse() won't
> be able to find any target location and hence it will set target to
> UINT64_MAX which transform 'bctr' to 'bctr UINT64_MAX'. This
> looks misleading.
>
> bctrl will be considered as call instruction but call_parse() won't
> be able to find any target function and hence it won't show any
> navigation arrow for this instruction. Which is same as filter it
> beforehand.
>
>> It doesn't look like we have the opcode handy here? Could we get it 
>> somehow?
>> That would make this a *lot* more robust.
>
> objdump prints machine code, but I don't know how difficult that would
> be to parse to get opcode.

Perf uses  --no-show-raw with objdump and hence objdump output does not
show opcodes. So change in current  objdump output may requires changes
in current parsing logic. Additionally I need to change tui as well to show
opcodes. This looks quite more work.

And this patchset is about enabling annotate for cross arch. So if you 
really
need opcode with perf anotate, can we do it separately?

Please let me know your thoughts.

-Ravi

>
> -Ravi
>
>> cheers
>>
>