perf tools: Add symfs option for off-box analysis using specified tree
PATCH 1/1 perf tools: Add symfs option for off-box analysis using specified tree
The symfs argument allows analysis of perf.data file using a locally accessible filesystem tree with debug symbols - e.g., tree created during image builds, sshfs mount, loop mounted KVM disk images, USB keys, initrds, etc. Anything with an OS tree can be analyzed from anywhere without the need to populate a local data store with build-ids.
--- tools/perf/builtin-annotate.c | 16 +++++-- tools/perf/builtin-diff.c | 2 + tools/perf/builtin-report.c | 2 + tools/perf/builtin-timechart.c | 2 + tools/perf/util/hist.c | 14 +++++- tools/perf/util/symbol.c | 82 +++++++++++++++++++++++++--------------- tools/perf/util/symbol.h | 1 + 7 files changed, 80 insertions(+), 39 deletions(-)
tools/perf/builtin-annotate.c +11 -5
@@ -280,7 +280,8 @@ static int hist_entry__tty_annotate(struct hist_entry *he)struct map *map = he->ms.map;struct dso *dso = map->dso;struct symbol *sym = he->ms.sym; -const char *filename = dso->long_name, *d_filename; +const char *filename = dso->long_name; +char d_filename[PATH_MAX ];u64 len;LIST_HEAD (head);struct objdump_line *pos, *n;
@@ -288,10 +289,13 @@ static int hist_entry__tty_annotate(struct hist_entry *he)if (hist_entry__annotate (he, &head,0 ) <0 )return -1 ; -if (full_paths) - d_filename = filename; -else - d_filename =basename (filename); +if (full_paths) { +snprintf (d_filename,sizeof (d_filename),"%s%s" , + symbol_conf.symfs, filename); + }else { +snprintf (d_filename,sizeof (d_filename),"%s/%s" , + symbol_conf.symfs,basename (filename)); + } len = sym->end - sym->start;
@@ -437,6 +441,8 @@ static const struct option options[] = { "print matching source lines (may be slow)"),OPT_BOOLEAN ('P' ,"full-paths" , &full_paths,"Don't shorten the displayed pathnames" ), +OPT_STRING (0 ,"symfs" , &symbol_conf.symfs,"directory" , +"Look for files with symbols relative to this directory" ), OPT_END() };
tools/perf/builtin-diff.c +2 -0
@@ -192,6 +192,8 @@ static const struct option options[] = {OPT_STRING ('t' ,"field-separator" , &symbol_conf.field_sep,"separator" ,"separator for columns, no spaces will be added between " "columns '.' is reserved." ), +OPT_STRING (0 ,"symfs" , &symbol_conf.symfs,"directory" , +"Look for files with symbols relative to this directory" ), OPT_END() };
tools/perf/builtin-report.c +2 -0
@@ -479,6 +479,8 @@ static const struct option options[] = { "columns '.' is reserved."),OPT_BOOLEAN ('U' ,"hide-unresolved" , &hide_unresolved,"Only display entries resolved to a symbol" ), +OPT_STRING (0 ,"symfs" , &symbol_conf.symfs,"directory" , +"Look for files with symbols relative to this directory" ), OPT_END() };
tools/perf/builtin-timechart.c +2 -0
@@ -1021,6 +1021,8 @@ static const struct option options[] = {OPT_CALLBACK ('p' ,"process" ,NULL ,"process" ,"process selector. Pass a pid or process name." , parse_process), +OPT_STRING (0 ,"symfs" , &symbol_conf.symfs,"directory" , +"Look for files with symbols relative to this directory" ), OPT_END() };
tools/perf/util/hist.c +11 -3
@@ -1092,6 +1092,12 @@ int hist_entry__annotate(struct hist_entry *self, struct list_head *head,FILE *file;int err =0 ;u64 len; +char symfs_filename[PATH_MAX ]; + +if (filename) { +snprintf (symfs_filename,sizeof (symfs_filename),"%s%s" , + symbol_conf.symfs, filename); + }if (filename ==NULL ) {if (dso->has_build_id) {
@@ -1100,9 +1106,9 @@ int hist_entry__annotate(struct hist_entry *self, struct list_head *head,return -ENOMEM ; } goto fallback; - }else if (readlink (filename, command,sizeof (command)) <0 || + }else if (readlink (symfs_filename, command,sizeof (command)) <0 ||strstr (command,"[kernel.kallsyms]" ) || -access (filename,R_OK )) { +access (symfs_filename,R_OK )) {free (filename); fallback:/*
@@ -1111,6 +1117,8 @@ fallback:* DSO is the same as when 'perf record' ran. */ filename = dso->long_name; +snprintf (symfs_filename,sizeof (symfs_filename),"%s%s" , + symbol_conf.symfs, filename); free_filename = false; }
@@ -1137,7 +1145,7 @@ fallback:"objdump --start-address=0x%016Lx --stop-address=0x%016Lx -dS -C %s|grep -v %s|expand" ,map__rip_2objdump (map, sym->start),map__rip_2objdump (map, sym->end), - filename, filename); + symfs_filename, filename);pr_debug ("Executing: %s\n" , command);
tools/perf/util/symbol.c +51 -31
147 lines not on this page · raw message
tools/perf/util/symbol.h +1 -0
@@ -85,6 +85,7 @@ struct symbol_conf {struct strlist *dso_list, *comm_list, *sym_list; +const char *symfs; };extern struct symbol_conf symbol_conf;
Signature · 1 line
1.7.2.3
Em Tue, Dec 07, 2010 at 07:42:54PM -0700, David Ahern escreveu:
Quoted 37 lines
The symfs argument allows analysis of perf.data file using a locally accessible filesystem tree with debug symbols - e.g., tree created during image builds, sshfs mount, loop mounted KVM disk images, USB keys, initrds, etc. Anything with an OS tree can be analyzed from anywhere without the need to populate a local data store with build-ids.
Signed-off-by David Ahern <[email protected]>--- tools/perf/builtin-annotate.c | 16 +++++-- tools/perf/builtin-diff.c | 2 + tools/perf/builtin-report.c | 2 + tools/perf/builtin-timechart.c | 2 + tools/perf/util/hist.c | 14 +++++- tools/perf/util/symbol.c | 82 +++++++++++++++++++++++++--------------- tools/perf/util/symbol.h | 1 + 7 files changed, 80 insertions(+), 39 deletions(-)tools/perf/builtin-annotate.c +2
-2@@ -280,7 +280,8 @@ static int hist_entry__tty_annotate(struct hist_entry *he)struct map *map = he->ms.map;struct dso *dso = map->dso;struct symbol *sym = he->ms.sym; -const char *filename = dso->long_name, *d_filename; +const char *filename = dso->long_name; +char d_filename[PATH_MAX ];u64 len;LIST_HEAD (head);struct objdump_line *pos, *n;@@ -288,10 +289,13 @@ static int hist_entry__tty_annotate(struct hist_entry *he)if (hist_entry__annotate (he, &head,0 ) <0 )return -1 ; -if (full_paths)
tools/perf/builtin-annotate.c static int hist_entry__tty_annotate(struct hist_entry *he) · line 298
Quoted 10 lines
tools/perf/builtin-annotate.c +7
-3- d_filename = filename; - else - d_filename =basename (filename); +if (full_paths) { +snprintf (d_filename,sizeof (d_filename),"%s%s" , + symbol_conf.symfs, filename); + }else { +snprintf (d_filename,sizeof (d_filename),"%s/%s" , + symbol_conf.symfs,basename (filename)); + }
Are you sure about the one above? I guess you don't need to concat here, its just informative message, if you're using --symfs, you know everything is from there, and in the !full_paths case the resulting filename will be bogus, right?
Quoted 55 lines
tools/perf/builtin-annotate.c +2
-0len = sym->end - sym->start;@@ -437,6 +441,8 @@ static const struct option options[] = { "print matching source lines (may be slow)"),OPT_BOOLEAN ('P' ,"full-paths" , &full_paths,"Don't shorten the displayed pathnames" ), +OPT_STRING (0 ,"symfs" , &symbol_conf.symfs,"directory" , +"Look for files with symbols relative to this directory" ), OPT_END() };tools/perf/builtin-diff.c +2
-0@@ -192,6 +192,8 @@ static const struct option options[] = {OPT_STRING ('t' ,"field-separator" , &symbol_conf.field_sep,"separator" ,"separator for columns, no spaces will be added between " "columns '.' is reserved." ), +OPT_STRING (0 ,"symfs" , &symbol_conf.symfs,"directory" , +"Look for files with symbols relative to this directory" ), OPT_END() };tools/perf/builtin-report.c +2
-0@@ -479,6 +479,8 @@ static const struct option options[] = { "columns '.' is reserved."),OPT_BOOLEAN ('U' ,"hide-unresolved" , &hide_unresolved,"Only display entries resolved to a symbol" ), +OPT_STRING (0 ,"symfs" , &symbol_conf.symfs,"directory" , +"Look for files with symbols relative to this directory" ), OPT_END() };tools/perf/builtin-timechart.c +2
-0@@ -1021,6 +1021,8 @@ static const struct option options[] = {OPT_CALLBACK ('p' ,"process" ,NULL ,"process" ,"process selector. Pass a pid or process name." , parse_process), +OPT_STRING (0 ,"symfs" , &symbol_conf.symfs,"directory" , +"Look for files with symbols relative to this directory" ), OPT_END() };tools/perf/util/hist.c +0
-0
tools/perf/util/hist.c int hist_entry__annotate(struct hist_entry *self, struct list_head *head, · line 1099
Quoted 8 lines
tools/perf/util/hist.c +5
-0@@ -1092,6 +1092,12 @@ int hist_entry__annotate(struct hist_entry *self, struct list_head *head,FILE *file;int err =0 ;u64 len; +char symfs_filename[PATH_MAX ]; + +if (filename) { +snprintf (symfs_filename,sizeof (symfs_filename),"%s%s" , + symbol_conf.symfs, filename);
Above you have tab/space damage
Quoted 17 lines
tools/perf/util/hist.c +3
-2+ }if (filename ==NULL ) {if (dso->has_build_id) {@@ -1100,9 +1106,9 @@ int hist_entry__annotate(struct hist_entry *self, struct list_head *head,return -ENOMEM ; } goto fallback; - }else if (readlink (filename, command,sizeof (command)) <0 || + }else if (readlink (symfs_filename, command,sizeof (command)) <0 ||strstr (command,"[kernel.kallsyms]" ) || -access (filename,R_OK )) { +access (symfs_filename,R_OK )) {free (filename); fallback:/*
tools/perf/util/hist.c fallback: · line 1121
Quoted 5 lines
tools/perf/util/hist.c +2
-0@@ -1111,6 +1117,8 @@ fallback:* DSO is the same as when 'perf record' ran. */ filename = dso->long_name; +snprintf (symfs_filename,sizeof (symfs_filename),"%s%s" , + symbol_conf.symfs, filename);
Ditto above
Quoted 38 lines
tools/perf/util/hist.c +1
-1free_filename = false; }@@ -1137,7 +1145,7 @@ fallback:"objdump --start-address=0x%016Lx --stop-address=0x%016Lx -dS -C %s|grep -v %s|expand" ,map__rip_2objdump (map, sym->start),map__rip_2objdump (map, sym->end), - filename, filename); + symfs_filename, filename);pr_debug ("Executing: %s\n" , command);tools/perf/util/symbol.c +5
-1@@ -41,6 +41,7 @@ struct symbol_conf symbol_conf = { .exclude_other = true, .use_modules = true, .try_vmlinux_path = true, + .symfs = "", };int dso__name_len (const struct dso *self)@@ -833,8 +834,11 @@ static int dso__synthesize_plt_symbols(struct dso *self, struct map *map,char sympltname[1024 ];Elf *elf;int nr =0 , symidx, fd, err =0 ; +char name[PATH_MAX ]; - fd =open (self->long_name,O_RDONLY ); +snprintf (name,sizeof (name),"%s%s" , + symbol_conf.symfs, self->long_name); + fd =open (name,O_RDONLY );if (fd <0 ) goto out;
tools/perf/util/symbol.c int dso__load(struct dso *self, struct map *map, symbol_filter_t filter) · line 1454
Quoted 6 lines
tools/perf/util/symbol.c +2
-1@@ -1446,16 +1450,19 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter) self->origin++) {switch (self->origin) {case DSO__ORIG_BUILD_ID_CACHE : -if (dso__build_id_filename (self, name, size) ==NULL ) +/* skip the locally configured cache if a symfs is given */ + if ((strlen(symbol_conf.symfs) != 0) ||
I suggest you use:
if (symbol_conf.symfs[1] ||
cheaper :)
Quoted 36 lines
tools/perf/util/symbol.c +10
-7+ (dso__build_id_filename(self, name, size) == NULL)) {continue ; + }break ;case DSO__ORIG_FEDORA : -snprintf (name, size,"/usr/lib/debug%s.debug" , - self->long_name); +snprintf (name, size,"%s/usr/lib/debug%s.debug" , + symbol_conf.symfs, self->long_name);break ;case DSO__ORIG_UBUNTU : -snprintf (name, size,"/usr/lib/debug%s" , - self->long_name); +snprintf (name, size,"%s/usr/lib/debug%s" , + symbol_conf.symfs, self->long_name);break ;case DSO__ORIG_BUILDID : {char build_id_hex[BUILD_ID_SIZE *2 +1 ];@@ -1467,12 +1474,13 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter)sizeof (self->build_id), build_id_hex);snprintf (name, size, -"/usr/lib/debug/.build-id/%.2s/%s.debug" , - build_id_hex, build_id_hex +2 ); +"%s/usr/lib/debug/.build-id/%.2s/%s.debug" , + symbol_conf.symfs, build_id_hex, build_id_hex +2 ); }break ;case DSO__ORIG_DSO : -snprintf (name, size,"%s" , self->long_name); +snprintf (name, size,"%s%s" , + symbol_conf.symfs, self->long_name);break ;case DSO__ORIG_GUEST_KMODULE :if (map->groups && map->groups->machine)
tools/perf/util/symbol.c static int dso__load_vmlinux(struct dso *self, struct map *map, · line 1791
Quoted 7 lines
tools/perf/util/symbol.c +3
-1@@ -1778,17 +1786,20 @@ static int dso__load_vmlinux(struct dso *self, struct map *map, const char *vmlinux, symbol_filter_t filter) { int err = -1, fd; +char symfs_vmlinux[PATH_MAX ]; - fd =open (vmlinux,O_RDONLY ); +snprintf (symfs_vmlinux,sizeof (symfs_vmlinux),"%s/%s" , + symbol_conf.symfs, vmlinux);
Its userspace, so stack is plenty, but I guess if using asprintf wouldn't be better...
I.e. if we were programming some kernel module, creating a 4K stack variable would be considered bad practice, so I think it is here as well.
Quoted 49 lines
tools/perf/util/symbol.c +7
-14+ fd =open (symfs_vmlinux,O_RDONLY );if (fd <0 )return -1 ;dso__set_loaded (self, map->type); - err =dso__load_sym (self, map, vmlinux, fd, filter,0 ,0 ); + err =dso__load_sym (self, map, symfs_vmlinux, fd, filter,0 ,0 );close (fd);if (err >0 ) -pr_debug ("Using %s for symbols\n" , vmlinux); +pr_debug ("Using %s for symbols\n" , symfs_vmlinux);return err; }@@ -1861,6 +1872,10 @@ static int dso__load_kernel_sym(struct dso *self, struct map *map, goto out_fixup; } +/* do not try local files if a symfs was given */ +if (strlen (symbol_conf.symfs) !=0 ) +return -1 ; +/* * Say the kernel DSO was created when processing the build-id header table, * we have a build-id, so check if it is the same as the running kernel, @@ -2210,9 +2225,6 @@ static int vmlinux_path__init(void)struct utsname uts;char bf[PATH_MAX ]; -if (uname (&uts) <0 ) -return -1 ; - vmlinux_path =malloc (sizeof (char *) *5 );if (vmlinux_path ==NULL )return -1 ;@@ -2225,22 +2237,30 @@ static int vmlinux_path__init(void)if (vmlinux_path[vmlinux_path__nr_entries] ==NULL ) goto out_fail; ++vmlinux_path__nr_entries; -snprintf (bf,sizeof (bf),"/boot/vmlinux-%s" , uts.release); - vmlinux_path[vmlinux_path__nr_entries] =strdup (bf); -if (vmlinux_path[vmlinux_path__nr_entries] ==NULL ) - goto out_fail; - ++vmlinux_path__nr_entries; -snprintf (bf,sizeof (bf),"/lib/modules/%s/build/vmlinux" , uts.release); - vmlinux_path[vmlinux_path__nr_entries] =strdup (bf); -if (vmlinux_path[vmlinux_path__nr_entries] ==NULL ) - goto out_fail;
tools/perf/util/symbol.c static int vmlinux_path__init(void) · line 2242
Quoted 10 lines
tools/perf/util/symbol.c +3
-7- ++vmlinux_path__nr_entries; -snprintf (bf,sizeof (bf),"/usr/lib/debug/lib/modules/%s/vmlinux" , - uts.release); - vmlinux_path[vmlinux_path__nr_entries] =strdup (bf); -if (vmlinux_path[vmlinux_path__nr_entries] ==NULL ) - goto out_fail; - ++vmlinux_path__nr_entries; + +/* if no symfs has been given try running kernel version too */ +if (strlen (symbol_conf.symfs) ==0 ) {
Do it as:
if (!symbol_conf.symfs[1]) return 0;
And then the patch can be made smaller.
Quoted 43 lines
tools/perf/util/symbol.c +21
-0+if (uname (&uts) <0 ) +return -1 ; + +snprintf (bf,sizeof (bf),"/boot/vmlinux-%s" , uts.release); + vmlinux_path[vmlinux_path__nr_entries] =strdup (bf); +if (vmlinux_path[vmlinux_path__nr_entries] ==NULL ) + goto out_fail; + ++vmlinux_path__nr_entries; +snprintf (bf,sizeof (bf),"/lib/modules/%s/build/vmlinux" , + uts.release); + vmlinux_path[vmlinux_path__nr_entries] =strdup (bf); +if (vmlinux_path[vmlinux_path__nr_entries] ==NULL ) + goto out_fail; + ++vmlinux_path__nr_entries; +snprintf (bf,sizeof (bf),"/usr/lib/debug/lib/modules/%s/vmlinux" , + uts.release); + vmlinux_path[vmlinux_path__nr_entries] =strdup (bf); +if (vmlinux_path[vmlinux_path__nr_entries] ==NULL ) + goto out_fail; + ++vmlinux_path__nr_entries; + }return 0 ;tools/perf/util/symbol.h +1
-0@@ -85,6 +85,7 @@ struct symbol_conf {struct strlist *dso_list, *comm_list, *sym_list; +const char *symfs; };extern struct symbol_conf symbol_conf;Signature · 6 lines
1.7.2.3 -- To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 37 lines · Arnaldo Carvalho de Melo
Em Tue, Dec 07, 2010 at 07:42:54PM -0700, David Ahern escreveu:
Quoted 36 lines
The symfs argument allows analysis of perf.data file using a locally accessible filesystem tree with debug symbols - e.g., tree created during image builds, sshfs mount, loop mounted KVM disk images, USB keys, initrds, etc. Anything with an OS tree can be analyzed from anywhere without the need to populate a local data store with build-ids.
Signed-off-by David Ahern <[email protected]>--- tools/perf/builtin-annotate.c | 16 +++++-- tools/perf/builtin-diff.c | 2 + tools/perf/builtin-report.c | 2 + tools/perf/builtin-timechart.c | 2 + tools/perf/util/hist.c | 14 +++++- tools/perf/util/symbol.c | 82 +++++++++++++++++++++++++--------------- tools/perf/util/symbol.h | 1 + 7 files changed, 80 insertions(+), 39 deletions(-)tools/perf/builtin-annotate.c +2
-1@@ -280,7 +280,8 @@ static int hist_entry__tty_annotate(struct hist_entry *he)struct map *map = he->ms.map;struct dso *dso = map->dso;struct symbol *sym = he->ms.sym; -const char *filename = dso->long_name, *d_filename; +const char *filename = dso->long_name; +char d_filename[PATH_MAX ];u64 len;LIST_HEAD (head);struct objdump_line *pos, *n;@@ -288,10 +289,13 @@ static int hist_entry__tty_annotate(struct hist_entry *he)if (hist_entry__annotate (he, &head,0 ) <0 )return -1 ;
Quoted 16 lines
tools/perf/builtin-annotate.c static int hist_entry__tty_annotate(struct hist_entry *he) · line 298
Quoted 11 lines
tools/perf/builtin-annotate.c +7
-4-if (full_paths) - d_filename = filename; -else - d_filename =basename (filename); +if (full_paths) { +snprintf (d_filename,sizeof (d_filename),"%s%s" , + symbol_conf.symfs, filename); + }else { +snprintf (d_filename,sizeof (d_filename),"%s/%s" , + symbol_conf.symfs,basename (filename)); + }Are you sure about the one above? I guess you don't need to concat here, its just informative message, if you're using --symfs, you know everything is from there, and in the !full_paths case the resulting filename will be bogus, right?
Ok. I removed.
Quoted 11 lines
tools/perf/builtin-annotate.c static const struct option options[] = { · line 447
Quoted 11 lines
tools/perf/builtin-annotate.c +2
-0len = sym->end - sym->start;@@ -437,6 +441,8 @@ static const struct option options[] = { "print matching source lines (may be slow)"),OPT_BOOLEAN ('P' ,"full-paths" , &full_paths,"Don't shorten the displayed pathnames" ), +OPT_STRING (0 ,"symfs" , &symbol_conf.symfs,"directory" , +"Look for files with symbols relative to this directory" ), OPT_END() };
In which case the above is not needed.
Quoted 7 lines
Quoted 7 lines
tools/perf/util/hist.c +0
-0@@ -1092,6 +1092,12 @@ int hist_entry__annotate(struct hist_entry *self, struct list_head *head,FILE *file;int err =0 ;
Quoted 8 lines
tools/perf/util/hist.c int hist_entry__annotate(struct hist_entry *self, struct list_head *head, · line 1099
Quoted 6 lines
tools/perf/util/hist.c +5
-0u64 len; +char symfs_filename[PATH_MAX ]; + +if (filename) { +snprintf (symfs_filename,sizeof (symfs_filename),"%s%s" , + symbol_conf.symfs, filename);Above you have tab/space damage
fixed
Quoted 17 lines
Quoted 17 lines
tools/perf/util/hist.c +3
-2+ }if (filename ==NULL ) {if (dso->has_build_id) {@@ -1100,9 +1106,9 @@ int hist_entry__annotate(struct hist_entry *self, struct list_head *head,return -ENOMEM ; } goto fallback; - }else if (readlink (filename, command,sizeof (command)) <0 || + }else if (readlink (symfs_filename, command,sizeof (command)) <0 ||strstr (command,"[kernel.kallsyms]" ) || -access (filename,R_OK )) { +access (symfs_filename,R_OK )) {free (filename); fallback:/*
Quoted 8 lines
tools/perf/util/hist.c fallback: · line 1121
Quoted 6 lines
tools/perf/util/hist.c +2
-0@@ -1111,6 +1117,8 @@ fallback:* DSO is the same as when 'perf record' ran. */ filename = dso->long_name; +snprintf (symfs_filename,sizeof (symfs_filename),"%s%s" , + symbol_conf.symfs, filename);Ditto above
fixed
Quoted 48 lines
Quoted 39 lines
tools/perf/util/hist.c +1
-1free_filename = false; }@@ -1137,7 +1145,7 @@ fallback:"objdump --start-address=0x%016Lx --stop-address=0x%016Lx -dS -C %s|grep -v %s|expand" ,map__rip_2objdump (map, sym->start),map__rip_2objdump (map, sym->end), - filename, filename); + symfs_filename, filename);pr_debug ("Executing: %s\n" , command);tools/perf/util/symbol.c +5
-1@@ -41,6 +41,7 @@ struct symbol_conf symbol_conf = { .exclude_other = true, .use_modules = true, .try_vmlinux_path = true, + .symfs = "", };int dso__name_len (const struct dso *self)@@ -833,8 +834,11 @@ static int dso__synthesize_plt_symbols(struct dso *self, struct map *map,char sympltname[1024 ];Elf *elf;int nr =0 , symidx, fd, err =0 ; +char name[PATH_MAX ]; - fd =open (self->long_name,O_RDONLY ); +snprintf (name,sizeof (name),"%s%s" , + symbol_conf.symfs, self->long_name); + fd =open (name,O_RDONLY );if (fd <0 ) goto out;tools/perf/util/symbol.c int dso__load(struct dso *self, struct map *map, symbol_filter_t filter) · line 1454
Quoted 6 lines
tools/perf/util/symbol.c +2
-1@@ -1446,16 +1450,19 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter) self->origin++) {switch (self->origin) {case DSO__ORIG_BUILD_ID_CACHE : -if (dso__build_id_filename (self, name, size) ==NULL ) +/* skip the locally configured cache if a symfs is given */ + if ((strlen(symbol_conf.symfs) != 0) ||I suggest you use:
Quoted 3 lines
if (symbol_conf.symfs[1] ||cheaper :)
Changed. I presume you meant: symbol_conf.symfs[0] -- ie., element 0 has something other than '\0' which means strlen > 0
...
Quoted 12 lines
tools/perf/util/symbol.c static int dso__load_vmlinux(struct dso *self, struct map *map, · line 1791
Quoted 8 lines
tools/perf/util/symbol.c +3
-1@@ -1778,17 +1786,20 @@ static int dso__load_vmlinux(struct dso *self, struct map *map, const char *vmlinux, symbol_filter_t filter) { int err = -1, fd; +char symfs_vmlinux[PATH_MAX ]; - fd =open (vmlinux,O_RDONLY ); +snprintf (symfs_vmlinux,sizeof (symfs_vmlinux),"%s/%s" , + symbol_conf.symfs, vmlinux);Its userspace, so stack is plenty, but I guess if using asprintf wouldn't be better...
Quoted 3 lines
I.e. if we were programming some kernel module, creating a 4K stack variable would be considered bad practice, so I think it is here as well.
Declaring symfs_vmlinux on the stack is consistent with other PATH_MAX declarations within perf, and especially within util/symbol.c.
Quoted 20 lines
Quoted 20 lines
tools/perf/util/symbol.c +3
-2+ fd =open (symfs_vmlinux,O_RDONLY );if (fd <0 )return -1 ;dso__set_loaded (self, map->type); - err =dso__load_sym (self, map, vmlinux, fd, filter,0 ,0 ); + err =dso__load_sym (self, map, symfs_vmlinux, fd, filter,0 ,0 );close (fd);if (err >0 ) -pr_debug ("Using %s for symbols\n" , vmlinux); +pr_debug ("Using %s for symbols\n" , symfs_vmlinux);return err; }@@ -1861,6 +1872,10 @@ static int dso__load_kernel_sym(struct dso *self, struct map *map, goto out_fixup; }
Quoted 4 lines
tools/perf/util/symbol.c static int dso__load_kernel_sym(struct dso *self, struct map *map, · line 1878
Quoted 4 lines
tools/perf/util/symbol.c +4
-0+/* do not try local files if a symfs was given */ +if (strlen (symbol_conf.symfs) !=0 ) +return -1 ; +
changed that strlen as well.
Quoted 39 lines
Quoted 26 lines
tools/perf/util/symbol.c +0
-12/* * Say the kernel DSO was created when processing the build-id header table, * we have a build-id, so check if it is the same as the running kernel, @@ -2210,9 +2225,6 @@ static int vmlinux_path__init(void)struct utsname uts;char bf[PATH_MAX ]; -if (uname (&uts) <0 ) -return -1 ; - vmlinux_path =malloc (sizeof (char *) *5 );if (vmlinux_path ==NULL )return -1 ;@@ -2225,22 +2237,30 @@ static int vmlinux_path__init(void)if (vmlinux_path[vmlinux_path__nr_entries] ==NULL ) goto out_fail; ++vmlinux_path__nr_entries; -snprintf (bf,sizeof (bf),"/boot/vmlinux-%s" , uts.release); - vmlinux_path[vmlinux_path__nr_entries] =strdup (bf); -if (vmlinux_path[vmlinux_path__nr_entries] ==NULL ) - goto out_fail; - ++vmlinux_path__nr_entries; -snprintf (bf,sizeof (bf),"/lib/modules/%s/build/vmlinux" , uts.release); - vmlinux_path[vmlinux_path__nr_entries] =strdup (bf); -if (vmlinux_path[vmlinux_path__nr_entries] ==NULL ) - goto out_fail;tools/perf/util/symbol.c static int vmlinux_path__init(void) · line 2242
Quoted 10 lines
tools/perf/util/symbol.c +3
-7- ++vmlinux_path__nr_entries; -snprintf (bf,sizeof (bf),"/usr/lib/debug/lib/modules/%s/vmlinux" , - uts.release); - vmlinux_path[vmlinux_path__nr_entries] =strdup (bf); -if (vmlinux_path[vmlinux_path__nr_entries] ==NULL ) - goto out_fail; - ++vmlinux_path__nr_entries; + +/* if no symfs has been given try running kernel version too */ +if (strlen (symbol_conf.symfs) ==0 ) {Do it as:
Quoted 4 lines
if (!symbol_conf.symfs[1]) return 0;And then the patch can be made smaller.
Done.
I also added the symfs option to the Documentation files. Will send an updated patch.
David
Em Wed, Dec 08, 2010 at 03:22:37PM -0700, David S. Ahern escreveu:
Quoted 6 lines · Arnaldo Carvalho de Melo
Quoted 5 lines
Em Tue, Dec 07, 2010 at 07:42:54PM -0700, David Ahern escreveu:
tools/perf/util/symbol.c int dso__load(struct dso *self, struct map *map, symbol_filter_t filter) · line 1454
Quoted 1 line
tools/perf/util/symbol.c +1
-0+ if ((strlen(symbol_conf.symfs) != 0) ||I suggest you use:
Quoted 6 lines
Quoted 3 lines
if (symbol_conf.symfs[1] ||cheaper :)
Changed. I presume you meant: symbol_conf.symfs[0] -- ie., element 0 has something other than '\0' which means strlen > 0
Right, off-by-one mistake by me, sorry
Quoted 12 lines
Quoted 12 lines
tools/perf/util/symbol.c static int dso__load_vmlinux(struct dso *self, struct map *map, · line 1791
Quoted 8 lines
tools/perf/util/symbol.c +3
-1@@ -1778,17 +1786,20 @@ static int dso__load_vmlinux(struct dso *self, struct map *map, const char *vmlinux, symbol_filter_t filter) { int err = -1, fd; +char symfs_vmlinux[PATH_MAX ]; - fd =open (vmlinux,O_RDONLY ); +snprintf (symfs_vmlinux,sizeof (symfs_vmlinux),"%s/%s" , + symbol_conf.symfs, vmlinux);Its userspace, so stack is plenty, but I guess if using asprintf wouldn't be better...
Quoted 6 lines
Quoted 3 lines
I.e. if we were programming some kernel module, creating a 4K stack variable would be considered bad practice, so I think it is here as well.
Declaring
symfs_vmlinuxon the stack is consistent with otherPATH_MAXdeclarations within perf, and especially withinutil/symbol.c.
Ok, so keep it like that and I'll then work on a follow on patch to remove these stack abuses.
Quoted 7 lines
Quoted 7 lines
tools/perf/util/symbol.c static int vmlinux_path__init(void) · line 2242
Quoted 1 line
tools/perf/util/symbol.c +1
-0+if (strlen (symbol_conf.symfs) ==0 ) {Do it as:
if (!symbol_conf.symfs[1]) return 0;
Quoted 7 lines
Quoted 1 line
And then the patch can be made smaller.
Done.
I also added the symfs option to the Documentation files. Will send an updated patch.
Thanks a lot!
- Arnaldo