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

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

On Thu, Mar 05, 2026 at 05:02:17PM +0800, duchangbin wrote:
> 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 ;)?
> 
Finally, I've decided to go with the simplest implementation. The result is: if
the path contains a comma, the layout must be specified, otherwise an error will
be raised (the part after the comma would be incorrectly recognized as the
layout option).

- /path → symfs=/path, layout=hierarchy
- /path,flat → symfs=/path, layout=flat
- /some,path/ → error
- /some,path/,flat → symfs=/some,path/, layout=flat

> > 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

-- 
Cheers,
Changbin Du