Re: [PATCH v3 2/4] perf: disasm: use build_id_path if fallback failed

[Namhyung Kim <namhyung@kernel.org>]

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
>