[v1] perf: support specify vdso path in cmdline

[PATCH 2/2] perf: disasm: prefer symsrc_filename for filename

Posted by Changbin Du 1 year, 8 months ago

If we already found a debugging version when loading symbols for that dso,
then use the same file for disasm instead of looking up in buildid-cache.

Signed-off-by: Changbin Du <changbin.du@huawei.com>
---
 tools/perf/util/disasm.c |  5 +++++
 tools/perf/util/symbol.c | 12 ++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 72aec8f61b94..75cfc6fbd11d 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1103,6 +1103,11 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	    !dso__is_kcore(dso))
 		return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;
 
+	if (dso__symsrc_filename(dso)) {
+		strlcpy(filename, dso__symsrc_filename(dso), filename_size);
+		return 0;
+	}
+
 	build_id_filename = dso__build_id_filename(dso, NULL, 0, false);
 	if (build_id_filename) {
 		__symbol__join_symfs(filename, filename_size, build_id_filename);
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 8d040039a7ce..a90c647d37e1 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2025,12 +2025,14 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
 		dso__set_binary_type(dso, DSO_BINARY_TYPE__VMLINUX);
 
 	err = dso__load_sym(dso, map, &ss, &ss, 0);
-	symsrc__destroy(&ss);
-
 	if (err > 0) {
 		dso__set_loaded(dso);
 		pr_debug("Using %s for symbols\n", symfs_vmlinux);
+
+		if (symsrc__has_symtab(&ss) && !dso__symsrc_filename(dso))
+			dso__set_symsrc_filename(dso, strdup(symfs_vmlinux));
 	}
+	symsrc__destroy(&ss);
 
 	return err;
 }
@@ -2395,12 +2397,14 @@ static int dso__load_vdso(struct dso *dso, struct map *map,
 	dso__set_binary_type(dso, DSO_BINARY_TYPE__SYSTEM_PATH_DSO);
 
 	err = dso__load_sym(dso, map, &ss, &ss, 0);
-	symsrc__destroy(&ss);
-
 	if (err > 0) {
 		dso__set_loaded(dso);
 		pr_debug("Using %s for %s symbols\n", symfs_vdso, dso__short_name(dso));
+
+		if (symsrc__has_symtab(&ss) && !dso__symsrc_filename(dso))
+			dso__set_symsrc_filename(dso, strdup(symfs_vdso));
 	}
+	symsrc__destroy(&ss);
 
 	return err;
 }
-- 
2.34.1

Re: [PATCH 2/2] perf: disasm: prefer symsrc_filename for filename

Posted by Adrian Hunter 1 year, 8 months ago

On 13/06/24 09:35, Changbin Du wrote:
> If we already found a debugging version when loading symbols for that dso,
> then use the same file for disasm instead of looking up in buildid-cache.

In the past, there have been cases where the debugging version has not
worked for reading object code.  I don't remember the details, but the
symbols and debugging information was OK while the object code was not.

In general, using anything other than the file that was actually executed
for reading object code seems like a bad idea.

> 
> Signed-off-by: Changbin Du <changbin.du@huawei.com>
> ---
>  tools/perf/util/disasm.c |  5 +++++
>  tools/perf/util/symbol.c | 12 ++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 72aec8f61b94..75cfc6fbd11d 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1103,6 +1103,11 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
>  	    !dso__is_kcore(dso))
>  		return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;
>  
> +	if (dso__symsrc_filename(dso)) {
> +		strlcpy(filename, dso__symsrc_filename(dso), filename_size);
> +		return 0;
> +	}
> +
>  	build_id_filename = dso__build_id_filename(dso, NULL, 0, false);
>  	if (build_id_filename) {
>  		__symbol__join_symfs(filename, filename_size, build_id_filename);
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 8d040039a7ce..a90c647d37e1 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -2025,12 +2025,14 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
>  		dso__set_binary_type(dso, DSO_BINARY_TYPE__VMLINUX);
>  
>  	err = dso__load_sym(dso, map, &ss, &ss, 0);
> -	symsrc__destroy(&ss);
> -
>  	if (err > 0) {
>  		dso__set_loaded(dso);
>  		pr_debug("Using %s for symbols\n", symfs_vmlinux);
> +
> +		if (symsrc__has_symtab(&ss) && !dso__symsrc_filename(dso))
> +			dso__set_symsrc_filename(dso, strdup(symfs_vmlinux));
>  	}
> +	symsrc__destroy(&ss);
>  
>  	return err;
>  }
> @@ -2395,12 +2397,14 @@ static int dso__load_vdso(struct dso *dso, struct map *map,
>  	dso__set_binary_type(dso, DSO_BINARY_TYPE__SYSTEM_PATH_DSO);
>  
>  	err = dso__load_sym(dso, map, &ss, &ss, 0);
> -	symsrc__destroy(&ss);
> -
>  	if (err > 0) {
>  		dso__set_loaded(dso);
>  		pr_debug("Using %s for %s symbols\n", symfs_vdso, dso__short_name(dso));
> +
> +		if (symsrc__has_symtab(&ss) && !dso__symsrc_filename(dso))
> +			dso__set_symsrc_filename(dso, strdup(symfs_vdso));
>  	}
> +	symsrc__destroy(&ss);
>  
>  	return err;
>  }

Re: [PATCH 2/2] perf: disasm: prefer symsrc_filename for filename

Posted by duchangbin 1 year, 8 months ago

On Thu, Jun 13, 2024 at 11:15:28AM +0300, Adrian Hunter wrote:
> On 13/06/24 09:35, Changbin Du wrote:
> > If we already found a debugging version when loading symbols for that dso,
> > then use the same file for disasm instead of looking up in buildid-cache.
> 
> In the past, there have been cases where the debugging version has not
> worked for reading object code.  I don't remember the details, but the
> symbols and debugging information was OK while the object code was not.
> 
> In general, using anything other than the file that was actually executed
> for reading object code seems like a bad idea.
> 
Is this a platform specific issue? AFAIK, the binary code in debugging and
non-debugging version should be identical. 

> 
> 

-- 
Cheers,
Changbin Du

Re: [PATCH 2/2] perf: disasm: prefer symsrc_filename for filename

Posted by Adrian Hunter 1 year, 8 months ago

On 13/06/24 12:43, duchangbin wrote:
> On Thu, Jun 13, 2024 at 11:15:28AM +0300, Adrian Hunter wrote:
>> On 13/06/24 09:35, Changbin Du wrote:
>>> If we already found a debugging version when loading symbols for that dso,
>>> then use the same file for disasm instead of looking up in buildid-cache.
>>
>> In the past, there have been cases where the debugging version has not
>> worked for reading object code.  I don't remember the details, but the
>> symbols and debugging information was OK while the object code was not.
>>
>> In general, using anything other than the file that was actually executed
>> for reading object code seems like a bad idea.
>>
> Is this a platform specific issue? AFAIK, the binary code in debugging and
> non-debugging version should be identical. 

"should be" != "guaranteed to be".  Simpler to avoid the issue and
stick with the file that was actually executed.  We already support
having separate symbol sources, so there should not really be a
problem.

Re: [PATCH 2/2] perf: disasm: prefer symsrc_filename for filename

Posted by duchangbin 1 year, 8 months ago

On Thu, Jun 13, 2024 at 01:26:39PM +0300, Adrian Hunter wrote:
> On 13/06/24 12:43, duchangbin wrote:
> > On Thu, Jun 13, 2024 at 11:15:28AM +0300, Adrian Hunter wrote:
> >> On 13/06/24 09:35, Changbin Du wrote:
> >>> If we already found a debugging version when loading symbols for that dso,
> >>> then use the same file for disasm instead of looking up in buildid-cache.
> >>
> >> In the past, there have been cases where the debugging version has not
> >> worked for reading object code.  I don't remember the details, but the
> >> symbols and debugging information was OK while the object code was not.
> >>
> >> In general, using anything other than the file that was actually executed
> >> for reading object code seems like a bad idea.
> >>
> > Is this a platform specific issue? AFAIK, the binary code in debugging and
> > non-debugging version should be identical. 
> 
> "should be" != "guaranteed to be".  Simpler to avoid the issue and
> stick with the file that was actually executed.  We already support
> having separate symbol sources, so there should not really be a
> problem.
>
ok, so for vdso I think we can flow the kernel part in dso__disassemble_filename.
I'll add vdso processing here.

> 

-- 
Cheers,
Changbin Du