Re: [PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch()

[James Clark]

On 23/07/2021 20:23, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 23, 2021 at 09:49:44AM +0800, Li Huafei escreveu:
>> On my aarch64 big endian machine, the perf annotate does not work.
>>
>>  # perf annotate
>>   Percent |      Source code & Disassembly of [kernel.kallsyms] for cycles (253 samples, percent: local period)
>>  --------------------------------------------------------------------------------------------------------------
>>   Percent |      Source code & Disassembly of [kernel.kallsyms] for cycles (1 samples, percent: local period)
>>  ------------------------------------------------------------------------------------------------------------
>>   Percent |      Source code & Disassembly of [kernel.kallsyms] for cycles (47 samples, percent: local period)
>>  -------------------------------------------------------------------------------------------------------------
>>  ...
>>
>> This is because the arch_find() function uses the normalized architecture
>> name provided by normalize_arch(), and my machine's architecture name
>> aarch64_be is not normalized to arm64.  Like other architectures such as
>> arm and powerpc, we can fuzzy match the architecture names associated with
>> aarch64.* and normalize them.
> 
> This looks ok modulo fixing the problem and adding that extra pr_err()
> in a single patch, please split this into two.
> 
> Also I fail to see why c34df25b40c2 introduced this problem :-\

I checked the parent commit of c34df25b40c2 and set the architecture to "aarch64_be"
but it doesn't work either, so I also don't see any regressions.

> 
> Can some ARM person ack/review this, please?
> 
> - Arnaldo
>  
>> Fixes: c34df25b40c2 ("perf annotate: Add symbol__annotate function")
>> Signed-off-by: Li Huafei <lihuafei1@xxxxxxxxxx>
>> ---
>>  tools/perf/util/annotate.c | 4 +++-
>>  tools/perf/util/env.c      | 2 +-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index aa04a3655236..cb280de3369f 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -2192,8 +2192,10 @@ int symbol__annotate(struct map_symbol *ms, struct evsel *evsel,
>>  		return errno;
>>  
>>  	args.arch = arch = arch__find(arch_name);
>> -	if (arch == NULL)
>> +	if (arch == NULL) {
>> +		pr_err("%s: unsupported arch %s\n", __func__, arch_name);
>>  		return ENOTSUP;
>> +	}
>>  
>>  	if (parch)
>>  		*parch = arch;
>> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
>> index cec2e6cad8aa..a91da1e9b201 100644
>> --- a/tools/perf/util/env.c
>> +++ b/tools/perf/util/env.c
>> @@ -349,7 +349,7 @@ static const char *normalize_arch(char *arch)
>>  		return "x86";
>>  	if (!strcmp(arch, "sun4u") || !strncmp(arch, "sparc", 5))
>>  		return "sparc";
>> -	if (!strcmp(arch, "aarch64") || !strcmp(arch, "arm64"))
>> +	if (!strncmp(arch, "aarch64", 7) || !strcmp(arch, "arm64"))

This seems ok to me, but a quick google shows some references to "arm64_be".
I don't know if this could ever get into perf, but most of the other ones
search for prefixes, so it probably doesn't hurt to do strcmp(arch, "arm64", 5)
as well.

Thanks
James

>>  		return "arm64";
>>  	if (!strncmp(arch, "arm", 3) || !strcmp(arch, "sa110"))
>>  		return "arm";
>> -- 
>> 2.17.1
>>
>