Re: [PATCH V2 2/3] perf symbols: improve abi compliance in arm mapping symbols handling

[Victor Kamensky]

On 22 January 2015 at 07:29, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> wrote:
> On Wed, Jan 21, 2015 at 08:52:57PM -0800, Victor Kamensky wrote:
>> Both Arm and Aarch64 ELF ABI allow mapping symbols be in from
>> either "$d" or "$d.<any>". But current code that handles mapping
>> symbols only deals with the first, dollar character and a single
>> letter, case.
>>
>> The patch adds handling of the second case with period
>> followed by any characters.
>>
>> Suggested-by: Russell King <linux@xxxxxxxxxxxxxxxx>
>> Suggested-by: Will Deacon <will.deacon@xxxxxxx>
>> Signed-off-by: Victor Kamensky <victor.kamensky@xxxxxxxxxx>
>> Acked-by: Will Deacon <will.deacon@xxxxxxx>
>> Cc: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
>> Cc: Paul Mackerras <paulus@xxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>> Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
>> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
>> Cc: Avi Kivity <avi@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
>> Cc: Anton Blanchard <anton@xxxxxxxxx>
>> Cc: David Ahern <dsahern@xxxxxxxxx>
>> Cc: Dave Martin <Dave.Martin@xxxxxxx>
>> ---
>>  tools/perf/util/symbol-elf.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index 1e188dd..7fd35d6 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -857,17 +857,16 @@ int dso__load_sym(struct dso *dso, struct map *map,
>>                * don't identify functions, so will confuse the profile
>>                * output: */
>>               if (ehdr.e_machine == EM_ARM) {
>> -                     if (!strcmp(elf_name, "$a") ||
>> -                         !strcmp(elf_name, "$d") ||
>> -                         !strcmp(elf_name, "$t"))
>> +                     if (elf_name[0] == '$' && strchr("adt", elf_name[1])
>> +                         && (elf_name[2] == '\0' || elf_name[2] == '.'))
>>                               continue;
>>               }
>>               /* Reject Aarch64 ELF "mapping symbols": these aren't unique and
>>                * don't identify functions, so will confuse the profile
>>                * output: */
>>               if (ehdr.e_machine == EM_AARCH64) {
>> -                     if (!strcmp(elf_name, "$x") ||
>> -                         !strcmp(elf_name, "$d"))
>> +                     if (elf_name[0] == '$' && strchr("dx", elf_name[1])
>> +                         && (elf_name[2] == '\0' || elf_name[2] == '.'))
>>                               continue;
>>               }
>
> Why not fix this first?  Then, all you'd need to do would be to change
> the test for EM_ARM to EM_ARM or EM_AARCH64, and add the 'x' into the
> strchr() string.

In previously discussed [1] I proposed two variants and ask
for people's preferences. Will replied that he did not care and
let me choose that I like.

It seems to me that you more like variant 2 (provided here
for reference again). It would replace patch 1 and patch 2 of
version 2. Please confirm. I don't practically care much and
can use either. I personally preferred variant 1 because in
variant 2 $x will be filtered on ARM V7, but V7 ABI does not
say anything about $x, only Aarch64 does. I.e special symbols
do to match between V7 and V8 exactly. But as it is
already in other places like kallsyms, I agree practically it
may not matter. If you give your node to variant 2 (as below)
I can report this miniseries: Please let me know

Variant 2: