Messages in this thread | | | From | Ricardo Ribalda Delgado <> | Date | Wed, 18 Sep 2013 11:47:11 +0200 | Subject | Re: [PATCH] perf: Support for Openembedded/Yocto -dbg packages |
| |
Hello Ingo
Incredible how many code style rules I can break in 20 lines of code :). I have just uploaded v2
Thanks for your comments.
On Wed, Sep 18, 2013 at 11:24 AM, Ingo Molnar <mingo@kernel.org> wrote: > > * Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote: > >> On OpenEmbedded the symbol files are located under a .debug folder on >> the same folder as the binary file. >> >> This patch adds support for such files. > > It would be nice to cite before/after perf report output, to see how this > improved things and to see the output format you picked. > >> >> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> >> --- >> tools/perf/util/dso.c | 16 ++++++++++++++++ >> tools/perf/util/dso.h | 1 + >> tools/perf/util/symbol.c | 1 + >> 3 files changed, 18 insertions(+) >> >> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c >> index c4374f0..bab18b7 100644 >> --- a/tools/perf/util/dso.c >> +++ b/tools/perf/util/dso.c >> @@ -14,6 +14,7 @@ char dso__symtab_origin(const struct dso *dso) >> [DSO_BINARY_TYPE__BUILD_ID_CACHE] = 'B', >> [DSO_BINARY_TYPE__FEDORA_DEBUGINFO] = 'f', >> [DSO_BINARY_TYPE__UBUNTU_DEBUGINFO] = 'u', >> + [DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO] = 'o', >> [DSO_BINARY_TYPE__BUILDID_DEBUGINFO] = 'b', >> [DSO_BINARY_TYPE__SYSTEM_PATH_DSO] = 'd', >> [DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE] = 'K', > > Stylistic nit: if a new entry breaks vertical alignment then re-align the > other entries as well so that it still looks nice after your change ... > >> @@ -64,6 +65,21 @@ int dso__binary_type_file(struct dso *dso, enum dso_binary_type type, >> symbol_conf.symfs, dso->long_name); >> break; >> >> + case DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO:{ > > /: { > >> + char *last_slash; >> + >> + last_slash = dso->long_name + dso->long_name_len; >> + while (last_slash != dso->long_name && *last_slash != '/') >> + last_slash--; >> + >> + snprintf(file, MIN(size, strlen(symbol_conf.symfs) + >> + (last_slash - dso->long_name) + 2), "%s%s", >> + symbol_conf.symfs, dso->long_name); >> + snprintf(file + strlen(file), size-strlen(file), ".debug%s", >> + last_slash); > > So the way such multi-snprintf() sequences are implemented in the kernel > is not this unreadable, fragile, repetitive concatenation of length > calculations, but an adjustment of 'size': > > size -= snprintf(..., size, ...); > > that way 'size' always tracks the remaining limit of the output string, > each snprintf consumes from it. > >> + } >> + break; > > Small nit: we generally put the final break inside the block. > > Besides the details it looks like a useful patch. > > Thanks, > > Ingo
-- Ricardo Ribalda
| |