[PATCH v6 7/8] perf: disasm: prefer debugging files in build-id cache

Changbin Du posted 8 patches 1 year, 6 months ago
There is a newer version of this series
[PATCH v6 7/8] perf: disasm: prefer debugging files in build-id cache
Posted by Changbin Du 1 year, 6 months ago
The build-id cache might have both debugging and non-debugging files. Here
we prefer the debugging version for annotation.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Changbin Du <changbin.du@huawei.com>
---
 tools/perf/util/disasm.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 6af9fbec3a95..5f66b3632770 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1162,18 +1162,25 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	    !dso__is_kcore(dso))
 		return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;
 
-	build_id_filename = dso__build_id_filename(dso, NULL, 0, false);
-	if (build_id_filename) {
-		__symbol__join_symfs(filename, filename_size, build_id_filename);
-		free(build_id_filename);
-	} else {
-		if (dso__has_build_id(dso))
-			return ENOMEM;
-		return fallback_filename(dso, filename, filename_size);
-	}
+	/* Prefer debugging file if exists, otherwise non-debugging one is used. */
+	for (int i = 0; i < 2; i++) {
+		build_id_filename = dso__build_id_filename(dso, NULL, 0, !i);
+		if (build_id_filename) {
+			__symbol__join_symfs(filename, filename_size, build_id_filename);
+			free(build_id_filename);
+		} else {
+			if (dso__has_build_id(dso))
+				return ENOMEM;
+			return fallback_filename(dso, filename, filename_size);
+		}
 
-	if (access(filename, R_OK))
-		return fallback_filename(dso, filename, filename_size);
+		if (!access(filename, R_OK))
+			break;
+		else if (i != 0) {
+			/* nor debugging or non-debugging is found */
+			return fallback_filename(dso, filename, filename_size);
+		}
+	}
 
 	if (dso__is_kcore(dso) || dso__is_vdso(dso))
 		goto fallback;
-- 
2.34.1
Re: [PATCH v6 7/8] perf: disasm: prefer debugging files in build-id cache
Posted by Adrian Hunter 1 year, 5 months ago
On 25/07/24 05:15, Changbin Du wrote:
> The build-id cache might have both debugging and non-debugging files. Here
> we prefer the debugging version for annotation.

As I pointed out before, disassembling a different file
from the one that actually executed can have pitfalls.

If you want this, it needs to be optional, not the default.

But if you take the approach to remove vdso from the buildid
cache, and add back the debug version, then this patch would
not be needed for vdso.

> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Changbin Du <changbin.du@huawei.com>
> ---
>  tools/perf/util/disasm.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 6af9fbec3a95..5f66b3632770 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1162,18 +1162,25 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
>  	    !dso__is_kcore(dso))
>  		return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;
>  
> -	build_id_filename = dso__build_id_filename(dso, NULL, 0, false);
> -	if (build_id_filename) {
> -		__symbol__join_symfs(filename, filename_size, build_id_filename);
> -		free(build_id_filename);
> -	} else {
> -		if (dso__has_build_id(dso))
> -			return ENOMEM;
> -		return fallback_filename(dso, filename, filename_size);
> -	}
> +	/* Prefer debugging file if exists, otherwise non-debugging one is used. */
> +	for (int i = 0; i < 2; i++) {
> +		build_id_filename = dso__build_id_filename(dso, NULL, 0, !i);
> +		if (build_id_filename) {
> +			__symbol__join_symfs(filename, filename_size, build_id_filename);
> +			free(build_id_filename);
> +		} else {
> +			if (dso__has_build_id(dso))
> +				return ENOMEM;
> +			return fallback_filename(dso, filename, filename_size);
> +		}
>  
> -	if (access(filename, R_OK))
> -		return fallback_filename(dso, filename, filename_size);
> +		if (!access(filename, R_OK))
> +			break;
> +		else if (i != 0) {
> +			/* nor debugging or non-debugging is found */
> +			return fallback_filename(dso, filename, filename_size);
> +		}
> +	}
>  
>  	if (dso__is_kcore(dso) || dso__is_vdso(dso))
>  		goto fallback;