* [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 read-only IMAP folder(s) and NNTP newsgroup(s).