* Frederic Weisbecker <[email protected]> wrote:
> Hi,
>
> This is essentially a rebase against latest tip:perf/core updates.
> Mostly due to conflicts against the perf Makefile updates.
>
> I think we all agree that this library needs improvements but these
> should rather be done incrementally. The current perf trace event parsing
> is anyway much backward compared to that library.
>
> Default target is a static library in tools/libtraceevent/libtraceevent.a
>
> This can be pulled from:
>
> git://github.com/fweisbec/tracing.git
> perf/parse-events-4
It gives me:
/home/mingo/tip/tools/lib/traceevent/parse-filter.c: In function ‘create_arg_item’:
/home/mingo/tip/tools/lib/traceevent/parse-filter.c:343:9: warning: comparison between ‘enum filter_arg_type’ and ‘enum event_type’ [-Wenum-compare]
/home/mingo/tip/tools/lib/traceevent/parse-filter.c:339:2: warning: case value ‘8’ not in enumerated type ‘enum filter_arg_type’ [-Wswitch]
Had this been in tools/perf/ it would use -Werror already and
you'd have fixed it, not requiring me to unpull these bits :-/
So can we please make a libevent.so, built sanely within
tools/perf/lib/ or such and distributed together with perf so
that the two can never get out of sync?
Thanks,
Ingo
Hi,
On Mon, 7 May 2012 10:14:24 +0200, Ingo Molnar wrote:
> * Frederic Weisbecker <[email protected]> wrote:
>
>> Hi,
>>
>> This is essentially a rebase against latest tip:perf/core updates.
>> Mostly due to conflicts against the perf Makefile updates.
>>
>> I think we all agree that this library needs improvements but these
>> should rather be done incrementally. The current perf trace event parsing
>> is anyway much backward compared to that library.
>>
>> Default target is a static library in tools/libtraceevent/libtraceevent.a
>>
>> This can be pulled from:
>>
>> git://github.com/fweisbec/tracing.git
>> perf/parse-events-4
>
> It gives me:
>
> /home/mingo/tip/tools/lib/traceevent/parse-filter.c: In function ‘create_arg_item’:
> /home/mingo/tip/tools/lib/traceevent/parse-filter.c:343:9: warning: comparison between ‘enum filter_arg_type’ and ‘enum event_type’ [-Wenum-compare]
> /home/mingo/tip/tools/lib/traceevent/parse-filter.c:339:2: warning: case value ‘8’ not in enumerated type ‘enum filter_arg_type’ [-Wswitch]
>
> Had this been in tools/perf/ it would use -Werror already and
> you'd have fixed it, not requiring me to unpull these bits :-/
>
I already submitted a patch for it (along with other fixes) to Steven.
lkml.indiana.edu/hypermail/linux/kernel/1204.2/03713.html
Thanks,
Namhyung
> So can we please make a libevent.so, built sanely within
> tools/perf/lib/ or such and distributed together with perf so
> that the two can never get out of sync?
>
> Thanks,
>
> Ingo
On Mon, 2012-05-07 at 17:36 +0900, Namhyung Kim wrote:
> I already submitted a patch for it (along with other fixes) to Steven.
>
> lkml.indiana.edu/hypermail/linux/kernel/1204.2/03713.html
>
Thanks for the reminder. I've tested the patches and even added my own
patch on top. But I never pushed it to kernel.org :-p I think
kernel.org was acting funny that day, so I did other things, and then
forgot that I never did the final step.
Pushed now.
Thanks!
-- Steve
On Mon, 2012-05-07 at 10:14 +0200, Ingo Molnar wrote:
> It gives me:
>
> /home/mingo/tip/tools/lib/traceevent/parse-filter.c: In function ‘create_arg_item’:
> /home/mingo/tip/tools/lib/traceevent/parse-filter.c:343:9: warning: comparison between ‘enum filter_arg_type’ and ‘enum event_type’ [-Wenum-compare]
> /home/mingo/tip/tools/lib/traceevent/parse-filter.c:339:2: warning: case value ‘8’ not in enumerated type ‘enum filter_arg_type’ [-Wswitch]
Note, older gcc did not error with these (as I'm still on F13 I have an
older gcc). Otherwise I would have fixed them a long time ago. And
-Werror would have not helped.
-- Steve
On Mon, 2012-05-07 at 10:14 +0200, Ingo Molnar wrote:
> So can we please make a libevent.so, built sanely within
> tools/perf/lib/ or such and distributed together with perf so
> that the two can never get out of sync?
If you do this, you need to find a way to turn off -Wswitch-enum as that
seems to (at least on my gcc) ignore defaults.
Thus we end up with:
diff --git a/Makefile b/Makefile
index 18ad079..217548b 100644
diff --git a/parse-filter.c b/parse-filter.c
index d09fbd2..0ac249c 100644
--- a/parse-filter.c
+++ b/parse-filter.c
@@ -367,6 +367,12 @@ create_arg_item(struct event_format *event, const
char *token,
arg->type = FILTER_ARG_FIELD;
arg->field.field = field;
break;
+ case EVENT_ERROR:
+ case EVENT_NONE:
+ case EVENT_SPACE:
+ case EVENT_NEWLINE:
+ case EVENT_OP:
+ case EVENT_DELIM:
default:
free_arg(arg);
show_error(error_str, "expected a value but found %s",
I found that I added over 20 of these trying to keep -Wswitch-enum
happy, before i decided to give up (there's many more than 20 places
that need updates).
Looking at what was done in the current code, there's lots of :
case PRINT_NULL:
case PRINT_FIELD ... PRINT_SYMBOL:
case PRINT_STRING:
default:
Which looks more of a maintenance nightmare. How does this help? The
FOO ... BAR, will hide the same errors that you are trying to prevent.
If you were suppose to handle something between FOO and BAR, then you
just ignored it too.
It also makes that "default" redundant.
This is one of those warnings that causes more pain than it helps.
If you strongly believe that all these warnings are helpful, than we
should push to add these warnings to the kernel too.
-- Steve
* Steven Rostedt <[email protected]> wrote:
> On Mon, 2012-05-07 at 10:14 +0200, Ingo Molnar wrote:
>
> > So can we please make a libevent.so, built sanely within
> > tools/perf/lib/ or such and distributed together with perf so
> > that the two can never get out of sync?
>
> If you do this, you need to find a way to turn off -Wswitch-enum as that
> seems to (at least on my gcc) ignore defaults.
>
> Thus we end up with:
>
> diff --git a/Makefile b/Makefile
> index 18ad079..217548b 100644
>
> diff --git a/parse-filter.c b/parse-filter.c
> index d09fbd2..0ac249c 100644
> --- a/parse-filter.c
> +++ b/parse-filter.c
> @@ -367,6 +367,12 @@ create_arg_item(struct event_format *event, const
> char *token,
> arg->type = FILTER_ARG_FIELD;
> arg->field.field = field;
> break;
> + case EVENT_ERROR:
> + case EVENT_NONE:
> + case EVENT_SPACE:
> + case EVENT_NEWLINE:
> + case EVENT_OP:
> + case EVENT_DELIM:
> default:
> free_arg(arg);
> show_error(error_str, "expected a value but found %s",
The advantage of this warning is that when a new enum value is
added, every usage site is forced to consider it.
Code not using it indeed needs to be updated.
> Looking at what was done in the current code, there's lots of :
>
> case PRINT_NULL:
> case PRINT_FIELD ... PRINT_SYMBOL:
> case PRINT_STRING:
> default:
>
> Which looks more of a maintenance nightmare. How does this help? The
> FOO ... BAR, will hide the same errors that you are trying to prevent.
Indeed the FOO...BAR pattern should not be used (i.e. the above
is a bug in essence), unless it's clearly some genuine same-type
numeric range that is expressed, where new fields can never get
inbetween.
> If you were suppose to handle something between FOO and BAR, then you
> just ignored it too.
Correct.
> It also makes that "default" redundant.
Yes.
> This is one of those warnings that causes more pain than it
> helps.
I disagree, when I switched to it then it found some real bugs
and new changes were incremental.
> If you strongly believe that all these warnings are helpful,
> than we should push to add these warnings to the kernel too.
For subsystems I maintain we could consider it - but for the
kernel as a whole it would be quite some work.
Thanks,
Ingo
Hi, Ingo
On Mon, 7 May 2012 10:14:24 +0200, Ingo Molnar wrote:
> * Frederic Weisbecker <[email protected]> wrote:
>> This is essentially a rebase against latest tip:perf/core updates.
>> Mostly due to conflicts against the perf Makefile updates.
>>
>> I think we all agree that this library needs improvements but these
>> should rather be done incrementally. The current perf trace event parsing
>> is anyway much backward compared to that library.
>>
>> Default target is a static library in tools/lib/traceevent/libtraceevent.a
>>
>> This can be pulled from:
>>
>> git://github.com/fweisbec/tracing.git
>> perf/parse-events-4
>
> It gives me:
>
> /home/mingo/tip/tools/lib/traceevent/parse-filter.c: In function ‘create_arg_item’:
> /home/mingo/tip/tools/lib/traceevent/parse-filter.c:343:9: warning: comparison between ‘enum filter_arg_type’ and ‘enum event_type’ [-Wenum-compare]
> /home/mingo/tip/tools/lib/traceevent/parse-filter.c:339:2: warning: case value ‘8’ not in enumerated type ‘enum filter_arg_type’ [-Wswitch]
>
> Had this been in tools/perf/ it would use -Werror already and
> you'd have fixed it, not requiring me to unpull these bits :-/
>
> So can we please make a libevent.so, built sanely within
> tools/perf/lib/ or such and distributed together with perf so
> that the two can never get out of sync?
>
So what are you gonna do with this patchset? If you merge this series,
I can back-port latest changes on trace-cmd and do my further work like
'die' removal on it.
Thanks,
Namhyung