[PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[Victor Kamensky]

Aarch64 ELF files use mapping symbols with special names $x, $d
to identify regions of Aarch64 code (see Aarch64 ELF ABI - "ARM
IHI 0056B", section "4.5.4 Mapping symbols").

The patch filters out these symbols at load time, similar to
"696b97a perf symbols: Ignore mapping symbols on ARM" changes
done for ARM before V8.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Avi Kivity <avi@cloudius-systems.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Anton Blanchard <anton@samba.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Dave Martin <Dave.Martin@arm.com>
---
 tools/perf/util/symbol-elf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 06fcd1b..1e188dd 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -862,6 +862,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
 			    !strcmp(elf_name, "$t"))
 				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"))
+				continue;
+		}
 
 		if (runtime_ss->opdsec && sym.st_shndx == runtime_ss->opdidx) {
 			u32 offset = sym.st_value - syms_ss->opdshdr.sh_addr;
-- 
1.9.3

[PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[Victor Kamensky]

Aarch64 ELF files use mapping symbols with special names $x, $d
to identify regions of Aarch64 code (see Aarch64 ELF ABI - "ARM
IHI 0056B", section "4.5.4 Mapping symbols").

The patch filters out these symbols at load time, similar to
"696b97a perf symbols: Ignore mapping symbols on ARM" changes
done for ARM before V8.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Avi Kivity <avi@cloudius-systems.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Anton Blanchard <anton@samba.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Dave Martin <Dave.Martin@arm.com>
---
 tools/perf/util/symbol-elf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 06fcd1b..1e188dd 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -862,6 +862,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
 			    !strcmp(elf_name, "$t"))
 				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"))
+				continue;
+		}
 
 		if (runtime_ss->opdsec && sym.st_shndx == runtime_ss->opdidx) {
 			u32 offset = sym.st_value - syms_ss->opdshdr.sh_addr;
-- 
1.9.3

[PATCH 2/2] perf symbols: debuglink should take symfs option into account

[Victor Kamensky]

Currently code that tries to read corresponding debug symbol
file from .gnu_debuglink section (DSO_BINARY_TYPE__DEBUGLINK)
does not take in account symfs option, so filename__read_debuglink
function cannot open ELF file, if symfs option is used.

Fix is to add proper handling of symfs as it is done in other
places: use __symbol__join_symfs function to get real file name
of target ELF file.

Created malloced copy of target filename in symfs before passing
it to __symbol__join_symfs function because filename will be
modified by it if corresponding debuglink is found.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Waiman Long <Waiman.Long@hp.com>
Cc: David Ahern <dsahern@gmail.com>
---
 tools/perf/util/dso.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 45be944..6a2f663 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -42,19 +42,30 @@ int dso__read_binary_type_filename(const struct dso *dso,
 	size_t len;
 
 	switch (type) {
-	case DSO_BINARY_TYPE__DEBUGLINK: {
+	case DSO_BINARY_TYPE__DEBUGLINK:
+	{
 		char *debuglink;
-
-		strncpy(filename, dso->long_name, size);
-		debuglink = filename + dso->long_name_len;
-		while (debuglink != filename && *debuglink != '/')
-			debuglink--;
-		if (*debuglink == '/')
-			debuglink++;
-		ret = filename__read_debuglink(dso->long_name, debuglink,
-					       size - (debuglink - filename));
-		}
+		char *filename_copy;
+
+		filename_copy = malloc(PATH_MAX);
+		if (filename_copy) {
+			len = __symbol__join_symfs(filename, size,
+						   dso->long_name);
+			strncpy(filename_copy, filename, PATH_MAX);
+			debuglink = filename + len;
+			while (debuglink != filename && *debuglink != '/')
+				debuglink--;
+			if (*debuglink == '/')
+				debuglink++;
+			ret = filename__read_debuglink(filename_copy, debuglink,
+						       size - (debuglink -
+							       filename));
+			free(filename_copy);
+		} else
+			ret = -1;
 		break;
+	}
+
 	case DSO_BINARY_TYPE__BUILD_ID_CACHE:
 		/* skip the locally configured cache if a symfs is given */
 		if (symbol_conf.symfs[0] ||
-- 
1.9.3

[PATCH 2/2] perf symbols: debuglink should take symfs option into account

[Victor Kamensky]

Currently code that tries to read corresponding debug symbol
file from .gnu_debuglink section (DSO_BINARY_TYPE__DEBUGLINK)
does not take in account symfs option, so filename__read_debuglink
function cannot open ELF file, if symfs option is used.

Fix is to add proper handling of symfs as it is done in other
places: use __symbol__join_symfs function to get real file name
of target ELF file.

Created malloced copy of target filename in symfs before passing
it to __symbol__join_symfs function because filename will be
modified by it if corresponding debuglink is found.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Waiman Long <Waiman.Long@hp.com>
Cc: David Ahern <dsahern@gmail.com>
---
 tools/perf/util/dso.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 45be944..6a2f663 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -42,19 +42,30 @@ int dso__read_binary_type_filename(const struct dso *dso,
 	size_t len;
 
 	switch (type) {
-	case DSO_BINARY_TYPE__DEBUGLINK: {
+	case DSO_BINARY_TYPE__DEBUGLINK:
+	{
 		char *debuglink;
-
-		strncpy(filename, dso->long_name, size);
-		debuglink = filename + dso->long_name_len;
-		while (debuglink != filename && *debuglink != '/')
-			debuglink--;
-		if (*debuglink == '/')
-			debuglink++;
-		ret = filename__read_debuglink(dso->long_name, debuglink,
-					       size - (debuglink - filename));
-		}
+		char *filename_copy;
+
+		filename_copy = malloc(PATH_MAX);
+		if (filename_copy) {
+			len = __symbol__join_symfs(filename, size,
+						   dso->long_name);
+			strncpy(filename_copy, filename, PATH_MAX);
+			debuglink = filename + len;
+			while (debuglink != filename && *debuglink != '/')
+				debuglink--;
+			if (*debuglink == '/')
+				debuglink++;
+			ret = filename__read_debuglink(filename_copy, debuglink,
+						       size - (debuglink -
+							       filename));
+			free(filename_copy);
+		} else
+			ret = -1;
 		break;
+	}
+
 	case DSO_BINARY_TYPE__BUILD_ID_CACHE:
 		/* skip the locally configured cache if a symfs is given */
 		if (symbol_conf.symfs[0] ||
-- 
1.9.3

Re: [PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[Will Deacon]

On Tue, Jan 13, 2015 at 04:59:03PM +0000, Victor Kamensky wrote:
> Aarch64 ELF files use mapping symbols with special names $x, $d
> to identify regions of Aarch64 code (see Aarch64 ELF ABI - "ARM
> IHI 0056B", section "4.5.4 Mapping symbols").
> 
> The patch filters out these symbols at load time, similar to
> "696b97a perf symbols: Ignore mapping symbols on ARM" changes
> done for ARM before V8.
> 
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Avi Kivity <avi@cloudius-systems.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Anton Blanchard <anton@samba.org>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Dave Martin <Dave.Martin@arm.com>
> ---
>  tools/perf/util/symbol-elf.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 06fcd1b..1e188dd 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -862,6 +862,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
>  			    !strcmp(elf_name, "$t"))
>  				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"))
> +				continue;
> +		}

Do we need to skip $x.* and $d.* too? I doubt GCC generates them, but they
are permitted by the ELF ABI.

Will

[PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[Will Deacon]

On Tue, Jan 13, 2015 at 04:59:03PM +0000, Victor Kamensky wrote:
> Aarch64 ELF files use mapping symbols with special names $x, $d
> to identify regions of Aarch64 code (see Aarch64 ELF ABI - "ARM
> IHI 0056B", section "4.5.4 Mapping symbols").
> 
> The patch filters out these symbols at load time, similar to
> "696b97a perf symbols: Ignore mapping symbols on ARM" changes
> done for ARM before V8.
> 
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Avi Kivity <avi@cloudius-systems.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Anton Blanchard <anton@samba.org>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Dave Martin <Dave.Martin@arm.com>
> ---
>  tools/perf/util/symbol-elf.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 06fcd1b..1e188dd 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -862,6 +862,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
>  			    !strcmp(elf_name, "$t"))
>  				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"))
> +				continue;
> +		}

Do we need to skip $x.* and $d.* too? I doubt GCC generates them, but they
are permitted by the ELF ABI.

Will

Re: [PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[Victor Kamensky]

On 14 January 2015 at 03:22, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jan 13, 2015 at 04:59:03PM +0000, Victor Kamensky wrote:
>> Aarch64 ELF files use mapping symbols with special names $x, $d
>> to identify regions of Aarch64 code (see Aarch64 ELF ABI - "ARM
>> IHI 0056B", section "4.5.4 Mapping symbols").
>>
>> The patch filters out these symbols at load time, similar to
>> "696b97a perf symbols: Ignore mapping symbols on ARM" changes
>> done for ARM before V8.
>>
>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Avi Kivity <avi@cloudius-systems.com>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Anton Blanchard <anton@samba.org>
>> Cc: David Ahern <dsahern@gmail.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Dave Martin <Dave.Martin@arm.com>
>> ---
>>  tools/perf/util/symbol-elf.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index 06fcd1b..1e188dd 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -862,6 +862,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
>>                           !strcmp(elf_name, "$t"))
>>                               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"))
>> +                             continue;
>> +             }
>
> Do we need to skip $x.* and $d.* too? I doubt GCC generates them, but they
> are permitted by the ELF ABI.

Fair enough. But I think it would need to be done for both EM_ARM and
EM_AARCH64. My above patch follows EM_ARM current case. Also it
seems that it would be quite rare case, as ABI suggests symbols
with dot notation should be used in manual asm where assembler does not
support multiple definitions of the same symbol.

Will something like the following suffice? I can add that as separate follow up
patch in this small series. Tested with explicit "$x.func1" symbol introduced in
manual asm.

>From fed6caab410ddcaf487ff23a3908eca129e50b89 Mon Sep 17 00:00:00 2001
From: Victor Kamensky <victor.kamensky@linaro.org>
Date: Wed, 14 Jan 2015 07:42:41 -0800
Subject: [PATCH 3/3] perf symbols: improve abi compliance in arm mapping
 symbols handling

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: Will Deacon <will.deacon@arm.com>
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 tools/perf/util/symbol-elf.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 1e188dd..ae92c27 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -859,7 +859,10 @@ int dso__load_sym(struct dso *dso, struct map *map,
         if (ehdr.e_machine == EM_ARM) {
             if (!strcmp(elf_name, "$a") ||
                 !strcmp(elf_name, "$d") ||
-                !strcmp(elf_name, "$t"))
+                !strcmp(elf_name, "$t") ||
+                !strncmp(elf_name, "$a.", 3) ||
+                !strncmp(elf_name, "$d.", 3) ||
+                !strncmp(elf_name, "$t.", 3))
                 continue;
         }
         /* Reject Aarch64 ELF "mapping symbols": these aren't unique and
@@ -867,7 +870,9 @@ int dso__load_sym(struct dso *dso, struct map *map,
          * output: */
         if (ehdr.e_machine == EM_AARCH64) {
             if (!strcmp(elf_name, "$x") ||
-                !strcmp(elf_name, "$d"))
+                !strcmp(elf_name, "$d") ||
+                !strncmp(elf_name, "$x.", 3) ||
+                !strncmp(elf_name, "$d.", 3))
                 continue;
         }

-- 
1.9.3

Thanks,
Victor

> Will

[PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[Victor Kamensky]

On 14 January 2015 at 03:22, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jan 13, 2015 at 04:59:03PM +0000, Victor Kamensky wrote:
>> Aarch64 ELF files use mapping symbols with special names $x, $d
>> to identify regions of Aarch64 code (see Aarch64 ELF ABI - "ARM
>> IHI 0056B", section "4.5.4 Mapping symbols").
>>
>> The patch filters out these symbols at load time, similar to
>> "696b97a perf symbols: Ignore mapping symbols on ARM" changes
>> done for ARM before V8.
>>
>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Avi Kivity <avi@cloudius-systems.com>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Anton Blanchard <anton@samba.org>
>> Cc: David Ahern <dsahern@gmail.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Dave Martin <Dave.Martin@arm.com>
>> ---
>>  tools/perf/util/symbol-elf.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index 06fcd1b..1e188dd 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -862,6 +862,14 @@ int dso__load_sym(struct dso *dso, struct map *map,
>>                           !strcmp(elf_name, "$t"))
>>                               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"))
>> +                             continue;
>> +             }
>
> Do we need to skip $x.* and $d.* too? I doubt GCC generates them, but they
> are permitted by the ELF ABI.

Fair enough. But I think it would need to be done for both EM_ARM and
EM_AARCH64. My above patch follows EM_ARM current case. Also it
seems that it would be quite rare case, as ABI suggests symbols
with dot notation should be used in manual asm where assembler does not
support multiple definitions of the same symbol.

Will something like the following suffice? I can add that as separate follow up
patch in this small series. Tested with explicit "$x.func1" symbol introduced in
manual asm.

>From fed6caab410ddcaf487ff23a3908eca129e50b89 Mon Sep 17 00:00:00 2001
From: Victor Kamensky <victor.kamensky@linaro.org>
Date: Wed, 14 Jan 2015 07:42:41 -0800
Subject: [PATCH 3/3] perf symbols: improve abi compliance in arm mapping
 symbols handling

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: Will Deacon <will.deacon@arm.com>
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 tools/perf/util/symbol-elf.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 1e188dd..ae92c27 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -859,7 +859,10 @@ int dso__load_sym(struct dso *dso, struct map *map,
         if (ehdr.e_machine == EM_ARM) {
             if (!strcmp(elf_name, "$a") ||
                 !strcmp(elf_name, "$d") ||
-                !strcmp(elf_name, "$t"))
+                !strcmp(elf_name, "$t") ||
+                !strncmp(elf_name, "$a.", 3) ||
+                !strncmp(elf_name, "$d.", 3) ||
+                !strncmp(elf_name, "$t.", 3))
                 continue;
         }
         /* Reject Aarch64 ELF "mapping symbols": these aren't unique and
@@ -867,7 +870,9 @@ int dso__load_sym(struct dso *dso, struct map *map,
          * output: */
         if (ehdr.e_machine == EM_AARCH64) {
             if (!strcmp(elf_name, "$x") ||
-                !strcmp(elf_name, "$d"))
+                !strcmp(elf_name, "$d") ||
+                !strncmp(elf_name, "$x.", 3) ||
+                !strncmp(elf_name, "$d.", 3))
                 continue;
         }

-- 
1.9.3

Thanks,
Victor

> Will

Re: [PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[Russell King - ARM Linux]

On Wed, Jan 14, 2015 at 10:38:38AM -0800, Victor Kamensky wrote:
> >From fed6caab410ddcaf487ff23a3908eca129e50b89 Mon Sep 17 00:00:00 2001
> From: Victor Kamensky <victor.kamensky@linaro.org>
> Date: Wed, 14 Jan 2015 07:42:41 -0800
> Subject: [PATCH 3/3] perf symbols: improve abi compliance in arm mapping
>  symbols handling
> 
> 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: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>

I wonder if it would make more sense to re-use the "is_arm_mapping_symbol"
thing which we have in kernel/module.c and scripts/kallsyms.c - it
seems silly to re-invent code which we already have to detect these
symbols.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[Russell King - ARM Linux]

On Wed, Jan 14, 2015 at 10:38:38AM -0800, Victor Kamensky wrote:
> >From fed6caab410ddcaf487ff23a3908eca129e50b89 Mon Sep 17 00:00:00 2001
> From: Victor Kamensky <victor.kamensky@linaro.org>
> Date: Wed, 14 Jan 2015 07:42:41 -0800
> Subject: [PATCH 3/3] perf symbols: improve abi compliance in arm mapping
>  symbols handling
> 
> 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: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>

I wonder if it would make more sense to re-use the "is_arm_mapping_symbol"
thing which we have in kernel/module.c and scripts/kallsyms.c - it
seems silly to re-invent code which we already have to detect these
symbols.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[Victor Kamensky]

On 14 January 2015 at 16:03, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Jan 14, 2015 at 10:38:38AM -0800, Victor Kamensky wrote:
>> >From fed6caab410ddcaf487ff23a3908eca129e50b89 Mon Sep 17 00:00:00 2001
>> From: Victor Kamensky <victor.kamensky@linaro.org>
>> Date: Wed, 14 Jan 2015 07:42:41 -0800
>> Subject: [PATCH 3/3] perf symbols: improve abi compliance in arm mapping
>>  symbols handling
>>
>> 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: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>
> I wonder if it would make more sense to re-use the "is_arm_mapping_symbol"
> thing which we have in kernel/module.c and scripts/kallsyms.c - it
> seems silly to re-invent code which we already have to detect these
> symbols.

Thanks for pointing this out. I did not know about
"is_arm_mapping_symbol" function.

Do you suggest we copy one of those functions into tools/perf?
Since tools/perf is separate user-land utility I don't see easy way
how can we reuse those directly.

Also those functions check for mapping symbols
seems to be more efficient that one I come with, for example
one from scripts/kallsyms.c

static inline int is_arm_mapping_symbol(const char *str)
{
    return str[0] == '$' && strchr("axtd", str[1])
           && (str[2] == '\0' || str[2] == '.');
}

But it seems that they are somewhat accurate: because they bundle
EM_ARM and EM_AARCH64 into one case. According to ABIs
for EM_ARM we have $a, $t, $d, $a.<any>, $t.<any>, $d.<any>;
and for EM_AARCH64 we have $x, $d, $x.<any>, $d.<any>.

How about the following two variants of the patch. It follows the same
mapping handling logic as in other 3 copies of is_arm_mapping_symbol
function in kernel, but it still separate copy in tools/perf code. Personally I
prefer variant 1, but I am fine with variant 2 too, because practically it
will be OK.

Variant 1 (as addition to this patch, as above):

>From e08d348bd72d406d8939993d266729d805577c4b Mon Sep 17 00:00:00 2001
From: Victor Kamensky <victor.kamensky@linaro.org>
Date: Wed, 14 Jan 2015 07:42:41 -0800
Subject: [PATCH 3/3] perf symbols: improve abi compliance in arm mapping
 symbols handling

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@arm.linux.org.uk>
Suggested-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 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..a038c98 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;
         }

-- 
1.9.3

Variant 2 instead of patch posted with current subject:

 From c8d08ebddc61203daf21b17c891c26c1d08e14f1 Mon Sep 17 00:00:00 2001
From: Victor Kamensky <victor.kamensky@linaro.org>
Date: Mon, 12 Jan 2015 14:13:36 -0800
Subject: [PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

Aarch64 ELF files use mapping symbols with special names $x, $d
to identify regions of Aarch64 code (see Aarch64 ELF ABI - "ARM
IHI 0056B", section "4.5.4 Mapping symbols").

The patch filters out these symbols at load time, similar to
"696b97a perf symbols: Ignore mapping symbols on ARM" changes
done for ARM before V8.

Also added handling of mapping symbols that has format
"$d.<any>" and similar for both cases.

Note we are not making difference between EM_ARM and
EM_AARCH64 mapping symbols instead code handles superset
of both.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Avi Kivity <avi@cloudius-systems.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Anton Blanchard <anton@samba.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Dave Martin <Dave.Martin@arm.com>
---
 tools/perf/util/symbol-elf.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 06fcd1b..b2eb0f9 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -856,10 +856,9 @@ int dso__load_sym(struct dso *dso, struct map *map,
         /* Reject ARM ELF "mapping symbols": these aren't unique and
          * 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 (ehdr.e_machine == EM_ARM || ehdr.e_machine == EM_AARCH64) {
+                        if (elf_name[0] == '$' && strchr("adtx", elf_name[1])
+                        && (elf_name[2] == '\0' || elf_name[2] == '.'))
                 continue;
         }

-- 
1.9.3

Thanks,
Victor

> --
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.

[PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[Victor Kamensky]

On 14 January 2015 at 16:03, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Jan 14, 2015 at 10:38:38AM -0800, Victor Kamensky wrote:
>> >From fed6caab410ddcaf487ff23a3908eca129e50b89 Mon Sep 17 00:00:00 2001
>> From: Victor Kamensky <victor.kamensky@linaro.org>
>> Date: Wed, 14 Jan 2015 07:42:41 -0800
>> Subject: [PATCH 3/3] perf symbols: improve abi compliance in arm mapping
>>  symbols handling
>>
>> 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: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
>
> I wonder if it would make more sense to re-use the "is_arm_mapping_symbol"
> thing which we have in kernel/module.c and scripts/kallsyms.c - it
> seems silly to re-invent code which we already have to detect these
> symbols.

Thanks for pointing this out. I did not know about
"is_arm_mapping_symbol" function.

Do you suggest we copy one of those functions into tools/perf?
Since tools/perf is separate user-land utility I don't see easy way
how can we reuse those directly.

Also those functions check for mapping symbols
seems to be more efficient that one I come with, for example
one from scripts/kallsyms.c

static inline int is_arm_mapping_symbol(const char *str)
{
    return str[0] == '$' && strchr("axtd", str[1])
           && (str[2] == '\0' || str[2] == '.');
}

But it seems that they are somewhat accurate: because they bundle
EM_ARM and EM_AARCH64 into one case. According to ABIs
for EM_ARM we have $a, $t, $d, $a.<any>, $t.<any>, $d.<any>;
and for EM_AARCH64 we have $x, $d, $x.<any>, $d.<any>.

How about the following two variants of the patch. It follows the same
mapping handling logic as in other 3 copies of is_arm_mapping_symbol
function in kernel, but it still separate copy in tools/perf code. Personally I
prefer variant 1, but I am fine with variant 2 too, because practically it
will be OK.

Variant 1 (as addition to this patch, as above):

>From e08d348bd72d406d8939993d266729d805577c4b Mon Sep 17 00:00:00 2001
From: Victor Kamensky <victor.kamensky@linaro.org>
Date: Wed, 14 Jan 2015 07:42:41 -0800
Subject: [PATCH 3/3] perf symbols: improve abi compliance in arm mapping
 symbols handling

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@arm.linux.org.uk>
Suggested-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 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..a038c98 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;
         }

-- 
1.9.3

Variant 2 instead of patch posted with current subject:

 From c8d08ebddc61203daf21b17c891c26c1d08e14f1 Mon Sep 17 00:00:00 2001
From: Victor Kamensky <victor.kamensky@linaro.org>
Date: Mon, 12 Jan 2015 14:13:36 -0800
Subject: [PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

Aarch64 ELF files use mapping symbols with special names $x, $d
to identify regions of Aarch64 code (see Aarch64 ELF ABI - "ARM
IHI 0056B", section "4.5.4 Mapping symbols").

The patch filters out these symbols at load time, similar to
"696b97a perf symbols: Ignore mapping symbols on ARM" changes
done for ARM before V8.

Also added handling of mapping symbols that has format
"$d.<any>" and similar for both cases.

Note we are not making difference between EM_ARM and
EM_AARCH64 mapping symbols instead code handles superset
of both.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Avi Kivity <avi@cloudius-systems.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Anton Blanchard <anton@samba.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Dave Martin <Dave.Martin@arm.com>
---
 tools/perf/util/symbol-elf.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 06fcd1b..b2eb0f9 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -856,10 +856,9 @@ int dso__load_sym(struct dso *dso, struct map *map,
         /* Reject ARM ELF "mapping symbols": these aren't unique and
          * 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 (ehdr.e_machine == EM_ARM || ehdr.e_machine == EM_AARCH64) {
+                        if (elf_name[0] == '$' && strchr("adtx", elf_name[1])
+                        && (elf_name[2] == '\0' || elf_name[2] == '.'))
                 continue;
         }

-- 
1.9.3

Thanks,
Victor

> --
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.

Re: [PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[Will Deacon]

On Thu, Jan 15, 2015 at 02:51:06AM +0000, Victor Kamensky wrote:
> On 14 January 2015 at 16:03, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Wed, Jan 14, 2015 at 10:38:38AM -0800, Victor Kamensky wrote:
> >> >From fed6caab410ddcaf487ff23a3908eca129e50b89 Mon Sep 17 00:00:00 2001
> >> From: Victor Kamensky <victor.kamensky@linaro.org>
> >> Date: Wed, 14 Jan 2015 07:42:41 -0800
> >> Subject: [PATCH 3/3] perf symbols: improve abi compliance in arm mapping
> >>  symbols handling
> >>
> >> 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: Will Deacon <will.deacon@arm.com>
> >> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> >
> > I wonder if it would make more sense to re-use the "is_arm_mapping_symbol"
> > thing which we have in kernel/module.c and scripts/kallsyms.c - it
> > seems silly to re-invent code which we already have to detect these
> > symbols.
> 
> Thanks for pointing this out. I did not know about
> "is_arm_mapping_symbol" function.
> 
> Do you suggest we copy one of those functions into tools/perf?
> Since tools/perf is separate user-land utility I don't see easy way
> how can we reuse those directly.
> 
> Also those functions check for mapping symbols
> seems to be more efficient that one I come with, for example
> one from scripts/kallsyms.c
> 
> static inline int is_arm_mapping_symbol(const char *str)
> {
>     return str[0] == '$' && strchr("axtd", str[1])
>            && (str[2] == '\0' || str[2] == '.');
> }
> 
> But it seems that they are somewhat accurate: because they bundle
> EM_ARM and EM_AARCH64 into one case. According to ABIs
> for EM_ARM we have $a, $t, $d, $a.<any>, $t.<any>, $d.<any>;
> and for EM_AARCH64 we have $x, $d, $x.<any>, $d.<any>.
> 
> How about the following two variants of the patch. It follows the same
> mapping handling logic as in other 3 copies of is_arm_mapping_symbol
> function in kernel, but it still separate copy in tools/perf code. Personally I
> prefer variant 1, but I am fine with variant 2 too, because practically it
> will be OK.

They both look fine to me; pick your favourite.

Acked-by: Will Deacon <will.deacon@arm.com>

Will

> Variant 1 (as addition to this patch, as above):
> 
> From e08d348bd72d406d8939993d266729d805577c4b Mon Sep 17 00:00:00 2001
> From: Victor Kamensky <victor.kamensky@linaro.org>
> Date: Wed, 14 Jan 2015 07:42:41 -0800
> Subject: [PATCH 3/3] perf symbols: improve abi compliance in arm mapping
>  symbols handling
> 
> 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@arm.linux.org.uk>
> Suggested-by: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
>  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..a038c98 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;
>          }
> 
> -- 
> 1.9.3
> 
> Variant 2 instead of patch posted with current subject:
> 
>  From c8d08ebddc61203daf21b17c891c26c1d08e14f1 Mon Sep 17 00:00:00 2001
> From: Victor Kamensky <victor.kamensky@linaro.org>
> Date: Mon, 12 Jan 2015 14:13:36 -0800
> Subject: [PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64
> 
> Aarch64 ELF files use mapping symbols with special names $x, $d
> to identify regions of Aarch64 code (see Aarch64 ELF ABI - "ARM
> IHI 0056B", section "4.5.4 Mapping symbols").
> 
> The patch filters out these symbols at load time, similar to
> "696b97a perf symbols: Ignore mapping symbols on ARM" changes
> done for ARM before V8.
> 
> Also added handling of mapping symbols that has format
> "$d.<any>" and similar for both cases.
> 
> Note we are not making difference between EM_ARM and
> EM_AARCH64 mapping symbols instead code handles superset
> of both.
> 
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Avi Kivity <avi@cloudius-systems.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Anton Blanchard <anton@samba.org>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Dave Martin <Dave.Martin@arm.com>
> ---
>  tools/perf/util/symbol-elf.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 06fcd1b..b2eb0f9 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -856,10 +856,9 @@ int dso__load_sym(struct dso *dso, struct map *map,
>          /* Reject ARM ELF "mapping symbols": these aren't unique and
>           * 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 (ehdr.e_machine == EM_ARM || ehdr.e_machine == EM_AARCH64) {
> +                        if (elf_name[0] == '$' && strchr("adtx", elf_name[1])
> +                        && (elf_name[2] == '\0' || elf_name[2] == '.'))
>                  continue;
>          }
> 
> -- 
> 1.9.3
> 
> Thanks,
> Victor
> 
> > --
> > FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> > according to speedtest.net.
> 

[PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64

[Will Deacon]

On Thu, Jan 15, 2015 at 02:51:06AM +0000, Victor Kamensky wrote:
> On 14 January 2015 at 16:03, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Wed, Jan 14, 2015 at 10:38:38AM -0800, Victor Kamensky wrote:
> >> >From fed6caab410ddcaf487ff23a3908eca129e50b89 Mon Sep 17 00:00:00 2001
> >> From: Victor Kamensky <victor.kamensky@linaro.org>
> >> Date: Wed, 14 Jan 2015 07:42:41 -0800
> >> Subject: [PATCH 3/3] perf symbols: improve abi compliance in arm mapping
> >>  symbols handling
> >>
> >> 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: Will Deacon <will.deacon@arm.com>
> >> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> >
> > I wonder if it would make more sense to re-use the "is_arm_mapping_symbol"
> > thing which we have in kernel/module.c and scripts/kallsyms.c - it
> > seems silly to re-invent code which we already have to detect these
> > symbols.
> 
> Thanks for pointing this out. I did not know about
> "is_arm_mapping_symbol" function.
> 
> Do you suggest we copy one of those functions into tools/perf?
> Since tools/perf is separate user-land utility I don't see easy way
> how can we reuse those directly.
> 
> Also those functions check for mapping symbols
> seems to be more efficient that one I come with, for example
> one from scripts/kallsyms.c
> 
> static inline int is_arm_mapping_symbol(const char *str)
> {
>     return str[0] == '$' && strchr("axtd", str[1])
>            && (str[2] == '\0' || str[2] == '.');
> }
> 
> But it seems that they are somewhat accurate: because they bundle
> EM_ARM and EM_AARCH64 into one case. According to ABIs
> for EM_ARM we have $a, $t, $d, $a.<any>, $t.<any>, $d.<any>;
> and for EM_AARCH64 we have $x, $d, $x.<any>, $d.<any>.
> 
> How about the following two variants of the patch. It follows the same
> mapping handling logic as in other 3 copies of is_arm_mapping_symbol
> function in kernel, but it still separate copy in tools/perf code. Personally I
> prefer variant 1, but I am fine with variant 2 too, because practically it
> will be OK.

They both look fine to me; pick your favourite.

Acked-by: Will Deacon <will.deacon@arm.com>

Will

> Variant 1 (as addition to this patch, as above):
> 
> From e08d348bd72d406d8939993d266729d805577c4b Mon Sep 17 00:00:00 2001
> From: Victor Kamensky <victor.kamensky@linaro.org>
> Date: Wed, 14 Jan 2015 07:42:41 -0800
> Subject: [PATCH 3/3] perf symbols: improve abi compliance in arm mapping
>  symbols handling
> 
> 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@arm.linux.org.uk>
> Suggested-by: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
>  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..a038c98 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;
>          }
> 
> -- 
> 1.9.3
> 
> Variant 2 instead of patch posted with current subject:
> 
>  From c8d08ebddc61203daf21b17c891c26c1d08e14f1 Mon Sep 17 00:00:00 2001
> From: Victor Kamensky <victor.kamensky@linaro.org>
> Date: Mon, 12 Jan 2015 14:13:36 -0800
> Subject: [PATCH 1/2] perf symbols: Ignore mapping symbols on aarch64
> 
> Aarch64 ELF files use mapping symbols with special names $x, $d
> to identify regions of Aarch64 code (see Aarch64 ELF ABI - "ARM
> IHI 0056B", section "4.5.4 Mapping symbols").
> 
> The patch filters out these symbols at load time, similar to
> "696b97a perf symbols: Ignore mapping symbols on ARM" changes
> done for ARM before V8.
> 
> Also added handling of mapping symbols that has format
> "$d.<any>" and similar for both cases.
> 
> Note we are not making difference between EM_ARM and
> EM_AARCH64 mapping symbols instead code handles superset
> of both.
> 
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Avi Kivity <avi@cloudius-systems.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Anton Blanchard <anton@samba.org>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Dave Martin <Dave.Martin@arm.com>
> ---
>  tools/perf/util/symbol-elf.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 06fcd1b..b2eb0f9 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -856,10 +856,9 @@ int dso__load_sym(struct dso *dso, struct map *map,
>          /* Reject ARM ELF "mapping symbols": these aren't unique and
>           * 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 (ehdr.e_machine == EM_ARM || ehdr.e_machine == EM_AARCH64) {
> +                        if (elf_name[0] == '$' && strchr("adtx", elf_name[1])
> +                        && (elf_name[2] == '\0' || elf_name[2] == '.'))
>                  continue;
>          }
> 
> -- 
> 1.9.3
> 
> Thanks,
> Victor
> 
> > --
> > FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> > according to speedtest.net.
> 

Re: [PATCH 2/2] perf symbols: debuglink should take symfs option into account

[Victor Kamensky]

Hi Guys,

If it helps to look at below patch, here is a test case
for failure that below patch addresses.

Run on target with root NFS mounted in this case. Current
target is ARM TC2 board.

But I am pretty sure the same sequence will fail on any other
device that is built in cross compiled way, with .gnu_debuglink
split debug symbols and with and perf used with --symfs option:

On target device built split debug executable with .gnu_debuglink
section and collect perf data:

root@genericarmv7a:~/tests/hang# cat hang.c
void hang(void)
{
    while(1);
}

int main (void)
{
    hang();
    return 0;
}
root@genericarmv7a:~/tests/hang# gcc -o hang -g hang.c
root@genericarmv7a:~/tests/hang# cp hang hang.debug
root@genericarmv7a:~/tests/hang# strip --only-keep-debug hang.debug
root@genericarmv7a:~/tests/hang# objcopy --add-gnu-debuglink=hang.debug hang
root@genericarmv7a:~/tests/hang# strip hang
root@genericarmv7a:~/tests/hang# readelf -S hang | grep gnu_debuglink
  [27] .gnu_debuglink    PROGBITS        00000000 00060b 000010 00      0   0  1
root@genericarmv7a:~/tests/hang# readelf -x 27 hang

Hex dump of section '.gnu_debuglink':
  0x00000000 68616e67 2e646562 75670000 ee7e9545 hang.debug...~.E

root@genericarmv7a:~/tests/hang# perf record -e cycles --no-buildid-cache ./hang
^C[ perf record: Woken up 6 times to write data ]
[ perf record: Captured and wrote 1.448 MB perf.data (~63258 samples) ]

root@genericarmv7a:~/tests/hang# chmod a+r perf.data


On the host trying to decode perf result without proposed fix
using --symfs option to point to target rootfs. Note failure of
decode symbol for hang function.

[kamensky@coreos-lnx2 hang]$ ls -a
.  ..  hang  hang.c  hang.debug  perf.data
[kamensky@coreos-lnx2 hang]$ $L/tools/perf/perf report --stdio --symfs
/wd2/nfs/rootfs_32_le_tc2_20141218 | head -10
[kernel.kallsyms] with build id
c70bcdc8d97df0dcded9984e3ada2b6ff5a69510 not found, continuing without
symbols
no symbols found in /home/root/tests/hang/hang, maybe install a debug package?
# To display the perf.data header info, please use
--header/--header-only options.
#
# Samples: 37K of event 'cycles'
# Event count (approx.): 9469365389
#
# Overhead  Command  Shared Object      Symbol
# ........  .......  .................  ......................
#
    99.97%  hang     hang               [.] 0x00000000000003ec <--
failed to decode symbol
     0.01%  hang     [kernel.kallsyms]  [k] 0x00000000c08c5438
[kamensky@coreos-lnx2 hang]$ strace -o /tmp/perf.trace.txt
$L/tools/perf/perf report --stdio --symfs
/wd2/nfs/rootfs_32_le_tc2_20141218 >& /dev/null
[kamensky@coreos-lnx2 hang]$ grep open /tmp/perf.trace.txt | grep hang
open("/home/root/tests/hang/hang", O_RDONLY) = -1 ENOENT (No such file
or directory) <--- filename__read_debuglink tries to open file
open("/wd2/nfs/rootfs_32_le_tc2_20141218//usr/lib/debug/home/root/tests/hang/hang.debug",
O_RDONLY) = -1 ENOENT (No such file or directory)
open("/wd2/nfs/rootfs_32_le_tc2_20141218//usr/lib/debug/home/root/tests/hang/hang",
O_RDONLY) = -1 ENOENT (No such file or directory)
open("/wd2/nfs/rootfs_32_le_tc2_20141218//home/root/tests/hang/hang",
O_RDONLY) = 4
open("/wd2/nfs/rootfs_32_le_tc2_20141218//home/root/tests/hang/.debug/hang",
O_RDONLY) = -1 ENOENT (No such file or directory)

Running version of perf with proposed fix, now perf picks up
right hang.debug symbols file:

[kamensky@coreos-lnx2 hang]$ $L/tools/perf/perf report --stdio --symfs
/wd2/nfs/rootfs_32_le_tc2_20141218 | head -10
[kernel.kallsyms] with build id
c70bcdc8d97df0dcded9984e3ada2b6ff5a69510 not found, continuing without
symbols
# To display the perf.data header info, please use
--header/--header-only options.
#
# Samples: 37K of event 'cycles'
# Event count (approx.): 9469365389
#
# Overhead  Command  Shared Object      Symbol
# ........  .......  .................  ......................
#
    99.97%  hang     hang               [.] hang
     0.01%  hang     [kernel.kallsyms]  [k] 0x00000000c08c5438

Thanks,
Victor


On 13 January 2015 at 08:59, Victor Kamensky <victor.kamensky@linaro.org> wrote:
> Currently code that tries to read corresponding debug symbol
> file from .gnu_debuglink section (DSO_BINARY_TYPE__DEBUGLINK)
> does not take in account symfs option, so filename__read_debuglink
> function cannot open ELF file, if symfs option is used.
>
> Fix is to add proper handling of symfs as it is done in other
> places: use __symbol__join_symfs function to get real file name
> of target ELF file.
>
> Created malloced copy of target filename in symfs before passing
> it to __symbol__join_symfs function because filename will be
> modified by it if corresponding debuglink is found.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Waiman Long <Waiman.Long@hp.com>
> Cc: David Ahern <dsahern@gmail.com>
> ---
>  tools/perf/util/dso.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 45be944..6a2f663 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -42,19 +42,30 @@ int dso__read_binary_type_filename(const struct dso *dso,
>         size_t len;
>
>         switch (type) {
> -       case DSO_BINARY_TYPE__DEBUGLINK: {
> +       case DSO_BINARY_TYPE__DEBUGLINK:
> +       {
>                 char *debuglink;
> -
> -               strncpy(filename, dso->long_name, size);
> -               debuglink = filename + dso->long_name_len;
> -               while (debuglink != filename && *debuglink != '/')
> -                       debuglink--;
> -               if (*debuglink == '/')
> -                       debuglink++;
> -               ret = filename__read_debuglink(dso->long_name, debuglink,
> -                                              size - (debuglink - filename));
> -               }
> +               char *filename_copy;
> +
> +               filename_copy = malloc(PATH_MAX);
> +               if (filename_copy) {
> +                       len = __symbol__join_symfs(filename, size,
> +                                                  dso->long_name);
> +                       strncpy(filename_copy, filename, PATH_MAX);
> +                       debuglink = filename + len;
> +                       while (debuglink != filename && *debuglink != '/')
> +                               debuglink--;
> +                       if (*debuglink == '/')
> +                               debuglink++;
> +                       ret = filename__read_debuglink(filename_copy, debuglink,
> +                                                      size - (debuglink -
> +                                                              filename));
> +                       free(filename_copy);
> +               } else
> +                       ret = -1;
>                 break;
> +       }
> +
>         case DSO_BINARY_TYPE__BUILD_ID_CACHE:
>                 /* skip the locally configured cache if a symfs is given */
>                 if (symbol_conf.symfs[0] ||
> --
> 1.9.3
>

[PATCH 2/2] perf symbols: debuglink should take symfs option into account

[Victor Kamensky]

Hi Guys,

If it helps to look at below patch, here is a test case
for failure that below patch addresses.

Run on target with root NFS mounted in this case. Current
target is ARM TC2 board.

But I am pretty sure the same sequence will fail on any other
device that is built in cross compiled way, with .gnu_debuglink
split debug symbols and with and perf used with --symfs option:

On target device built split debug executable with .gnu_debuglink
section and collect perf data:

root at genericarmv7a:~/tests/hang# cat hang.c
void hang(void)
{
    while(1);
}

int main (void)
{
    hang();
    return 0;
}
root at genericarmv7a:~/tests/hang# gcc -o hang -g hang.c
root at genericarmv7a:~/tests/hang# cp hang hang.debug
root at genericarmv7a:~/tests/hang# strip --only-keep-debug hang.debug
root at genericarmv7a:~/tests/hang# objcopy --add-gnu-debuglink=hang.debug hang
root at genericarmv7a:~/tests/hang# strip hang
root at genericarmv7a:~/tests/hang# readelf -S hang | grep gnu_debuglink
  [27] .gnu_debuglink    PROGBITS        00000000 00060b 000010 00      0   0  1
root at genericarmv7a:~/tests/hang# readelf -x 27 hang

Hex dump of section '.gnu_debuglink':
  0x00000000 68616e67 2e646562 75670000 ee7e9545 hang.debug...~.E

root at genericarmv7a:~/tests/hang# perf record -e cycles --no-buildid-cache ./hang
^C[ perf record: Woken up 6 times to write data ]
[ perf record: Captured and wrote 1.448 MB perf.data (~63258 samples) ]

root at genericarmv7a:~/tests/hang# chmod a+r perf.data


On the host trying to decode perf result without proposed fix
using --symfs option to point to target rootfs. Note failure of
decode symbol for hang function.

[kamensky at coreos-lnx2 hang]$ ls -a
.  ..  hang  hang.c  hang.debug  perf.data
[kamensky at coreos-lnx2 hang]$ $L/tools/perf/perf report --stdio --symfs
/wd2/nfs/rootfs_32_le_tc2_20141218 | head -10
[kernel.kallsyms] with build id
c70bcdc8d97df0dcded9984e3ada2b6ff5a69510 not found, continuing without
symbols
no symbols found in /home/root/tests/hang/hang, maybe install a debug package?
# To display the perf.data header info, please use
--header/--header-only options.
#
# Samples: 37K of event 'cycles'
# Event count (approx.): 9469365389
#
# Overhead  Command  Shared Object      Symbol
# ........  .......  .................  ......................
#
    99.97%  hang     hang               [.] 0x00000000000003ec <--
failed to decode symbol
     0.01%  hang     [kernel.kallsyms]  [k] 0x00000000c08c5438
[kamensky@coreos-lnx2 hang]$ strace -o /tmp/perf.trace.txt
$L/tools/perf/perf report --stdio --symfs
/wd2/nfs/rootfs_32_le_tc2_20141218 >& /dev/null
[kamensky at coreos-lnx2 hang]$ grep open /tmp/perf.trace.txt | grep hang
open("/home/root/tests/hang/hang", O_RDONLY) = -1 ENOENT (No such file
or directory) <--- filename__read_debuglink tries to open file
open("/wd2/nfs/rootfs_32_le_tc2_20141218//usr/lib/debug/home/root/tests/hang/hang.debug",
O_RDONLY) = -1 ENOENT (No such file or directory)
open("/wd2/nfs/rootfs_32_le_tc2_20141218//usr/lib/debug/home/root/tests/hang/hang",
O_RDONLY) = -1 ENOENT (No such file or directory)
open("/wd2/nfs/rootfs_32_le_tc2_20141218//home/root/tests/hang/hang",
O_RDONLY) = 4
open("/wd2/nfs/rootfs_32_le_tc2_20141218//home/root/tests/hang/.debug/hang",
O_RDONLY) = -1 ENOENT (No such file or directory)

Running version of perf with proposed fix, now perf picks up
right hang.debug symbols file:

[kamensky at coreos-lnx2 hang]$ $L/tools/perf/perf report --stdio --symfs
/wd2/nfs/rootfs_32_le_tc2_20141218 | head -10
[kernel.kallsyms] with build id
c70bcdc8d97df0dcded9984e3ada2b6ff5a69510 not found, continuing without
symbols
# To display the perf.data header info, please use
--header/--header-only options.
#
# Samples: 37K of event 'cycles'
# Event count (approx.): 9469365389
#
# Overhead  Command  Shared Object      Symbol
# ........  .......  .................  ......................
#
    99.97%  hang     hang               [.] hang
     0.01%  hang     [kernel.kallsyms]  [k] 0x00000000c08c5438

Thanks,
Victor


On 13 January 2015 at 08:59, Victor Kamensky <victor.kamensky@linaro.org> wrote:
> Currently code that tries to read corresponding debug symbol
> file from .gnu_debuglink section (DSO_BINARY_TYPE__DEBUGLINK)
> does not take in account symfs option, so filename__read_debuglink
> function cannot open ELF file, if symfs option is used.
>
> Fix is to add proper handling of symfs as it is done in other
> places: use __symbol__join_symfs function to get real file name
> of target ELF file.
>
> Created malloced copy of target filename in symfs before passing
> it to __symbol__join_symfs function because filename will be
> modified by it if corresponding debuglink is found.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Waiman Long <Waiman.Long@hp.com>
> Cc: David Ahern <dsahern@gmail.com>
> ---
>  tools/perf/util/dso.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 45be944..6a2f663 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -42,19 +42,30 @@ int dso__read_binary_type_filename(const struct dso *dso,
>         size_t len;
>
>         switch (type) {
> -       case DSO_BINARY_TYPE__DEBUGLINK: {
> +       case DSO_BINARY_TYPE__DEBUGLINK:
> +       {
>                 char *debuglink;
> -
> -               strncpy(filename, dso->long_name, size);
> -               debuglink = filename + dso->long_name_len;
> -               while (debuglink != filename && *debuglink != '/')
> -                       debuglink--;
> -               if (*debuglink == '/')
> -                       debuglink++;
> -               ret = filename__read_debuglink(dso->long_name, debuglink,
> -                                              size - (debuglink - filename));
> -               }
> +               char *filename_copy;
> +
> +               filename_copy = malloc(PATH_MAX);
> +               if (filename_copy) {
> +                       len = __symbol__join_symfs(filename, size,
> +                                                  dso->long_name);
> +                       strncpy(filename_copy, filename, PATH_MAX);
> +                       debuglink = filename + len;
> +                       while (debuglink != filename && *debuglink != '/')
> +                               debuglink--;
> +                       if (*debuglink == '/')
> +                               debuglink++;
> +                       ret = filename__read_debuglink(filename_copy, debuglink,
> +                                                      size - (debuglink -
> +                                                              filename));
> +                       free(filename_copy);
> +               } else
> +                       ret = -1;
>                 break;
> +       }
> +
>         case DSO_BINARY_TYPE__BUILD_ID_CACHE:
>                 /* skip the locally configured cache if a symfs is given */
>                 if (symbol_conf.symfs[0] ||
> --
> 1.9.3
>

Re: [PATCH 2/2] perf symbols: debuglink should take symfs option into account

[David Ahern]

On 1/19/15 10:50 AM, Victor Kamensky wrote:
>> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
>> index 45be944..6a2f663 100644
>> --- a/tools/perf/util/dso.c
>> +++ b/tools/perf/util/dso.c
>> @@ -42,19 +42,30 @@ int dso__read_binary_type_filename(const struct dso *dso,
>>          size_t len;
>>
>>          switch (type) {
>> -       case DSO_BINARY_TYPE__DEBUGLINK: {
>> +       case DSO_BINARY_TYPE__DEBUGLINK:
>> +       {
>>                  char *debuglink;
>> -
>> -               strncpy(filename, dso->long_name, size);
>> -               debuglink = filename + dso->long_name_len;
>> -               while (debuglink != filename && *debuglink != '/')
>> -                       debuglink--;
>> -               if (*debuglink == '/')
>> -                       debuglink++;
>> -               ret = filename__read_debuglink(dso->long_name, debuglink,
>> -                                              size - (debuglink - filename));
>> -               }
>> +               char *filename_copy;
>> +
>> +               filename_copy = malloc(PATH_MAX);
>> +               if (filename_copy) {
>> +                       len = __symbol__join_symfs(filename, size,
>> +                                                  dso->long_name);
>> +                       strncpy(filename_copy, filename, PATH_MAX);
>> +                       debuglink = filename + len;
>> +                       while (debuglink != filename && *debuglink != '/')
>> +                               debuglink--;
>> +                       if (*debuglink == '/')
>> +                               debuglink++;
>> +                       ret = filename__read_debuglink(filename_copy, debuglink,
>> +                                                      size - (debuglink -
>> +                                                              filename));
>> +                       free(filename_copy);
>> +               } else
>> +                       ret = -1;
>>                  break;
>> +       }
>> +

I do not believe the filename_copy is needed; just add the symfs path to 
filename and pass filename to read_debuglink

David

[PATCH 2/2] perf symbols: debuglink should take symfs option into account

[David Ahern]

On 1/19/15 10:50 AM, Victor Kamensky wrote:
>> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
>> index 45be944..6a2f663 100644
>> --- a/tools/perf/util/dso.c
>> +++ b/tools/perf/util/dso.c
>> @@ -42,19 +42,30 @@ int dso__read_binary_type_filename(const struct dso *dso,
>>          size_t len;
>>
>>          switch (type) {
>> -       case DSO_BINARY_TYPE__DEBUGLINK: {
>> +       case DSO_BINARY_TYPE__DEBUGLINK:
>> +       {
>>                  char *debuglink;
>> -
>> -               strncpy(filename, dso->long_name, size);
>> -               debuglink = filename + dso->long_name_len;
>> -               while (debuglink != filename && *debuglink != '/')
>> -                       debuglink--;
>> -               if (*debuglink == '/')
>> -                       debuglink++;
>> -               ret = filename__read_debuglink(dso->long_name, debuglink,
>> -                                              size - (debuglink - filename));
>> -               }
>> +               char *filename_copy;
>> +
>> +               filename_copy = malloc(PATH_MAX);
>> +               if (filename_copy) {
>> +                       len = __symbol__join_symfs(filename, size,
>> +                                                  dso->long_name);
>> +                       strncpy(filename_copy, filename, PATH_MAX);
>> +                       debuglink = filename + len;
>> +                       while (debuglink != filename && *debuglink != '/')
>> +                               debuglink--;
>> +                       if (*debuglink == '/')
>> +                               debuglink++;
>> +                       ret = filename__read_debuglink(filename_copy, debuglink,
>> +                                                      size - (debuglink -
>> +                                                              filename));
>> +                       free(filename_copy);
>> +               } else
>> +                       ret = -1;
>>                  break;
>> +       }
>> +

I do not believe the filename_copy is needed; just add the symfs path to 
filename and pass filename to read_debuglink

David

Re: [PATCH 2/2] perf symbols: debuglink should take symfs option into account

[Victor Kamensky]

David,

Thank you for response!

On 21 January 2015 at 14:00, David Ahern <dsahern@gmail.com> wrote:
> On 1/19/15 10:50 AM, Victor Kamensky wrote:
>>>
>>> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
>>> index 45be944..6a2f663 100644
>>> --- a/tools/perf/util/dso.c
>>> +++ b/tools/perf/util/dso.c
>>> @@ -42,19 +42,30 @@ int dso__read_binary_type_filename(const struct dso
>>> *dso,
>>>          size_t len;
>>>
>>>          switch (type) {
>>> -       case DSO_BINARY_TYPE__DEBUGLINK: {
>>> +       case DSO_BINARY_TYPE__DEBUGLINK:
>>> +       {
>>>                  char *debuglink;
>>> -
>>> -               strncpy(filename, dso->long_name, size);
>>> -               debuglink = filename + dso->long_name_len;
>>> -               while (debuglink != filename && *debuglink != '/')
>>> -                       debuglink--;
>>> -               if (*debuglink == '/')
>>> -                       debuglink++;
>>> -               ret = filename__read_debuglink(dso->long_name, debuglink,
>>> -                                              size - (debuglink -
>>> filename));
>>> -               }
>>> +               char *filename_copy;
>>> +
>>> +               filename_copy = malloc(PATH_MAX);
>>> +               if (filename_copy) {
>>> +                       len = __symbol__join_symfs(filename, size,
>>> +                                                  dso->long_name);
>>> +                       strncpy(filename_copy, filename, PATH_MAX);
>>> +                       debuglink = filename + len;
>>> +                       while (debuglink != filename && *debuglink !=
>>> '/')
>>> +                               debuglink--;
>>> +                       if (*debuglink == '/')
>>> +                               debuglink++;
>>> +                       ret = filename__read_debuglink(filename_copy,
>>> debuglink,
>>> +                                                      size - (debuglink
>>> -
>>> +
>>> filename));
>>> +                       free(filename_copy);
>>> +               } else
>>> +                       ret = -1;
>>>                  break;
>>> +       }
>>> +
>
>
> I do not believe the filename_copy is needed; just add the symfs path to
> filename and pass filename to read_debuglink

My first version of the fix did not create filename_copy and
looked like as patch below. But then I noticed that
filename__read_debuglink function receives two pointers:

'filename' first parameter pointer to executable path from which
.gnu_debuglink sections content will be read

'debuglink' path to directory that will be updated by the function
(note  strncpy at line 531) to point to debug symbol file.

Before my change offset within 'filename' parameter of
dso__read_binary_type_filename function is passed as
'debuglink' parameter and it will be updated, and
dso->long_name is separate, passed as 'filename'
parameter to filename__read_debuglink function.

When in the first version of patch, as below, I pass filename
buffer to both (filename to open first and pointer within
filename to be updated as debuglink) as in patch
below, it will work with current implementation because
open happens first but update happens after that. But that
is specific dependency on current implementation of
filename__read_debuglink function. It did not feel quite
right to me, but practically it could be OK.

Here is the first version of the without filename_copy.
Practically I am OK with it. Please let me know if
you prefer this version:

>From cafc06d95886f1d82f7b127af58a51384c0fe931 Mon Sep 17 00:00:00 2001
From: Victor Kamensky <victor.kamensky@linaro.org>
Date: Mon, 12 Jan 2015 17:33:06 -0800
Subject: [PATCH 2/2] perf symbols: debuglink should take symfs option into
 account

Currently code that tries to read corresponding debug symbol
file from .gnu_debuglink section (DSO_BINARY_TYPE__DEBUGLINK)
does not take in account symfs option, so filename__read_debuglink
function cannot open ELF file, if symfs option is used.

Fix is to add proper handling of symfs as it is done in other
places: use __symbol__join_symfs function to get real file name
of target ELF file.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 tools/perf/util/dso.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 45be944..ca8d8d5 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -45,13 +45,13 @@ int dso__read_binary_type_filename(const struct dso *dso,
     case DSO_BINARY_TYPE__DEBUGLINK: {
         char *debuglink;

-        strncpy(filename, dso->long_name, size);
-        debuglink = filename + dso->long_name_len;
+        len = __symbol__join_symfs(filename, size, dso->long_name);
+        debuglink = filename + len;
         while (debuglink != filename && *debuglink != '/')
             debuglink--;
         if (*debuglink == '/')
             debuglink++;
-        ret = filename__read_debuglink(dso->long_name, debuglink,
+        ret = filename__read_debuglink(filename, debuglink,
                            size - (debuglink - filename));
         }
         break;
-- 
1.9.3

Thanks,
Victor

> David
>

[PATCH 2/2] perf symbols: debuglink should take symfs option into account

[Victor Kamensky]

David,

Thank you for response!

On 21 January 2015 at 14:00, David Ahern <dsahern@gmail.com> wrote:
> On 1/19/15 10:50 AM, Victor Kamensky wrote:
>>>
>>> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
>>> index 45be944..6a2f663 100644
>>> --- a/tools/perf/util/dso.c
>>> +++ b/tools/perf/util/dso.c
>>> @@ -42,19 +42,30 @@ int dso__read_binary_type_filename(const struct dso
>>> *dso,
>>>          size_t len;
>>>
>>>          switch (type) {
>>> -       case DSO_BINARY_TYPE__DEBUGLINK: {
>>> +       case DSO_BINARY_TYPE__DEBUGLINK:
>>> +       {
>>>                  char *debuglink;
>>> -
>>> -               strncpy(filename, dso->long_name, size);
>>> -               debuglink = filename + dso->long_name_len;
>>> -               while (debuglink != filename && *debuglink != '/')
>>> -                       debuglink--;
>>> -               if (*debuglink == '/')
>>> -                       debuglink++;
>>> -               ret = filename__read_debuglink(dso->long_name, debuglink,
>>> -                                              size - (debuglink -
>>> filename));
>>> -               }
>>> +               char *filename_copy;
>>> +
>>> +               filename_copy = malloc(PATH_MAX);
>>> +               if (filename_copy) {
>>> +                       len = __symbol__join_symfs(filename, size,
>>> +                                                  dso->long_name);
>>> +                       strncpy(filename_copy, filename, PATH_MAX);
>>> +                       debuglink = filename + len;
>>> +                       while (debuglink != filename && *debuglink !=
>>> '/')
>>> +                               debuglink--;
>>> +                       if (*debuglink == '/')
>>> +                               debuglink++;
>>> +                       ret = filename__read_debuglink(filename_copy,
>>> debuglink,
>>> +                                                      size - (debuglink
>>> -
>>> +
>>> filename));
>>> +                       free(filename_copy);
>>> +               } else
>>> +                       ret = -1;
>>>                  break;
>>> +       }
>>> +
>
>
> I do not believe the filename_copy is needed; just add the symfs path to
> filename and pass filename to read_debuglink

My first version of the fix did not create filename_copy and
looked like as patch below. But then I noticed that
filename__read_debuglink function receives two pointers:

'filename' first parameter pointer to executable path from which
.gnu_debuglink sections content will be read

'debuglink' path to directory that will be updated by the function
(note  strncpy at line 531) to point to debug symbol file.

Before my change offset within 'filename' parameter of
dso__read_binary_type_filename function is passed as
'debuglink' parameter and it will be updated, and
dso->long_name is separate, passed as 'filename'
parameter to filename__read_debuglink function.

When in the first version of patch, as below, I pass filename
buffer to both (filename to open first and pointer within
filename to be updated as debuglink) as in patch
below, it will work with current implementation because
open happens first but update happens after that. But that
is specific dependency on current implementation of
filename__read_debuglink function. It did not feel quite
right to me, but practically it could be OK.

Here is the first version of the without filename_copy.
Practically I am OK with it. Please let me know if
you prefer this version:

>From cafc06d95886f1d82f7b127af58a51384c0fe931 Mon Sep 17 00:00:00 2001
From: Victor Kamensky <victor.kamensky@linaro.org>
Date: Mon, 12 Jan 2015 17:33:06 -0800
Subject: [PATCH 2/2] perf symbols: debuglink should take symfs option into
 account

Currently code that tries to read corresponding debug symbol
file from .gnu_debuglink section (DSO_BINARY_TYPE__DEBUGLINK)
does not take in account symfs option, so filename__read_debuglink
function cannot open ELF file, if symfs option is used.

Fix is to add proper handling of symfs as it is done in other
places: use __symbol__join_symfs function to get real file name
of target ELF file.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 tools/perf/util/dso.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 45be944..ca8d8d5 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -45,13 +45,13 @@ int dso__read_binary_type_filename(const struct dso *dso,
     case DSO_BINARY_TYPE__DEBUGLINK: {
         char *debuglink;

-        strncpy(filename, dso->long_name, size);
-        debuglink = filename + dso->long_name_len;
+        len = __symbol__join_symfs(filename, size, dso->long_name);
+        debuglink = filename + len;
         while (debuglink != filename && *debuglink != '/')
             debuglink--;
         if (*debuglink == '/')
             debuglink++;
-        ret = filename__read_debuglink(dso->long_name, debuglink,
+        ret = filename__read_debuglink(filename, debuglink,
                            size - (debuglink - filename));
         }
         break;
-- 
1.9.3

Thanks,
Victor

> David
>

Re: [PATCH 2/2] perf symbols: debuglink should take symfs option into account

[David Ahern]

On 1/21/15 5:34 PM, Victor Kamensky wrote:
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 45be944..ca8d8d5 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -45,13 +45,13 @@ int dso__read_binary_type_filename(const struct dso *dso,
>       case DSO_BINARY_TYPE__DEBUGLINK: {
>           char *debuglink;
>
> -        strncpy(filename, dso->long_name, size);
> -        debuglink = filename + dso->long_name_len;
> +        len = __symbol__join_symfs(filename, size, dso->long_name);
> +        debuglink = filename + len;
>           while (debuglink != filename && *debuglink != '/')
>               debuglink--;
>           if (*debuglink == '/')
>               debuglink++;
> -        ret = filename__read_debuglink(dso->long_name, debuglink,
> +        ret = filename__read_debuglink(filename, debuglink,
>                              size - (debuglink - filename));
>           }
>           break;
>

I do not see any reason this will not work. Essentially after 
filename__read_debuglink filename contains symfs + dso path + debuglink 
read from .gnu_debuglink section which is what is wanted.

Thanks for the example. I used it with both a symfs and non-symfs 
example and both times this change worked properly -- the correct 
hang.debug file is read.

Arnaldo?

David

[PATCH 2/2] perf symbols: debuglink should take symfs option into account

[David Ahern]

On 1/21/15 5:34 PM, Victor Kamensky wrote:
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 45be944..ca8d8d5 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -45,13 +45,13 @@ int dso__read_binary_type_filename(const struct dso *dso,
>       case DSO_BINARY_TYPE__DEBUGLINK: {
>           char *debuglink;
>
> -        strncpy(filename, dso->long_name, size);
> -        debuglink = filename + dso->long_name_len;
> +        len = __symbol__join_symfs(filename, size, dso->long_name);
> +        debuglink = filename + len;
>           while (debuglink != filename && *debuglink != '/')
>               debuglink--;
>           if (*debuglink == '/')
>               debuglink++;
> -        ret = filename__read_debuglink(dso->long_name, debuglink,
> +        ret = filename__read_debuglink(filename, debuglink,
>                              size - (debuglink - filename));
>           }
>           break;
>

I do not see any reason this will not work. Essentially after 
filename__read_debuglink filename contains symfs + dso path + debuglink 
read from .gnu_debuglink section which is what is wanted.

Thanks for the example. I used it with both a symfs and non-symfs 
example and both times this change worked properly -- the correct 
hang.debug file is read.

Arnaldo?

David

Re: [PATCH 2/2] perf symbols: debuglink should take symfs option into account

[Victor Kamensky]

On 21 January 2015 at 16:53, David Ahern <dsahern@gmail.com> wrote:
> On 1/21/15 5:34 PM, Victor Kamensky wrote:
>>
>> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
>> index 45be944..ca8d8d5 100644
>> --- a/tools/perf/util/dso.c
>> +++ b/tools/perf/util/dso.c
>> @@ -45,13 +45,13 @@ int dso__read_binary_type_filename(const struct dso
>> *dso,
>>       case DSO_BINARY_TYPE__DEBUGLINK: {
>>           char *debuglink;
>>
>> -        strncpy(filename, dso->long_name, size);
>> -        debuglink = filename + dso->long_name_len;
>> +        len = __symbol__join_symfs(filename, size, dso->long_name);
>> +        debuglink = filename + len;
>>           while (debuglink != filename && *debuglink != '/')
>>               debuglink--;
>>           if (*debuglink == '/')
>>               debuglink++;
>> -        ret = filename__read_debuglink(dso->long_name, debuglink,
>> +        ret = filename__read_debuglink(filename, debuglink,
>>                              size - (debuglink - filename));
>>           }
>>           break;
>>
>
> I do not see any reason this will not work. Essentially after
> filename__read_debuglink filename contains symfs + dso path + debuglink read
> from .gnu_debuglink section which is what is wanted.

OK, I am good with this. Let me repost the whole
mini-series based on mailing list review comments.

May I use 'Acked-by' (maybe 'Tested-by' as well) with
your name for the shorter (no filename_copy) as above
version of the patch for .gnu_debuglink issue fix.

ARM/Aarch64 specific change also needs update based on
discussion with Will and Russell.

I'll post updated version 2 latter tonight.

Thanks,
Victor

> Thanks for the example. I used it with both a symfs and non-symfs example
> and both times this change worked properly -- the correct hang.debug file is
> read.
>
> Arnaldo?
>
> David

[PATCH 2/2] perf symbols: debuglink should take symfs option into account

[Victor Kamensky]

On 21 January 2015 at 16:53, David Ahern <dsahern@gmail.com> wrote:
> On 1/21/15 5:34 PM, Victor Kamensky wrote:
>>
>> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
>> index 45be944..ca8d8d5 100644
>> --- a/tools/perf/util/dso.c
>> +++ b/tools/perf/util/dso.c
>> @@ -45,13 +45,13 @@ int dso__read_binary_type_filename(const struct dso
>> *dso,
>>       case DSO_BINARY_TYPE__DEBUGLINK: {
>>           char *debuglink;
>>
>> -        strncpy(filename, dso->long_name, size);
>> -        debuglink = filename + dso->long_name_len;
>> +        len = __symbol__join_symfs(filename, size, dso->long_name);
>> +        debuglink = filename + len;
>>           while (debuglink != filename && *debuglink != '/')
>>               debuglink--;
>>           if (*debuglink == '/')
>>               debuglink++;
>> -        ret = filename__read_debuglink(dso->long_name, debuglink,
>> +        ret = filename__read_debuglink(filename, debuglink,
>>                              size - (debuglink - filename));
>>           }
>>           break;
>>
>
> I do not see any reason this will not work. Essentially after
> filename__read_debuglink filename contains symfs + dso path + debuglink read
> from .gnu_debuglink section which is what is wanted.

OK, I am good with this. Let me repost the whole
mini-series based on mailing list review comments.

May I use 'Acked-by' (maybe 'Tested-by' as well) with
your name for the shorter (no filename_copy) as above
version of the patch for .gnu_debuglink issue fix.

ARM/Aarch64 specific change also needs update based on
discussion with Will and Russell.

I'll post updated version 2 latter tonight.

Thanks,
Victor

> Thanks for the example. I used it with both a symfs and non-symfs example
> and both times this change worked properly -- the correct hang.debug file is
> read.
>
> Arnaldo?
>
> David

Re: [PATCH 2/2] perf symbols: debuglink should take symfs option into account

[David Ahern]

On 1/21/15 6:32 PM, Victor Kamensky wrote:
> May I use 'Acked-by' (maybe 'Tested-by' as well) with
> your name for the shorter (no filename_copy) as above
> version of the patch for .gnu_debuglink issue fix.

You can add both to your second patch -- the short version.

Acked-and-Tested-by: David Ahern <dsahern@gmail.com>

[PATCH 2/2] perf symbols: debuglink should take symfs option into account

[David Ahern]

On 1/21/15 6:32 PM, Victor Kamensky wrote:
> May I use 'Acked-by' (maybe 'Tested-by' as well) with
> your name for the shorter (no filename_copy) as above
> version of the patch for .gnu_debuglink issue fix.

You can add both to your second patch -- the short version.

Acked-and-Tested-by: David Ahern <dsahern@gmail.com>