* [PATCH] perf: Support for Openembedded/Yocto -dbg packages @ 2013-09-18 8:43 Ricardo Ribalda Delgado 2013-09-18 9:24 ` Ingo Molnar 0 siblings, 1 reply; 11+ messages in thread From: Ricardo Ribalda Delgado @ 2013-09-18 8:43 UTC (permalink / raw) To: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Waiman Long, Stephane Eranian, Jiri Olsa, David Ahern, linux-kernel Cc: Ricardo Ribalda Delgado 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. 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', @@ -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); + } + break; + case DSO_BINARY_TYPE__BUILDID_DEBUGINFO: if (!dso->has_build_id) { ret = -1; diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h index d51aaf2..4f78e7b 100644 --- a/tools/perf/util/dso.h +++ b/tools/perf/util/dso.h @@ -20,6 +20,7 @@ enum dso_binary_type { DSO_BINARY_TYPE__SYSTEM_PATH_DSO, DSO_BINARY_TYPE__GUEST_KMODULE, DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE, + DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO, DSO_BINARY_TYPE__NOT_FOUND, }; diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c index d5528e1..f69d57b 100644 --- a/tools/perf/util/symbol.c +++ b/tools/perf/util/symbol.c @@ -51,6 +51,7 @@ static enum dso_binary_type binary_type_symtab[] = { DSO_BINARY_TYPE__SYSTEM_PATH_DSO, DSO_BINARY_TYPE__GUEST_KMODULE, DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE, + DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO, DSO_BINARY_TYPE__NOT_FOUND, }; -- 1.8.4.rc3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] perf: Support for Openembedded/Yocto -dbg packages 2013-09-18 8:43 [PATCH] perf: Support for Openembedded/Yocto -dbg packages Ricardo Ribalda Delgado @ 2013-09-18 9:24 ` Ingo Molnar 2013-09-18 9:47 ` Ricardo Ribalda Delgado 0 siblings, 1 reply; 11+ messages in thread From: Ingo Molnar @ 2013-09-18 9:24 UTC (permalink / raw) To: Ricardo Ribalda Delgado Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Waiman Long, Stephane Eranian, Jiri Olsa, David Ahern, linux-kernel * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf: Support for Openembedded/Yocto -dbg packages 2013-09-18 9:24 ` Ingo Molnar @ 2013-09-18 9:47 ` Ricardo Ribalda Delgado 2013-09-18 10:02 ` Ricardo Ribalda Delgado 0 siblings, 1 reply; 11+ messages in thread From: Ricardo Ribalda Delgado @ 2013-09-18 9:47 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Waiman Long, Stephane Eranian, Jiri Olsa, David Ahern, LKML 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf: Support for Openembedded/Yocto -dbg packages 2013-09-18 9:47 ` Ricardo Ribalda Delgado @ 2013-09-18 10:02 ` Ricardo Ribalda Delgado 2013-09-18 10:18 ` Peter Zijlstra 0 siblings, 1 reply; 11+ messages in thread From: Ricardo Ribalda Delgado @ 2013-09-18 10:02 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Waiman Long, Stephane Eranian, Jiri Olsa, David Ahern, LKML Hello again Ingo: Perhaps this is even more clear than v2: len = snprintf(file, size, "%s", symbol_conf.symfs); size -= len; file += len; len = snprintf(file, MIN(size,(last_slash - dso->long_name) + 2), "%s", dso->long_name); size -= len; file += len; len = snprintf(file, size, ".debug%s", last_slash); Just tell me if you prefer this and I send v3. Thanks! On Wed, Sep 18, 2013 at 11:47 AM, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote: > 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 -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf: Support for Openembedded/Yocto -dbg packages 2013-09-18 10:02 ` Ricardo Ribalda Delgado @ 2013-09-18 10:18 ` Peter Zijlstra 2013-09-18 13:07 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 11+ messages in thread From: Peter Zijlstra @ 2013-09-18 10:18 UTC (permalink / raw) To: Ricardo Ribalda Delgado Cc: Ingo Molnar, Paul Mackerras, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Waiman Long, Stephane Eranian, Jiri Olsa, David Ahern, LKML On Wed, Sep 18, 2013 at 12:02:12PM +0200, Ricardo Ribalda Delgado wrote: > Hello again Ingo: > > Perhaps this is even more clear than v2: > > len = snprintf(file, size, "%s", symbol_conf.symfs); > size -= len; > file += len; > len = snprintf(file, MIN(size,(last_slash - dso->long_name) + 2), > "%s", dso->long_name); > size -= len; > file += len; > len = snprintf(file, size, ".debug%s", last_slash); > len = 0; len += snprintf(str + len, size - len, ...); len += snprintf(str + len, size - len, ...); ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf: Support for Openembedded/Yocto -dbg packages 2013-09-18 10:18 ` Peter Zijlstra @ 2013-09-18 13:07 ` Arnaldo Carvalho de Melo 2013-09-18 13:29 ` Ingo Molnar 0 siblings, 1 reply; 11+ messages in thread From: Arnaldo Carvalho de Melo @ 2013-09-18 13:07 UTC (permalink / raw) To: Peter Zijlstra Cc: Ricardo Ribalda Delgado, Ingo Molnar, Paul Mackerras, Ingo Molnar, Namhyung Kim, Waiman Long, Stephane Eranian, Jiri Olsa, David Ahern, LKML Em Wed, Sep 18, 2013 at 12:18:01PM +0200, Peter Zijlstra escreveu: > On Wed, Sep 18, 2013 at 12:02:12PM +0200, Ricardo Ribalda Delgado wrote: > > Perhaps this is even more clear than v2: > > len = snprintf(file, size, "%s", symbol_conf.symfs); > > size -= len; > > file += len; > > len = snprintf(file, MIN(size,(last_slash - dso->long_name) + 2), > > "%s", dso->long_name); > > size -= len; > > file += len; > > len = snprintf(file, size, ".debug%s", last_slash); > len = 0; > len += snprintf(str + len, size - len, ...); > len += snprintf(str + len, size - len, ...); And avoid snprintf like the plague, use scnprintf instead... See e7f01d1e3d8d501deb8abeaa269d5d48a703b8b0 for details :-) - Arnaldo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf: Support for Openembedded/Yocto -dbg packages 2013-09-18 13:07 ` Arnaldo Carvalho de Melo @ 2013-09-18 13:29 ` Ingo Molnar 2013-09-18 13:58 ` Ricardo Ribalda Delgado ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Ingo Molnar @ 2013-09-18 13:29 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Peter Zijlstra, Ricardo Ribalda Delgado, Paul Mackerras, Ingo Molnar, Namhyung Kim, Waiman Long, Stephane Eranian, Jiri Olsa, David Ahern, LKML * Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote: > Em Wed, Sep 18, 2013 at 12:18:01PM +0200, Peter Zijlstra escreveu: > > On Wed, Sep 18, 2013 at 12:02:12PM +0200, Ricardo Ribalda Delgado wrote: > > > Perhaps this is even more clear than v2: > > > > len = snprintf(file, size, "%s", symbol_conf.symfs); > > > size -= len; > > > file += len; > > > len = snprintf(file, MIN(size,(last_slash - dso->long_name) + 2), > > > "%s", dso->long_name); > > > size -= len; > > > file += len; > > > len = snprintf(file, size, ".debug%s", last_slash); > > > len = 0; > > > len += snprintf(str + len, size - len, ...); > > len += snprintf(str + len, size - len, ...); > > And avoid snprintf like the plague, use scnprintf instead... See > e7f01d1e3d8d501deb8abeaa269d5d48a703b8b0 for details :-) Hm, could we do: #define snprintf scnprintf or: #define snprintf(x...) BUILD_BUG() ? I don't think there's any valid code, except printf wrappers (which we don't have in perf), where the semantics of snprintf() would be needed. Thanks, Ingo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf: Support for Openembedded/Yocto -dbg packages 2013-09-18 13:29 ` Ingo Molnar @ 2013-09-18 13:58 ` Ricardo Ribalda Delgado 2013-09-18 14:08 ` Arnaldo Carvalho de Melo 2013-10-02 0:54 ` Namhyung Kim 2 siblings, 0 replies; 11+ messages in thread From: Ricardo Ribalda Delgado @ 2013-09-18 13:58 UTC (permalink / raw) To: Ingo Molnar Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, Waiman Long, Stephane Eranian, Jiri Olsa, David Ahern, LKML Hello Ingo Arnaldo and Peter I have just posted v3, Thanks for your comments! On Wed, Sep 18, 2013 at 3:29 PM, Ingo Molnar <mingo@kernel.org> wrote: > > * Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote: > >> Em Wed, Sep 18, 2013 at 12:18:01PM +0200, Peter Zijlstra escreveu: >> > On Wed, Sep 18, 2013 at 12:02:12PM +0200, Ricardo Ribalda Delgado wrote: >> > > Perhaps this is even more clear than v2: >> >> > > len = snprintf(file, size, "%s", symbol_conf.symfs); >> > > size -= len; >> > > file += len; >> > > len = snprintf(file, MIN(size,(last_slash - dso->long_name) + 2), >> > > "%s", dso->long_name); >> > > size -= len; >> > > file += len; >> > > len = snprintf(file, size, ".debug%s", last_slash); >> >> > len = 0; >> >> > len += snprintf(str + len, size - len, ...); >> > len += snprintf(str + len, size - len, ...); >> >> And avoid snprintf like the plague, use scnprintf instead... See >> e7f01d1e3d8d501deb8abeaa269d5d48a703b8b0 for details :-) > > Hm, could we do: > > #define snprintf scnprintf > > or: > > #define snprintf(x...) BUILD_BUG() > > ? > > I don't think there's any valid code, except printf wrappers (which we > don't have in perf), where the semantics of snprintf() would be needed. > > Thanks, > > Ingo -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf: Support for Openembedded/Yocto -dbg packages 2013-09-18 13:29 ` Ingo Molnar 2013-09-18 13:58 ` Ricardo Ribalda Delgado @ 2013-09-18 14:08 ` Arnaldo Carvalho de Melo 2013-10-02 0:54 ` Namhyung Kim 2 siblings, 0 replies; 11+ messages in thread From: Arnaldo Carvalho de Melo @ 2013-09-18 14:08 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Ricardo Ribalda Delgado, Paul Mackerras, Ingo Molnar, Namhyung Kim, Waiman Long, Stephane Eranian, Jiri Olsa, David Ahern, LKML Em Wed, Sep 18, 2013 at 03:29:03PM +0200, Ingo Molnar escreveu: > * Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote: > > Em Wed, Sep 18, 2013 at 12:18:01PM +0200, Peter Zijlstra escreveu: > > > len += snprintf(str + len, size - len, ...); > > > len += snprintf(str + len, size - len, ...); > > And avoid snprintf like the plague, use scnprintf instead... See > > e7f01d1e3d8d501deb8abeaa269d5d48a703b8b0 for details :-) > Hm, could we do: > #define snprintf scnprintf > or: > #define snprintf(x...) BUILD_BUG() > ? Yes, I prefer the later, with a short explanation, will do. - Arnaldo > I don't think there's any valid code, except printf wrappers (which we > don't have in perf), where the semantics of snprintf() would be needed. > > Thanks, > > Ingo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf: Support for Openembedded/Yocto -dbg packages 2013-09-18 13:29 ` Ingo Molnar 2013-09-18 13:58 ` Ricardo Ribalda Delgado 2013-09-18 14:08 ` Arnaldo Carvalho de Melo @ 2013-10-02 0:54 ` Namhyung Kim 2013-10-02 6:13 ` Ingo Molnar 2 siblings, 1 reply; 11+ messages in thread From: Namhyung Kim @ 2013-10-02 0:54 UTC (permalink / raw) To: Ingo Molnar Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ricardo Ribalda Delgado, Paul Mackerras, Ingo Molnar, Waiman Long, Stephane Eranian, Jiri Olsa, David Ahern, LKML Hi Ingo and Arnaldo, On Wed, 18 Sep 2013 15:29:03 +0200, Ingo Molnar wrote: > * Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote: > >> Em Wed, Sep 18, 2013 at 12:18:01PM +0200, Peter Zijlstra escreveu: >> > On Wed, Sep 18, 2013 at 12:02:12PM +0200, Ricardo Ribalda Delgado wrote: >> > > Perhaps this is even more clear than v2: >> >> > > len = snprintf(file, size, "%s", symbol_conf.symfs); >> > > size -= len; >> > > file += len; >> > > len = snprintf(file, MIN(size,(last_slash - dso->long_name) + 2), >> > > "%s", dso->long_name); >> > > size -= len; >> > > file += len; >> > > len = snprintf(file, size, ".debug%s", last_slash); >> >> > len = 0; >> >> > len += snprintf(str + len, size - len, ...); >> > len += snprintf(str + len, size - len, ...); >> >> And avoid snprintf like the plague, use scnprintf instead... See >> e7f01d1e3d8d501deb8abeaa269d5d48a703b8b0 for details :-) > > Hm, could we do: > > #define snprintf scnprintf > > or: > > #define snprintf(x...) BUILD_BUG() > > ? > > I don't think there's any valid code, except printf wrappers (which we > don't have in perf), where the semantics of snprintf() would be needed. There are some places that use snprintf() to query the actual length: tools/perf/util/srcline.c:245: size = snprintf(NULL, 0, "%s:%u", file, line) + 1; tools/perf/util/values.c:147: width = snprintf(NULL, 0, "%d", values->pid[i]); tools/perf/util/values.c:150: width = snprintf(NULL, 0, "%d", values->tid[i]); tools/perf/util/values.c:154: width = snprintf(NULL, 0, "%" PRIu64, values->value[i][j]); tools/perf/util/values.c:189: width = snprintf(NULL, 0, "%d", values->pid[i]); tools/perf/util/values.c:192: width = snprintf(NULL, 0, "%d", values->tid[i]); tools/perf/util/values.c:200: width = snprintf(NULL, 0, "%" PRIx64, values->counterrawid[j]); tools/perf/util/values.c:206: width = snprintf(NULL, 0, "%" PRIu64, values->value[i][j]); Thanks, Namhyung ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] perf: Support for Openembedded/Yocto -dbg packages 2013-10-02 0:54 ` Namhyung Kim @ 2013-10-02 6:13 ` Ingo Molnar 0 siblings, 0 replies; 11+ messages in thread From: Ingo Molnar @ 2013-10-02 6:13 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ricardo Ribalda Delgado, Paul Mackerras, Ingo Molnar, Waiman Long, Stephane Eranian, Jiri Olsa, David Ahern, LKML * Namhyung Kim <namhyung@kernel.org> wrote: > Hi Ingo and Arnaldo, > > On Wed, 18 Sep 2013 15:29:03 +0200, Ingo Molnar wrote: > > * Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote: > > > >> Em Wed, Sep 18, 2013 at 12:18:01PM +0200, Peter Zijlstra escreveu: > >> > On Wed, Sep 18, 2013 at 12:02:12PM +0200, Ricardo Ribalda Delgado wrote: > >> > > Perhaps this is even more clear than v2: > >> > >> > > len = snprintf(file, size, "%s", symbol_conf.symfs); > >> > > size -= len; > >> > > file += len; > >> > > len = snprintf(file, MIN(size,(last_slash - dso->long_name) + 2), > >> > > "%s", dso->long_name); > >> > > size -= len; > >> > > file += len; > >> > > len = snprintf(file, size, ".debug%s", last_slash); > >> > >> > len = 0; > >> > >> > len += snprintf(str + len, size - len, ...); > >> > len += snprintf(str + len, size - len, ...); > >> > >> And avoid snprintf like the plague, use scnprintf instead... See > >> e7f01d1e3d8d501deb8abeaa269d5d48a703b8b0 for details :-) > > > > Hm, could we do: > > > > #define snprintf scnprintf > > > > or: > > > > #define snprintf(x...) BUILD_BUG() > > > > ? > > > > I don't think there's any valid code, except printf wrappers (which we > > don't have in perf), where the semantics of snprintf() would be needed. > > There are some places that use snprintf() to query the actual length: > > tools/perf/util/srcline.c:245: size = snprintf(NULL, 0, "%s:%u", file, line) + 1; > tools/perf/util/values.c:147: width = snprintf(NULL, 0, "%d", values->pid[i]); > tools/perf/util/values.c:150: width = snprintf(NULL, 0, "%d", values->tid[i]); > tools/perf/util/values.c:154: width = snprintf(NULL, 0, "%" PRIu64, values->value[i][j]); > tools/perf/util/values.c:189: width = snprintf(NULL, 0, "%d", values->pid[i]); > tools/perf/util/values.c:192: width = snprintf(NULL, 0, "%d", values->tid[i]); > tools/perf/util/values.c:200: width = snprintf(NULL, 0, "%" PRIx64, values->counterrawid[j]); > tools/perf/util/values.c:206: width = snprintf(NULL, 0, "%" PRIu64, values->value[i][j]); I think that should be a separate, more obviously named helper inline instead, something like: static inline printf_width(const char *fmt, ...) { va_list args; int width; va_start(args, fmt); width = vsnprintf(NULL, 0, fmt, args); va_end(args); return width; } #define snprintf(fmt...) BUILD_BUG_ON(1, "Use scnprintf() instead of snprintf()") or so? Then we could use it like this: > tools/perf/util/values.c:147: width = printf_width("%d", values->pid[i]); and naked snprintf use would provoke a build bug. Various printf wrappers can still use vsnprintf() - it's the routine use of snprintf() instead of scnprintf() that should be inhibited. Thanks, Ingo ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-10-02 6:13 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-18 8:43 [PATCH] perf: Support for Openembedded/Yocto -dbg packages Ricardo Ribalda Delgado 2013-09-18 9:24 ` Ingo Molnar 2013-09-18 9:47 ` Ricardo Ribalda Delgado 2013-09-18 10:02 ` Ricardo Ribalda Delgado 2013-09-18 10:18 ` Peter Zijlstra 2013-09-18 13:07 ` Arnaldo Carvalho de Melo 2013-09-18 13:29 ` Ingo Molnar 2013-09-18 13:58 ` Ricardo Ribalda Delgado 2013-09-18 14:08 ` Arnaldo Carvalho de Melo 2013-10-02 0:54 ` Namhyung Kim 2013-10-02 6:13 ` Ingo Molnar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).