Re: [PATCH v3] perf: Add layout support for --symfs option

[duchangbin <changbin.du@huawei.com>]

On Wed, Mar 04, 2026 at 10:48:00PM -0800, Namhyung Kim wrote:
> Hello,
> 
> > @@ -2491,16 +2492,36 @@ int symbol__config_symfs(const struct option *opt __maybe_unused,
> >  			 const char *dir, int unset __maybe_unused)
> >  {
> >  	char *bf = NULL;
> > +	char *layout_str;
> >  	int ret;
> >  
> > -	symbol_conf.symfs = strdup(dir);
> > -	if (symbol_conf.symfs == NULL)
> > -		return -ENOMEM;
> > +	layout_str = strrchr(dir, ',');
> > +	if (layout_str &&
> > +	    (!strcmp(layout_str + 1, "flat") || !strcmp(layout_str + 1, "hierarchy"))) {
> 
> I think it's better to fail with a message if unknown layout is given.
>
Agreed. The current issue is that if we use ',' as the delimiter (to maintain a
consistent style with other options), and the path itself contains a comma, it
would result in the following ambiguity:
 - /path,flat → symfs=/path, layout=flat
 - /path,hierarchy → symfs=/path, layout=hierarchy
 - /some,path/ → symfs=/some, layout=path/

Do you have better suggestions? Using a different delimiter (e.g., : or ;)?

> Thanks,
> Namhyung
> 
> > +		size_t dir_len = layout_str - dir;
> > +		char *dir_copy = strndup(dir, dir_len);
> > +
> > +		if (dir_copy == NULL)
> > +			return -ENOMEM;
> > +
> > +		symbol_conf.symfs = dir_copy;
> > +
> > +		layout_str++;
> > +		if (!strcmp(layout_str, "flat"))
> > +			symbol_conf.symfs_layout_flat = true;
> > +		else
> > +			symbol_conf.symfs_layout_flat = false;
> > +	} else {
> > +		symbol_conf.symfs = strdup(dir);
> > +		if (symbol_conf.symfs == NULL)
> > +			return -ENOMEM;
> > +		symbol_conf.symfs_layout_flat = false;
> > +	}
> >  
> >  	/* skip the locally configured cache if a symfs is given, and
> >  	 * config buildid dir to symfs/.debug
> >  	 */
> > -	ret = asprintf(&bf, "%s/%s", dir, ".debug");
> > +	ret = asprintf(&bf, "%s/%s", symbol_conf.symfs, ".debug");
> >  	if (ret < 0)
> >  		return -ENOMEM;
> >  
> > diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> > index 3fb5d146d9b1..4f1dbd1ebd99 100644
> > --- a/tools/perf/util/symbol.h
> > +++ b/tools/perf/util/symbol.h
> > @@ -9,6 +9,7 @@
> >  #include <linux/list.h>
> >  #include <linux/rbtree.h>
> >  #include <stdio.h>
> > +#include <errno.h>
> >  #include "addr_location.h"
> >  #include "path.h"
> >  #include "symbol_conf.h"
> > @@ -96,6 +97,18 @@ struct intlist;
> >  
> >  static inline int __symbol__join_symfs(char *bf, size_t size, const char *path)
> >  {
> > +	if (symbol_conf.symfs_layout_flat) {
> > +		char *path_copy = strdup(path);
> > +		char *base;
> > +		int ret;
> > +
> > +		if (!path_copy)
> > +			return -ENOMEM;
> > +		base = basename(path_copy);
> > +		ret = path__join(bf, size, symbol_conf.symfs, base);
> > +		free(path_copy);
> > +		return ret;
> > +	}
> >  	return path__join(bf, size, symbol_conf.symfs, path);
> >  }
> >  
> > @@ -169,6 +182,11 @@ size_t symbol__fprintf_symname(const struct symbol *sym, FILE *fp);
> >  size_t symbol__fprintf(struct symbol *sym, FILE *fp);
> >  bool symbol__restricted_filename(const char *filename,
> >  				 const char *restricted_filename);
> > +
> > +#define SYMFS_HELP "setup root directory which contains debug files:\n" \
> > +	"\t\t\t\t" "directory:\tLook for files with symbols relative to this directory.\n" \
> > +	"\t\t\t\t" "layout:   \tLayout of files, 'hierarchy' matches full path (default), 'flat' only matches base name.\n"
> > +
> >  int symbol__config_symfs(const struct option *opt __maybe_unused,
> >  			 const char *dir, int unset __maybe_unused);
> >  
> > diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
> > index 71bb17372a6c..ac1b444a8fd8 100644
> > --- a/tools/perf/util/symbol_conf.h
> > +++ b/tools/perf/util/symbol_conf.h
> > @@ -93,6 +93,7 @@ struct symbol_conf {
> >  			*tid_list,
> >  			*addr_list;
> >  	const char	*symfs;
> > +	bool		symfs_layout_flat;
> >  	int		res_sample;
> >  	int		pad_output_len_dso;
> >  	int		group_sort_idx;
> > -- 
> > 2.43.0
> > 
> 

-- 
Cheers,
Changbin Du