If we can not fallback for special dso (vmlinx and vdso), use the
build_id_path found previously.

To make change easy, this change first refactors the code by extracting
two functions read_buildid_linkname() and fallback_filename().

Signed-off-by: Changbin Du <changbin.du@huawei.com>
---
 tools/perf/util/disasm.c | 121 ++++++++++++++++++++++++---------------
 1 file changed, 75 insertions(+), 46 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 442e9802a772..3075daa61916 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1092,14 +1092,72 @@ int symbol__strerror_disassemble(struct map_symbol *ms, int errnum, char *buf, s
 	return 0;
 }
 
-static int dso__disassemble_filename(struct dso *dso, char *filename, size_t filename_size)
+static int read_buildid_linkname(char *filename, char *linkname, size_t linkname_size)
 {
-	char linkname[PATH_MAX];
-	char *build_id_filename;
 	char *build_id_path = NULL;
 	char *pos;
 	int len;
 
+	build_id_path = strdup(filename);
+	if (!build_id_path)
+		return ENOMEM;
+
+	/*
+	 * old style build-id cache has name of XX/XXXXXXX.. while
+	 * new style has XX/XXXXXXX../{elf,kallsyms,vdso}.
+	 * extract the build-id part of dirname in the new style only.
+	 */
+	pos = strrchr(build_id_path, '/');
+	if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
+		dirname(build_id_path);
+
+	len = readlink(build_id_path, linkname, linkname_size);
+	if (len < 0) {
+		free(build_id_path);
+		return -1;
+	}
+
+	linkname[len] = '\0';
+	free(build_id_path);
+	return 0;
+}
+
+static int fallback_filename(struct dso *dso, char *filename, size_t filename_size)
+{
+	char filepath[PATH_MAX];
+
+	/*
+	 * If we don't have build-ids or the build-id file isn't in the
+	 * cache, or is just a kallsyms file, well, lets hope that this
+	 * DSO is the same as when 'perf record' ran.
+	 */
+	if ((dso__kernel(dso) || dso__is_vdso(dso)) && dso__long_name(dso)[0] == '/')
+		snprintf(filepath, sizeof(filepath), "%s", dso__long_name(dso));
+	else
+		__symbol__join_symfs(filepath, sizeof(filepath), dso__long_name(dso));
+
+	mutex_lock(dso__lock(dso));
+	if (access(filepath, R_OK) && errno == ENOENT && dso__nsinfo(dso)) {
+		char *new_name = dso__filename_with_chroot(dso, filepath);
+		if (new_name) {
+			strlcpy(filepath, new_name, sizeof(filepath));
+			free(new_name);
+		}
+	}
+	mutex_unlock(dso__lock(dso));
+
+	if (access(filepath, R_OK) && errno == ENOENT)
+		return ENOENT;
+
+	snprintf(filename, filename_size, "%s", filepath);
+	return 0;
+}
+
+static int dso__disassemble_filename(struct dso *dso, char *filename, size_t filename_size)
+{
+	char linkname[PATH_MAX];
+	char *build_id_filename;
+
 	if (dso__symtab_type(dso) == DSO_BINARY_TYPE__KALLSYMS &&
 	    !dso__is_kcore(dso))
 		return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;
@@ -1111,57 +1169,28 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	} else {
 		if (dso__has_build_id(dso))
 			return ENOMEM;
-		goto fallback;
+		return fallback_filename(dso, filename, filename_size);
 	}
 
-	build_id_path = strdup(filename);
-	if (!build_id_path)
-		return ENOMEM;
-
-	/*
-	 * old style build-id cache has name of XX/XXXXXXX.. while
-	 * new style has XX/XXXXXXX../{elf,kallsyms,vdso}.
-	 * extract the build-id part of dirname in the new style only.
-	 */
-	pos = strrchr(build_id_path, '/');
-	if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
-		dirname(build_id_path);
+	if (access(filename, R_OK))
+		return fallback_filename(dso, filename, filename_size);
 
-	if (dso__is_kcore(dso))
+	if (dso__is_kcore(dso) || dso__is_vdso(dso))
 		goto fallback;
 
-	len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
-	if (len < 0)
-		goto fallback;
+	if (!read_buildid_linkname(filename, linkname, sizeof(linkname) - 1) &&
+	    (!strstr(linkname, DSO__NAME_KALLSYMS) && !strstr(linkname, DSO__NAME_VDSO))) {
+		/* not kallsysms or vdso, use build_id path found above */
+		goto out;
+	}
 
-	linkname[len] = '\0';
-	if (strstr(linkname, DSO__NAME_KALLSYMS) || strstr(linkname, DSO__NAME_VDSO) ||
-		access(filename, R_OK)) {
 fallback:
-		/*
-		 * If we don't have build-ids or the build-id file isn't in the
-		 * cache, or is just a kallsyms file, well, lets hope that this
-		 * DSO is the same as when 'perf record' ran.
-		 */
-		if ((dso__kernel(dso) || dso__is_vdso(dso)) && dso__long_name(dso)[0] == '/')
-			snprintf(filename, filename_size, "%s", dso__long_name(dso));
-		else
-			__symbol__join_symfs(filename, filename_size, dso__long_name(dso));
-
-		mutex_lock(dso__lock(dso));
-		if (access(filename, R_OK) && errno == ENOENT && dso__nsinfo(dso)) {
-			char *new_name = dso__filename_with_chroot(dso, filename);
-			if (new_name) {
-				strlcpy(filename, new_name, filename_size);
-				free(new_name);
-			}
-		}
-		mutex_unlock(dso__lock(dso));
-	} else if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND) {
-		dso__set_binary_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
+	if (fallback_filename(dso, filename, filename_size)) {
+		/* if fallback failed, use build_id path found above */
+out:
+		if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND)
+			dso__set_binary_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
 	}
-
-	free(build_id_path);
 	return 0;
 }
 
-- 
2.34.1

Hello,

On Tue, Jun 18, 2024 at 09:55:28AM +0800, Changbin Du wrote:
> If we can not fallback for special dso (vmlinx and vdso), use the
> build_id_path found previously.
> 
> To make change easy, this change first refactors the code by extracting
> two functions read_buildid_linkname() and fallback_filename().

Can you please split the refactoring from the actual change?  It'd be
easier to review and to maintain the code.

Thanks,
Namhyung

> 
> Signed-off-by: Changbin Du <changbin.du@huawei.com>
> ---
>  tools/perf/util/disasm.c | 121 ++++++++++++++++++++++++---------------
>  1 file changed, 75 insertions(+), 46 deletions(-)
> 
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 442e9802a772..3075daa61916 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1092,14 +1092,72 @@ int symbol__strerror_disassemble(struct map_symbol *ms, int errnum, char *buf, s
>  	return 0;
>  }
>  
> -static int dso__disassemble_filename(struct dso *dso, char *filename, size_t filename_size)
> +static int read_buildid_linkname(char *filename, char *linkname, size_t linkname_size)
>  {
> -	char linkname[PATH_MAX];
> -	char *build_id_filename;
>  	char *build_id_path = NULL;
>  	char *pos;
>  	int len;
>  
> +	build_id_path = strdup(filename);
> +	if (!build_id_path)
> +		return ENOMEM;
> +
> +	/*
> +	 * old style build-id cache has name of XX/XXXXXXX.. while
> +	 * new style has XX/XXXXXXX../{elf,kallsyms,vdso}.
> +	 * extract the build-id part of dirname in the new style only.
> +	 */
> +	pos = strrchr(build_id_path, '/');
> +	if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
> +		dirname(build_id_path);
> +
> +	len = readlink(build_id_path, linkname, linkname_size);
> +	if (len < 0) {
> +		free(build_id_path);
> +		return -1;
> +	}
> +
> +	linkname[len] = '\0';
> +	free(build_id_path);
> +	return 0;
> +}
> +
> +static int fallback_filename(struct dso *dso, char *filename, size_t filename_size)
> +{
> +	char filepath[PATH_MAX];
> +
> +	/*
> +	 * If we don't have build-ids or the build-id file isn't in the
> +	 * cache, or is just a kallsyms file, well, lets hope that this
> +	 * DSO is the same as when 'perf record' ran.
> +	 */
> +	if ((dso__kernel(dso) || dso__is_vdso(dso)) && dso__long_name(dso)[0] == '/')
> +		snprintf(filepath, sizeof(filepath), "%s", dso__long_name(dso));
> +	else
> +		__symbol__join_symfs(filepath, sizeof(filepath), dso__long_name(dso));
> +
> +	mutex_lock(dso__lock(dso));
> +	if (access(filepath, R_OK) && errno == ENOENT && dso__nsinfo(dso)) {
> +		char *new_name = dso__filename_with_chroot(dso, filepath);
> +		if (new_name) {
> +			strlcpy(filepath, new_name, sizeof(filepath));
> +			free(new_name);
> +		}
> +	}
> +	mutex_unlock(dso__lock(dso));
> +
> +	if (access(filepath, R_OK) && errno == ENOENT)
> +		return ENOENT;
> +
> +	snprintf(filename, filename_size, "%s", filepath);
> +	return 0;
> +}
> +
> +static int dso__disassemble_filename(struct dso *dso, char *filename, size_t filename_size)
> +{
> +	char linkname[PATH_MAX];
> +	char *build_id_filename;
> +
>  	if (dso__symtab_type(dso) == DSO_BINARY_TYPE__KALLSYMS &&
>  	    !dso__is_kcore(dso))
>  		return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;
> @@ -1111,57 +1169,28 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
>  	} else {
>  		if (dso__has_build_id(dso))
>  			return ENOMEM;
> -		goto fallback;
> +		return fallback_filename(dso, filename, filename_size);
>  	}
>  
> -	build_id_path = strdup(filename);
> -	if (!build_id_path)
> -		return ENOMEM;
> -
> -	/*
> -	 * old style build-id cache has name of XX/XXXXXXX.. while
> -	 * new style has XX/XXXXXXX../{elf,kallsyms,vdso}.
> -	 * extract the build-id part of dirname in the new style only.
> -	 */
> -	pos = strrchr(build_id_path, '/');
> -	if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
> -		dirname(build_id_path);
> +	if (access(filename, R_OK))
> +		return fallback_filename(dso, filename, filename_size);
>  
> -	if (dso__is_kcore(dso))
> +	if (dso__is_kcore(dso) || dso__is_vdso(dso))
>  		goto fallback;
>  
> -	len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
> -	if (len < 0)
> -		goto fallback;
> +	if (!read_buildid_linkname(filename, linkname, sizeof(linkname) - 1) &&
> +	    (!strstr(linkname, DSO__NAME_KALLSYMS) && !strstr(linkname, DSO__NAME_VDSO))) {
> +		/* not kallsysms or vdso, use build_id path found above */
> +		goto out;
> +	}
>  
> -	linkname[len] = '\0';
> -	if (strstr(linkname, DSO__NAME_KALLSYMS) || strstr(linkname, DSO__NAME_VDSO) ||
> -		access(filename, R_OK)) {
>  fallback:
> -		/*
> -		 * If we don't have build-ids or the build-id file isn't in the
> -		 * cache, or is just a kallsyms file, well, lets hope that this
> -		 * DSO is the same as when 'perf record' ran.
> -		 */
> -		if ((dso__kernel(dso) || dso__is_vdso(dso)) && dso__long_name(dso)[0] == '/')
> -			snprintf(filename, filename_size, "%s", dso__long_name(dso));
> -		else
> -			__symbol__join_symfs(filename, filename_size, dso__long_name(dso));
> -
> -		mutex_lock(dso__lock(dso));
> -		if (access(filename, R_OK) && errno == ENOENT && dso__nsinfo(dso)) {
> -			char *new_name = dso__filename_with_chroot(dso, filename);
> -			if (new_name) {
> -				strlcpy(filename, new_name, filename_size);
> -				free(new_name);
> -			}
> -		}
> -		mutex_unlock(dso__lock(dso));
> -	} else if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND) {
> -		dso__set_binary_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
> +	if (fallback_filename(dso, filename, filename_size)) {
> +		/* if fallback failed, use build_id path found above */
> +out:
> +		if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND)
> +			dso__set_binary_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
>  	}
> -
> -	free(build_id_path);
>  	return 0;
>  }
>  
> -- 
> 2.34.1
>

On Mon, Jun 24, 2024 at 05:08:18PM -0700, Namhyung Kim wrote:
> Hello,
> 
> On Tue, Jun 18, 2024 at 09:55:28AM +0800, Changbin Du wrote:
> > If we can not fallback for special dso (vmlinx and vdso), use the
> > build_id_path found previously.
> > 
> > To make change easy, this change first refactors the code by extracting
> > two functions read_buildid_linkname() and fallback_filename().
> 
> Can you please split the refactoring from the actual change?  It'd be
> easier to review and to maintain the code.
> 
Sure, I'll do it taday.

> Thanks,
> Namhyung
> 

-- 
Cheers,
Changbin Du