Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
On Wed, 19 Oct 2016 15:06:34 -0300 Arnaldo Carvalho de Melo wrote: > Em Wed, Oct 19, 2016 at 03:05:48PM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Wed, Oct 19, 2016 at 02:48:45PM -0300, Arnaldo Carvalho de Melo > > escreveu: > > > Em Tue, Oct 18, 2016 at 11:29:53AM -0400, Steven Rostedt escreveu: > > > > On Tue, 18 Oct 2016 11:01:09 +0900 > > > > Namhyung Kim wrote: > > > > > > > > > Hi Honggyu, > > > > > > > > > > You need to CC relevant maintainers when you send patches to LKML. > > > > > For the libtraceevent, they are Arnaldo and Steven. You can use > > > > > scripts/get_maintainer.pl for this job later. In addition running > > > > > scripts/checkpatch.pl before sending patches is a good habit. > > > > > > > > > > Arnaldo and Steve, > > > > > > > > > > This is from uftrace building libtraceevent with the optimization flag > > > > > and we want to fix the upstream as well. > > > > > > > > > > > > > Acked-by: Steven Rostedt > > > > > > So right after applying this patch I get these new warnings, > > > investigating... > > > > Some are the compiler not grokking logic where the compiler gets > > confused with logic that tests one variable to use another and thinks it > > is using garbage (uninitialized stuff), I tried to follow the logic and > > I think it got slightly more confused than me, as I _think_ its not a > > problem, but the one on the case entry for > > > > OLD_RINGBUF_TYPE_TIME_EXTEND > > > > in old_update_pointers() looks like a bug, unless some macro magic is > > taking place that updates that 'lenght' variable. > > > > Rostedt, that -O2 unleashed some warnings, please check, I'll defer > > applying those patches till it doesn't show these warnings, i.e. till > > other patches fixing these issues or simply silencing the compiler with > > a harmless init gets submitted, > > Ah, the patch I had so far shutting off most of this is: > > > diff --git a/tools/lib/traceevent/event-parse.c > b/tools/lib/traceevent/event-parse.c > index 664c90c8e22b..449056e96fe6 100644 > --- a/tools/lib/traceevent/event-parse.c > +++ b/tools/lib/traceevent/event-parse.c > @@ -3490,7 +3490,7 @@ struct event_format * > pevent_find_event_by_name(struct pevent *pevent, > const char *sys, const char *name) > { > - struct event_format *event; > + struct event_format *event = NULL; > int i; Grumble. This is just bad gcc. I mean we have: for (i = 0; i < pevent->nr_events; i++) { event = pevent->events[i]; if (i == pevent->nr_events) event = NULL; How the hell can event be uninitialized after that? > > if (pevent->last_event && > @@ -4843,7 +4843,7 @@ static void pretty_print(struct trace_seq *s, void > *data, int size, struct event > char format[32]; > int show_func; > int len_as_arg; > - int len_arg; > + int len_arg = 0; Again, silly gcc. > int len; > int ls; > > @@ -5102,8 +5102,8 @@ void pevent_data_lat_fmt(struct pevent *pevent, > static int migrate_disable_exists; > unsigned int lat_flags; > unsigned int pc; > - int lock_depth; > - int migrate_disable; > + int lock_depth = 0; > + int migrate_disable = 0; > int hardirq; > int softirq; > void *data = record->data; silly gcc. Fine, add these. -- Steve
Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
On Wed, 19 Oct 2016 15:05:48 -0300 Arnaldo Carvalho de Melo wrote: > Some are the compiler not grokking logic where the compiler gets > confused with logic that tests one variable to use another and thinks it > is using garbage (uninitialized stuff), I tried to follow the logic and > I think it got slightly more confused than me, as I _think_ its not a > problem, but the one on the case entry for > > OLD_RINGBUF_TYPE_TIME_EXTEND > > in old_update_pointers() looks like a bug, unless some macro magic is > taking place that updates that 'lenght' variable. > > Rostedt, that -O2 unleashed some warnings, please check, I'll defer > applying those patches till it doesn't show these warnings, i.e. till > other patches fixing these issues or simply silencing the compiler with > a harmless init gets submitted, > > Thanks, Note, that code is for the first version of the ftrace ring buffer that got changed around 2.6.32 I believe. And since trace-cmd was the only tool that directly looked at the code, I was able to "break" abi and update trace-cmd to have a new version. So that code isn't even used anywhere on newer kernels. That said, could you add in the case statement for OLD_RINGBUF_TYPE_TIME_EXTEND: length = 0; I think that should be fine. -- Steve
Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
On Wed, 19 Oct 2016 14:48:45 -0300 Arnaldo Carvalho de Melo wrote: > Em Tue, Oct 18, 2016 at 11:29:53AM -0400, Steven Rostedt escreveu: > > On Tue, 18 Oct 2016 11:01:09 +0900 > > Namhyung Kim wrote: > > > > > Hi Honggyu, > > > > > > You need to CC relevant maintainers when you send patches to LKML. > > > For the libtraceevent, they are Arnaldo and Steven. You can use > > > scripts/get_maintainer.pl for this job later. In addition running > > > scripts/checkpatch.pl before sending patches is a good habit. > > > > > > Arnaldo and Steve, > > > > > > This is from uftrace building libtraceevent with the optimization flag > > > and we want to fix the upstream as well. > > > > > > > Acked-by: Steven Rostedt > > So right after applying this patch I get these new warnings, investigating... > > [acme@jouet linux]$ gcc -v > Using built-in specs. > COLLECT_GCC=gcc > COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/6.2.1/lto-wrapper > Target: x86_64-redhat-linux > Configured with: ../configure --enable-bootstrap > --enable-languages=c,c++,objc,obj-c++,fortran,ada,go,lto --prefix=/usr > --mandir=/usr/share/man --infodir=/usr/share/info > --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared > --enable-threads=posix --enable-checking=release --enable-multilib > --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions > --enable-gnu-unique-object --enable-linker-build-id > --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array > --disable-libgcj --with-isl --enable-libmpx --enable-gnu-indirect-function > --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux > Thread model: posix > gcc version 6.2.1 20160916 (Red Hat 6.2.1-2) (GCC) > [acme@jouet linux]$ > > LD /tmp/build/perf/plugin_mac80211-in.o > kbuffer-parse.c: In function ‘__old_next_event’: > kbuffer-parse.c:339:27: warning: ‘length’ may be used uninitialized in this > function [-Wmaybe-uninitialized] > kbuf->next = kbuf->index + length; >^~~~ > kbuffer-parse.c:297:15: note: ‘length’ was declared here > unsigned int length; >^~ Actually, that may be a bug. > CC /tmp/build/perf/plugin_sched_switch.o > CC /tmp/build/perf/run-command.o > event-parse.c: In function ‘pevent_find_event_by_name’: > event-parse.c:3513:21: warning: ‘event’ may be used uninitialized in this > function [-Wmaybe-uninitialized] > pevent->last_event = event; > ~~~^~~ > CC /tmp/build/perf/sigchain.o > LD /tmp/build/perf/plugin_sched_switch-in.o > CC /tmp/build/perf/plugin_function.o > event-parse.c: In function ‘pevent_data_lat_fmt’: > event-parse.c:5156:4: warning: ‘migrate_disable’ may be used uninitialized in > this function [-Wmaybe-uninitialized] > trace_seq_printf(s, "%d", migrate_disable); > ^~ > event-parse.c:5163:4: warning: ‘lock_depth’ may be used uninitialized in this > function [-Wmaybe-uninitialized] > trace_seq_printf(s, "%d", lock_depth); These two don't look like bugs because they need to have both migrate_disable_exists and lock_depth_exists to be set, and when they are those variables are. Funny it only complains about the one in the trace_seq_printf() and not the compare before them. ie. if (migrate_disable < 0) which is checked before calling the trace_seq_printf(). > ^ > plugin_function.c: In function ‘function_handler’: > plugin_function.c:133:6: warning: ‘index’ may be used uninitialized in this > function [-Wmaybe-uninitialized] > int index; > ^ Ah, this is a bug too. > CC /tmp/build/perf/subcmd-config.o > GEN perf-archive > LD /tmp/build/perf/plugin_function-in.o > GEN perf-with-kcore > CC /tmp/build/perf/plugin_xen.o > event-parse.c: In function ‘pevent_event_info’: > event-parse.c:5003:7: warning: ‘len_arg’ may be used uninitialized in this > function [-Wmaybe-uninitialized] >trace_seq_printf(s, format, len_arg, (char)val); >^~~ > event-parse.c:4846:6: note: ‘len_arg’ was declared here > int len_arg; Again, the "len_as_arg" needs to be set for this to be an issue. We could just initialize len_arg to zero to shut gcc up. -- Steve > ^~~ > MKDIR/tmp/build/perf/util/
Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
Em Wed, Oct 19, 2016 at 02:48:45PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Tue, Oct 18, 2016 at 11:29:53AM -0400, Steven Rostedt escreveu: > > On Tue, 18 Oct 2016 11:01:09 +0900 > > Namhyung Kim wrote: > > > > > Hi Honggyu, > > > > > > You need to CC relevant maintainers when you send patches to LKML. > > > For the libtraceevent, they are Arnaldo and Steven. You can use > > > scripts/get_maintainer.pl for this job later. In addition running > > > scripts/checkpatch.pl before sending patches is a good habit. > > > > > > Arnaldo and Steve, > > > > > > This is from uftrace building libtraceevent with the optimization flag > > > and we want to fix the upstream as well. > > > > > > > Acked-by: Steven Rostedt > > So right after applying this patch I get these new warnings, investigating... Some are the compiler not grokking logic where the compiler gets confused with logic that tests one variable to use another and thinks it is using garbage (uninitialized stuff), I tried to follow the logic and I think it got slightly more confused than me, as I _think_ its not a problem, but the one on the case entry for OLD_RINGBUF_TYPE_TIME_EXTEND in old_update_pointers() looks like a bug, unless some macro magic is taking place that updates that 'lenght' variable. Rostedt, that -O2 unleashed some warnings, please check, I'll defer applying those patches till it doesn't show these warnings, i.e. till other patches fixing these issues or simply silencing the compiler with a harmless init gets submitted, Thanks, - Arnaldo > [acme@jouet linux]$ gcc -v > Using built-in specs. > COLLECT_GCC=gcc > COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/6.2.1/lto-wrapper > Target: x86_64-redhat-linux > Configured with: ../configure --enable-bootstrap > --enable-languages=c,c++,objc,obj-c++,fortran,ada,go,lto --prefix=/usr > --mandir=/usr/share/man --infodir=/usr/share/info > --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared > --enable-threads=posix --enable-checking=release --enable-multilib > --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions > --enable-gnu-unique-object --enable-linker-build-id > --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array > --disable-libgcj --with-isl --enable-libmpx --enable-gnu-indirect-function > --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux > Thread model: posix > gcc version 6.2.1 20160916 (Red Hat 6.2.1-2) (GCC) > [acme@jouet linux]$ > > LD /tmp/build/perf/plugin_mac80211-in.o > kbuffer-parse.c: In function ‘__old_next_event’: > kbuffer-parse.c:339:27: warning: ‘length’ may be used uninitialized in this > function [-Wmaybe-uninitialized] > kbuf->next = kbuf->index + length; >^~~~ > kbuffer-parse.c:297:15: note: ‘length’ was declared here > unsigned int length; >^~ > CC /tmp/build/perf/plugin_sched_switch.o > CC /tmp/build/perf/run-command.o > event-parse.c: In function ‘pevent_find_event_by_name’: > event-parse.c:3513:21: warning: ‘event’ may be used uninitialized in this > function [-Wmaybe-uninitialized] > pevent->last_event = event; > ~~~^~~ > CC /tmp/build/perf/sigchain.o > LD /tmp/build/perf/plugin_sched_switch-in.o > CC /tmp/build/perf/plugin_function.o > event-parse.c: In function ‘pevent_data_lat_fmt’: > event-parse.c:5156:4: warning: ‘migrate_disable’ may be used uninitialized in > this function [-Wmaybe-uninitialized] > trace_seq_printf(s, "%d", migrate_disable); > ^~ > event-parse.c:5163:4: warning: ‘lock_depth’ may be used uninitialized in this > function [-Wmaybe-uninitialized] > trace_seq_printf(s, "%d", lock_depth); > ^ > plugin_function.c: In function ‘function_handler’: > plugin_function.c:133:6: warning: ‘index’ may be used uninitialized in this > function [-Wmaybe-uninitialized] > int index; > ^ > CC /tmp/build/perf/subcmd-config.o > GEN perf-archive > LD /tmp/build/perf/plugin_function-in.o > GEN perf-with-kcore > CC /tmp/build/perf/plugin_xen.o > event-parse.c: In function ‘pevent_event_info’: > event-parse.c:5003:7: warning: ‘len_arg’ may be used uninitialized in this > function [-Wmaybe-uninitialized] >trace_seq_printf(s, format, len_arg, (char)val); >^~~ > event-parse.c:4846:6: note: ‘len_arg’ was declared here > int len_arg; > ^~~ > MKDIR/tmp/build/perf/util/ >
Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
Em Wed, Oct 19, 2016 at 03:05:48PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Wed, Oct 19, 2016 at 02:48:45PM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Tue, Oct 18, 2016 at 11:29:53AM -0400, Steven Rostedt escreveu: > > > On Tue, 18 Oct 2016 11:01:09 +0900 > > > Namhyung Kim wrote: > > > > > > > Hi Honggyu, > > > > > > > > You need to CC relevant maintainers when you send patches to LKML. > > > > For the libtraceevent, they are Arnaldo and Steven. You can use > > > > scripts/get_maintainer.pl for this job later. In addition running > > > > scripts/checkpatch.pl before sending patches is a good habit. > > > > > > > > Arnaldo and Steve, > > > > > > > > This is from uftrace building libtraceevent with the optimization flag > > > > and we want to fix the upstream as well. > > > > > > > > > > Acked-by: Steven Rostedt > > > > So right after applying this patch I get these new warnings, > > investigating... > > Some are the compiler not grokking logic where the compiler gets > confused with logic that tests one variable to use another and thinks it > is using garbage (uninitialized stuff), I tried to follow the logic and > I think it got slightly more confused than me, as I _think_ its not a > problem, but the one on the case entry for > > OLD_RINGBUF_TYPE_TIME_EXTEND > > in old_update_pointers() looks like a bug, unless some macro magic is > taking place that updates that 'lenght' variable. > > Rostedt, that -O2 unleashed some warnings, please check, I'll defer > applying those patches till it doesn't show these warnings, i.e. till > other patches fixing these issues or simply silencing the compiler with > a harmless init gets submitted, Ah, the patch I had so far shutting off most of this is: diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c index 664c90c8e22b..449056e96fe6 100644 --- a/tools/lib/traceevent/event-parse.c +++ b/tools/lib/traceevent/event-parse.c @@ -3490,7 +3490,7 @@ struct event_format * pevent_find_event_by_name(struct pevent *pevent, const char *sys, const char *name) { - struct event_format *event; + struct event_format *event = NULL; int i; if (pevent->last_event && @@ -4843,7 +4843,7 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct event char format[32]; int show_func; int len_as_arg; - int len_arg; + int len_arg = 0; int len; int ls; @@ -5102,8 +5102,8 @@ void pevent_data_lat_fmt(struct pevent *pevent, static int migrate_disable_exists; unsigned int lat_flags; unsigned int pc; - int lock_depth; - int migrate_disable; + int lock_depth = 0; + int migrate_disable = 0; int hardirq; int softirq; void *data = record->data;
Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
Em Tue, Oct 18, 2016 at 11:29:53AM -0400, Steven Rostedt escreveu: > On Tue, 18 Oct 2016 11:01:09 +0900 > Namhyung Kim wrote: > > > Hi Honggyu, > > > > You need to CC relevant maintainers when you send patches to LKML. > > For the libtraceevent, they are Arnaldo and Steven. You can use > > scripts/get_maintainer.pl for this job later. In addition running > > scripts/checkpatch.pl before sending patches is a good habit. > > > > Arnaldo and Steve, > > > > This is from uftrace building libtraceevent with the optimization flag > > and we want to fix the upstream as well. > > > > Acked-by: Steven Rostedt So right after applying this patch I get these new warnings, investigating... [acme@jouet linux]$ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/6.2.1/lto-wrapper Target: x86_64-redhat-linux Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,objc,obj-c++,fortran,ada,go,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --disable-libgcj --with-isl --enable-libmpx --enable-gnu-indirect-function --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux Thread model: posix gcc version 6.2.1 20160916 (Red Hat 6.2.1-2) (GCC) [acme@jouet linux]$ LD /tmp/build/perf/plugin_mac80211-in.o kbuffer-parse.c: In function ‘__old_next_event’: kbuffer-parse.c:339:27: warning: ‘length’ may be used uninitialized in this function [-Wmaybe-uninitialized] kbuf->next = kbuf->index + length; ^~~~ kbuffer-parse.c:297:15: note: ‘length’ was declared here unsigned int length; ^~ CC /tmp/build/perf/plugin_sched_switch.o CC /tmp/build/perf/run-command.o event-parse.c: In function ‘pevent_find_event_by_name’: event-parse.c:3513:21: warning: ‘event’ may be used uninitialized in this function [-Wmaybe-uninitialized] pevent->last_event = event; ~~~^~~ CC /tmp/build/perf/sigchain.o LD /tmp/build/perf/plugin_sched_switch-in.o CC /tmp/build/perf/plugin_function.o event-parse.c: In function ‘pevent_data_lat_fmt’: event-parse.c:5156:4: warning: ‘migrate_disable’ may be used uninitialized in this function [-Wmaybe-uninitialized] trace_seq_printf(s, "%d", migrate_disable); ^~ event-parse.c:5163:4: warning: ‘lock_depth’ may be used uninitialized in this function [-Wmaybe-uninitialized] trace_seq_printf(s, "%d", lock_depth); ^ plugin_function.c: In function ‘function_handler’: plugin_function.c:133:6: warning: ‘index’ may be used uninitialized in this function [-Wmaybe-uninitialized] int index; ^ CC /tmp/build/perf/subcmd-config.o GEN perf-archive LD /tmp/build/perf/plugin_function-in.o GEN perf-with-kcore CC /tmp/build/perf/plugin_xen.o event-parse.c: In function ‘pevent_event_info’: event-parse.c:5003:7: warning: ‘len_arg’ may be used uninitialized in this function [-Wmaybe-uninitialized] trace_seq_printf(s, format, len_arg, (char)val); ^~~ event-parse.c:4846:6: note: ‘len_arg’ was declared here int len_arg; ^~~ MKDIR/tmp/build/perf/util/
Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
On Tue, 18 Oct 2016 11:01:09 +0900 Namhyung Kim wrote: > Hi Honggyu, > > You need to CC relevant maintainers when you send patches to LKML. > For the libtraceevent, they are Arnaldo and Steven. You can use > scripts/get_maintainer.pl for this job later. In addition running > scripts/checkpatch.pl before sending patches is a good habit. > > Arnaldo and Steve, > > This is from uftrace building libtraceevent with the optimization flag > and we want to fix the upstream as well. > Acked-by: Steven Rostedt -- Steve
Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
Hi Honggyu, You need to CC relevant maintainers when you send patches to LKML. For the libtraceevent, they are Arnaldo and Steven. You can use scripts/get_maintainer.pl for this job later. In addition running scripts/checkpatch.pl before sending patches is a good habit. Arnaldo and Steve, This is from uftrace building libtraceevent with the optimization flag and we want to fix the upstream as well. Thanks, Namhyung On Mon, Oct 17, 2016 at 11:17:10PM +0900, Honggyu Kim wrote: > Since current traceevent somehow does not have an optimization flag, > this patch just adds -O2 to optimize its code. > > Signed-off-by: Honggyu Kim > --- > tools/lib/traceevent/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/lib/traceevent/Makefile b/tools/lib/traceevent/Makefile > index 7851df1..56d223b 100644 > --- a/tools/lib/traceevent/Makefile > +++ b/tools/lib/traceevent/Makefile > @@ -124,7 +124,7 @@ else > endif > > # Append required CFLAGS > -override CFLAGS += -fPIC > +override CFLAGS += -O2 -fPIC > override CFLAGS += $(CONFIG_FLAGS) $(INCLUDES) $(PLUGIN_DIR_SQ) > override CFLAGS += $(udis86-flags) -D_GNU_SOURCE > > -- > 2.10.0.rc2.dirty >