* [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
@ 2016-10-17 14:17 Honggyu Kim
2016-10-17 14:17 ` [PATCH 2/3] tools lib traceevent: Check the return value of asprintf Honggyu Kim
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Honggyu Kim @ 2016-10-17 14:17 UTC (permalink / raw)
To: namhyung; +Cc: linux-kernel, Honggyu Kim
Since current traceevent somehow does not have an optimization flag,
this patch just adds -O2 to optimize its code.
Signed-off-by: Honggyu Kim <hong.gyu.kim@lge.com>
---
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] tools lib traceevent: Check the return value of asprintf
2016-10-17 14:17 [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent Honggyu Kim
@ 2016-10-17 14:17 ` Honggyu Kim
2016-10-18 17:59 ` Arnaldo Carvalho de Melo
2016-10-17 14:17 ` [PATCH 3/3] tools lib traceevent: Fix to set uninitialized variables Honggyu Kim
2016-10-18 2:01 ` [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent Namhyung Kim
2 siblings, 1 reply; 14+ messages in thread
From: Honggyu Kim @ 2016-10-17 14:17 UTC (permalink / raw)
To: namhyung; +Cc: linux-kernel, Honggyu Kim
Since asprintf generates a compiler warning when its return value is not
not properly handled, this patch checks that asprintf call is successful
or not.
Signed-off-by: Honggyu Kim <hong.gyu.kim@lge.com>
---
tools/lib/traceevent/parse-filter.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 7c214ce..f0fcdcd 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -2122,7 +2122,8 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
default:
break;
}
- asprintf(&str, val ? "TRUE" : "FALSE");
+ if (asprintf(&str, val ? "TRUE" : "FALSE") < 0)
+ return NULL;
break;
}
}
@@ -2140,7 +2141,8 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
break;
}
- asprintf(&str, "(%s) %s (%s)", left, op, right);
+ if (asprintf(&str, "(%s) %s (%s)", left, op, right) < 0)
+ return NULL;
break;
case FILTER_OP_NOT:
@@ -2156,10 +2158,12 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
right_val = 0;
if (right_val >= 0) {
/* just return the opposite */
- asprintf(&str, right_val ? "FALSE" : "TRUE");
+ if (asprintf(&str, right_val ? "FALSE" : "TRUE") < 0)
+ return NULL;
break;
}
- asprintf(&str, "%s(%s)", op, right);
+ if (asprintf(&str, "%s(%s)", op, right) < 0)
+ return NULL;
break;
default:
@@ -2175,7 +2179,8 @@ static char *val_to_str(struct event_filter *filter, struct filter_arg *arg)
{
char *str = NULL;
- asprintf(&str, "%lld", arg->value.val);
+ if (asprintf(&str, "%lld", arg->value.val) < 0)
+ return NULL;
return str;
}
@@ -2233,7 +2238,8 @@ static char *exp_to_str(struct event_filter *filter, struct filter_arg *arg)
break;
}
- asprintf(&str, "%s %s %s", lstr, op, rstr);
+ if (asprintf(&str, "%s %s %s", lstr, op, rstr) < 0)
+ return NULL;
out:
free(lstr);
free(rstr);
@@ -2277,7 +2283,8 @@ static char *num_to_str(struct event_filter *filter, struct filter_arg *arg)
if (!op)
op = "<=";
- asprintf(&str, "%s %s %s", lstr, op, rstr);
+ if (asprintf(&str, "%s %s %s", lstr, op, rstr) < 0)
+ return NULL;
break;
default:
@@ -2312,8 +2319,9 @@ static char *str_to_str(struct event_filter *filter, struct filter_arg *arg)
if (!op)
op = "!~";
- asprintf(&str, "%s %s \"%s\"",
- arg->str.field->name, op, arg->str.val);
+ if (asprintf(&str, "%s %s \"%s\"",
+ arg->str.field->name, op, arg->str.val) < 0)
+ return NULL;
break;
default:
@@ -2329,7 +2337,8 @@ static char *arg_to_str(struct event_filter *filter, struct filter_arg *arg)
switch (arg->type) {
case FILTER_ARG_BOOLEAN:
- asprintf(&str, arg->boolean.value ? "TRUE" : "FALSE");
+ if (asprintf(&str, arg->boolean.value ? "TRUE" : "FALSE") < 0)
+ return NULL;
return str;
case FILTER_ARG_OP:
--
2.10.0.rc2.dirty
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] tools lib traceevent: Fix to set uninitialized variables
2016-10-17 14:17 [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent Honggyu Kim
2016-10-17 14:17 ` [PATCH 2/3] tools lib traceevent: Check the return value of asprintf Honggyu Kim
@ 2016-10-17 14:17 ` Honggyu Kim
2016-10-18 17:59 ` Arnaldo Carvalho de Melo
2016-10-18 2:01 ` [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent Namhyung Kim
2 siblings, 1 reply; 14+ messages in thread
From: Honggyu Kim @ 2016-10-17 14:17 UTC (permalink / raw)
To: namhyung; +Cc: linux-kernel, Honggyu Kim
This patch fixes uninitialized variables to remove remaining compiler
warnings as follows:
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;
^
event-parse.c: In function ‘pevent_data_lat_fmt’:
event-parse.c:5156:20: warning: ‘migrate_disable’ may be used uninitialized in this function [-Wmaybe-uninitialized]
trace_seq_printf(s, "%d", migrate_disable);
^
event-parse.c:5163:20: warning: ‘lock_depth’ may be used uninitialized in this function [-Wmaybe-uninitialized]
trace_seq_printf(s, "%d", lock_depth);
^
event-parse.c: In function ‘pevent_event_info’:
event-parse.c:5060:18: warning: ‘len_arg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
print_str_arg(&p, data, size, event,
^
event-parse.c:4846:6: note: ‘len_arg’ was declared here
int len_arg;
^
...
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;
^
Signed-off-by: Honggyu Kim <hong.gyu.kim@lge.com>
---
tools/lib/traceevent/event-parse.c | 8 ++++----
tools/lib/traceevent/kbuffer-parse.c | 2 +-
tools/lib/traceevent/plugin_function.c | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 664c90c..2fb9338 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 = -1;
+ int migrate_disable = 0;
int hardirq;
int softirq;
void *data = record->data;
diff --git a/tools/lib/traceevent/kbuffer-parse.c b/tools/lib/traceevent/kbuffer-parse.c
index 65984f1..fc8f20c 100644
--- a/tools/lib/traceevent/kbuffer-parse.c
+++ b/tools/lib/traceevent/kbuffer-parse.c
@@ -294,7 +294,7 @@ static unsigned int old_update_pointers(struct kbuffer *kbuf)
unsigned int type;
unsigned int len;
unsigned int delta;
- unsigned int length;
+ unsigned int length = 0;
void *ptr = kbuf->data + kbuf->curr;
type_len_ts = read_4(kbuf, ptr);
diff --git a/tools/lib/traceevent/plugin_function.c b/tools/lib/traceevent/plugin_function.c
index a00ec19..42dbf73 100644
--- a/tools/lib/traceevent/plugin_function.c
+++ b/tools/lib/traceevent/plugin_function.c
@@ -130,7 +130,7 @@ static int function_handler(struct trace_seq *s, struct pevent_record *record,
unsigned long long pfunction;
const char *func;
const char *parent;
- int index;
+ int index = 0;
if (pevent_get_field_val(s, event, "ip", record, &function, 1))
return trace_seq_putc(s, '!');
--
2.10.0.rc2.dirty
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
2016-10-17 14:17 [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent Honggyu Kim
2016-10-17 14:17 ` [PATCH 2/3] tools lib traceevent: Check the return value of asprintf Honggyu Kim
2016-10-17 14:17 ` [PATCH 3/3] tools lib traceevent: Fix to set uninitialized variables Honggyu Kim
@ 2016-10-18 2:01 ` Namhyung Kim
2016-10-18 15:29 ` Steven Rostedt
2 siblings, 1 reply; 14+ messages in thread
From: Namhyung Kim @ 2016-10-18 2:01 UTC (permalink / raw)
To: Honggyu Kim; +Cc: linux-kernel, Arnaldo Carvalho de Melo, Steven Rostedt
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 <hong.gyu.kim@lge.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
2016-10-18 2:01 ` [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent Namhyung Kim
@ 2016-10-18 15:29 ` Steven Rostedt
2016-10-19 17:48 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 14+ messages in thread
From: Steven Rostedt @ 2016-10-18 15:29 UTC (permalink / raw)
To: Namhyung Kim; +Cc: Honggyu Kim, linux-kernel, Arnaldo Carvalho de Melo
On Tue, 18 Oct 2016 11:01:09 +0900
Namhyung Kim <namhyung@kernel.org> 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 <rostedt@goodmis.org>
-- Steve
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] tools lib traceevent: Check the return value of asprintf
2016-10-17 14:17 ` [PATCH 2/3] tools lib traceevent: Check the return value of asprintf Honggyu Kim
@ 2016-10-18 17:59 ` Arnaldo Carvalho de Melo
2016-10-19 0:25 ` Namhyung Kim
0 siblings, 1 reply; 14+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-10-18 17:59 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Honggyu Kim, namhyung, linux-kernel
Steven, one more, please ack.
- Arnaldo
Em Mon, Oct 17, 2016 at 11:17:11PM +0900, Honggyu Kim escreveu:
> Since asprintf generates a compiler warning when its return value is not
> not properly handled, this patch checks that asprintf call is successful
> or not.
>
> Signed-off-by: Honggyu Kim <hong.gyu.kim@lge.com>
> ---
> tools/lib/traceevent/parse-filter.c | 29 +++++++++++++++++++----------
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
> index 7c214ce..f0fcdcd 100644
> --- a/tools/lib/traceevent/parse-filter.c
> +++ b/tools/lib/traceevent/parse-filter.c
> @@ -2122,7 +2122,8 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
> default:
> break;
> }
> - asprintf(&str, val ? "TRUE" : "FALSE");
> + if (asprintf(&str, val ? "TRUE" : "FALSE") < 0)
> + return NULL;
> break;
> }
> }
> @@ -2140,7 +2141,8 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
> break;
> }
>
> - asprintf(&str, "(%s) %s (%s)", left, op, right);
> + if (asprintf(&str, "(%s) %s (%s)", left, op, right) < 0)
> + return NULL;
> break;
>
> case FILTER_OP_NOT:
> @@ -2156,10 +2158,12 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
> right_val = 0;
> if (right_val >= 0) {
> /* just return the opposite */
> - asprintf(&str, right_val ? "FALSE" : "TRUE");
> + if (asprintf(&str, right_val ? "FALSE" : "TRUE") < 0)
> + return NULL;
> break;
> }
> - asprintf(&str, "%s(%s)", op, right);
> + if (asprintf(&str, "%s(%s)", op, right) < 0)
> + return NULL;
> break;
>
> default:
> @@ -2175,7 +2179,8 @@ static char *val_to_str(struct event_filter *filter, struct filter_arg *arg)
> {
> char *str = NULL;
>
> - asprintf(&str, "%lld", arg->value.val);
> + if (asprintf(&str, "%lld", arg->value.val) < 0)
> + return NULL;
>
> return str;
> }
> @@ -2233,7 +2238,8 @@ static char *exp_to_str(struct event_filter *filter, struct filter_arg *arg)
> break;
> }
>
> - asprintf(&str, "%s %s %s", lstr, op, rstr);
> + if (asprintf(&str, "%s %s %s", lstr, op, rstr) < 0)
> + return NULL;
> out:
> free(lstr);
> free(rstr);
> @@ -2277,7 +2283,8 @@ static char *num_to_str(struct event_filter *filter, struct filter_arg *arg)
> if (!op)
> op = "<=";
>
> - asprintf(&str, "%s %s %s", lstr, op, rstr);
> + if (asprintf(&str, "%s %s %s", lstr, op, rstr) < 0)
> + return NULL;
> break;
>
> default:
> @@ -2312,8 +2319,9 @@ static char *str_to_str(struct event_filter *filter, struct filter_arg *arg)
> if (!op)
> op = "!~";
>
> - asprintf(&str, "%s %s \"%s\"",
> - arg->str.field->name, op, arg->str.val);
> + if (asprintf(&str, "%s %s \"%s\"",
> + arg->str.field->name, op, arg->str.val) < 0)
> + return NULL;
> break;
>
> default:
> @@ -2329,7 +2337,8 @@ static char *arg_to_str(struct event_filter *filter, struct filter_arg *arg)
>
> switch (arg->type) {
> case FILTER_ARG_BOOLEAN:
> - asprintf(&str, arg->boolean.value ? "TRUE" : "FALSE");
> + if (asprintf(&str, arg->boolean.value ? "TRUE" : "FALSE") < 0)
> + return NULL;
> return str;
>
> case FILTER_ARG_OP:
> --
> 2.10.0.rc2.dirty
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] tools lib traceevent: Fix to set uninitialized variables
2016-10-17 14:17 ` [PATCH 3/3] tools lib traceevent: Fix to set uninitialized variables Honggyu Kim
@ 2016-10-18 17:59 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 14+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-10-18 17:59 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Honggyu Kim, namhyung, linux-kernel
One more.
Em Mon, Oct 17, 2016 at 11:17:12PM +0900, Honggyu Kim escreveu:
> This patch fixes uninitialized variables to remove remaining compiler
> warnings as follows:
>
> 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;
> ^
> event-parse.c: In function ‘pevent_data_lat_fmt’:
> event-parse.c:5156:20: warning: ‘migrate_disable’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> trace_seq_printf(s, "%d", migrate_disable);
> ^
> event-parse.c:5163:20: warning: ‘lock_depth’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> trace_seq_printf(s, "%d", lock_depth);
> ^
> event-parse.c: In function ‘pevent_event_info’:
> event-parse.c:5060:18: warning: ‘len_arg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> print_str_arg(&p, data, size, event,
> ^
> event-parse.c:4846:6: note: ‘len_arg’ was declared here
> int len_arg;
> ^
> ...
> 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;
> ^
>
> Signed-off-by: Honggyu Kim <hong.gyu.kim@lge.com>
> ---
> tools/lib/traceevent/event-parse.c | 8 ++++----
> tools/lib/traceevent/kbuffer-parse.c | 2 +-
> tools/lib/traceevent/plugin_function.c | 2 +-
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index 664c90c..2fb9338 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 = -1;
> + int migrate_disable = 0;
> int hardirq;
> int softirq;
> void *data = record->data;
> diff --git a/tools/lib/traceevent/kbuffer-parse.c b/tools/lib/traceevent/kbuffer-parse.c
> index 65984f1..fc8f20c 100644
> --- a/tools/lib/traceevent/kbuffer-parse.c
> +++ b/tools/lib/traceevent/kbuffer-parse.c
> @@ -294,7 +294,7 @@ static unsigned int old_update_pointers(struct kbuffer *kbuf)
> unsigned int type;
> unsigned int len;
> unsigned int delta;
> - unsigned int length;
> + unsigned int length = 0;
> void *ptr = kbuf->data + kbuf->curr;
>
> type_len_ts = read_4(kbuf, ptr);
> diff --git a/tools/lib/traceevent/plugin_function.c b/tools/lib/traceevent/plugin_function.c
> index a00ec19..42dbf73 100644
> --- a/tools/lib/traceevent/plugin_function.c
> +++ b/tools/lib/traceevent/plugin_function.c
> @@ -130,7 +130,7 @@ static int function_handler(struct trace_seq *s, struct pevent_record *record,
> unsigned long long pfunction;
> const char *func;
> const char *parent;
> - int index;
> + int index = 0;
>
> if (pevent_get_field_val(s, event, "ip", record, &function, 1))
> return trace_seq_putc(s, '!');
> --
> 2.10.0.rc2.dirty
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] tools lib traceevent: Check the return value of asprintf
2016-10-18 17:59 ` Arnaldo Carvalho de Melo
@ 2016-10-19 0:25 ` Namhyung Kim
0 siblings, 0 replies; 14+ messages in thread
From: Namhyung Kim @ 2016-10-19 0:25 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: Steven Rostedt, Honggyu Kim, linux-kernel
Hi Arnaldo,
You can add my Acked-by for all 3 patches.
Thanks,
Namhyung
On Tue, Oct 18, 2016 at 02:59:17PM -0300, Arnaldo Carvalho de Melo wrote:
> Steven, one more, please ack.
>
> - Arnaldo
>
> Em Mon, Oct 17, 2016 at 11:17:11PM +0900, Honggyu Kim escreveu:
> > Since asprintf generates a compiler warning when its return value is not
> > not properly handled, this patch checks that asprintf call is successful
> > or not.
> >
> > Signed-off-by: Honggyu Kim <hong.gyu.kim@lge.com>
> > ---
> > tools/lib/traceevent/parse-filter.c | 29 +++++++++++++++++++----------
> > 1 file changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
> > index 7c214ce..f0fcdcd 100644
> > --- a/tools/lib/traceevent/parse-filter.c
> > +++ b/tools/lib/traceevent/parse-filter.c
> > @@ -2122,7 +2122,8 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
> > default:
> > break;
> > }
> > - asprintf(&str, val ? "TRUE" : "FALSE");
> > + if (asprintf(&str, val ? "TRUE" : "FALSE") < 0)
> > + return NULL;
> > break;
> > }
> > }
> > @@ -2140,7 +2141,8 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
> > break;
> > }
> >
> > - asprintf(&str, "(%s) %s (%s)", left, op, right);
> > + if (asprintf(&str, "(%s) %s (%s)", left, op, right) < 0)
> > + return NULL;
> > break;
> >
> > case FILTER_OP_NOT:
> > @@ -2156,10 +2158,12 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
> > right_val = 0;
> > if (right_val >= 0) {
> > /* just return the opposite */
> > - asprintf(&str, right_val ? "FALSE" : "TRUE");
> > + if (asprintf(&str, right_val ? "FALSE" : "TRUE") < 0)
> > + return NULL;
> > break;
> > }
> > - asprintf(&str, "%s(%s)", op, right);
> > + if (asprintf(&str, "%s(%s)", op, right) < 0)
> > + return NULL;
> > break;
> >
> > default:
> > @@ -2175,7 +2179,8 @@ static char *val_to_str(struct event_filter *filter, struct filter_arg *arg)
> > {
> > char *str = NULL;
> >
> > - asprintf(&str, "%lld", arg->value.val);
> > + if (asprintf(&str, "%lld", arg->value.val) < 0)
> > + return NULL;
> >
> > return str;
> > }
> > @@ -2233,7 +2238,8 @@ static char *exp_to_str(struct event_filter *filter, struct filter_arg *arg)
> > break;
> > }
> >
> > - asprintf(&str, "%s %s %s", lstr, op, rstr);
> > + if (asprintf(&str, "%s %s %s", lstr, op, rstr) < 0)
> > + return NULL;
> > out:
> > free(lstr);
> > free(rstr);
> > @@ -2277,7 +2283,8 @@ static char *num_to_str(struct event_filter *filter, struct filter_arg *arg)
> > if (!op)
> > op = "<=";
> >
> > - asprintf(&str, "%s %s %s", lstr, op, rstr);
> > + if (asprintf(&str, "%s %s %s", lstr, op, rstr) < 0)
> > + return NULL;
> > break;
> >
> > default:
> > @@ -2312,8 +2319,9 @@ static char *str_to_str(struct event_filter *filter, struct filter_arg *arg)
> > if (!op)
> > op = "!~";
> >
> > - asprintf(&str, "%s %s \"%s\"",
> > - arg->str.field->name, op, arg->str.val);
> > + if (asprintf(&str, "%s %s \"%s\"",
> > + arg->str.field->name, op, arg->str.val) < 0)
> > + return NULL;
> > break;
> >
> > default:
> > @@ -2329,7 +2337,8 @@ static char *arg_to_str(struct event_filter *filter, struct filter_arg *arg)
> >
> > switch (arg->type) {
> > case FILTER_ARG_BOOLEAN:
> > - asprintf(&str, arg->boolean.value ? "TRUE" : "FALSE");
> > + if (asprintf(&str, arg->boolean.value ? "TRUE" : "FALSE") < 0)
> > + return NULL;
> > return str;
> >
> > case FILTER_ARG_OP:
> > --
> > 2.10.0.rc2.dirty
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
2016-10-18 15:29 ` Steven Rostedt
@ 2016-10-19 17:48 ` Arnaldo Carvalho de Melo
2016-10-19 18:05 ` Arnaldo Carvalho de Melo
2016-10-19 18:21 ` Steven Rostedt
0 siblings, 2 replies; 14+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-10-19 17:48 UTC (permalink / raw)
To: Steven Rostedt
Cc: Namhyung Kim, Honggyu Kim, linux-kernel, Arnaldo Carvalho de Melo
Em Tue, Oct 18, 2016 at 11:29:53AM -0400, Steven Rostedt escreveu:
> On Tue, 18 Oct 2016 11:01:09 +0900
> Namhyung Kim <namhyung@kernel.org> 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 <rostedt@goodmis.org>
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/
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
2016-10-19 17:48 ` Arnaldo Carvalho de Melo
@ 2016-10-19 18:05 ` Arnaldo Carvalho de Melo
2016-10-19 18:06 ` Arnaldo Carvalho de Melo
2016-10-19 19:21 ` Steven Rostedt
2016-10-19 18:21 ` Steven Rostedt
1 sibling, 2 replies; 14+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-10-19 18:05 UTC (permalink / raw)
To: Steven Rostedt
Cc: Namhyung Kim, Honggyu Kim, linux-kernel, Arnaldo Carvalho de Melo
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 <namhyung@kernel.org> 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 <rostedt@goodmis.org>
>
> 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/
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
2016-10-19 18:05 ` Arnaldo Carvalho de Melo
@ 2016-10-19 18:06 ` Arnaldo Carvalho de Melo
2016-10-19 19:26 ` Steven Rostedt
2016-10-19 19:21 ` Steven Rostedt
1 sibling, 1 reply; 14+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-10-19 18:06 UTC (permalink / raw)
To: Steven Rostedt
Cc: Namhyung Kim, Honggyu Kim, linux-kernel, Arnaldo Carvalho de Melo
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 <namhyung@kernel.org> 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 <rostedt@goodmis.org>
> >
> > 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;
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
2016-10-19 17:48 ` Arnaldo Carvalho de Melo
2016-10-19 18:05 ` Arnaldo Carvalho de Melo
@ 2016-10-19 18:21 ` Steven Rostedt
1 sibling, 0 replies; 14+ messages in thread
From: Steven Rostedt @ 2016-10-19 18:21 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Honggyu Kim, linux-kernel, Arnaldo Carvalho de Melo
On Wed, 19 Oct 2016 14:48:45 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> 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 <namhyung@kernel.org> 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 <rostedt@goodmis.org>
>
> 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/
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
2016-10-19 18:05 ` Arnaldo Carvalho de Melo
2016-10-19 18:06 ` Arnaldo Carvalho de Melo
@ 2016-10-19 19:21 ` Steven Rostedt
1 sibling, 0 replies; 14+ messages in thread
From: Steven Rostedt @ 2016-10-19 19:21 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Honggyu Kim, linux-kernel, Arnaldo Carvalho de Melo
On Wed, 19 Oct 2016 15:05:48 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent
2016-10-19 18:06 ` Arnaldo Carvalho de Melo
@ 2016-10-19 19:26 ` Steven Rostedt
0 siblings, 0 replies; 14+ messages in thread
From: Steven Rostedt @ 2016-10-19 19:26 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Honggyu Kim, linux-kernel, Arnaldo Carvalho de Melo
On Wed, 19 Oct 2016 15:06:34 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> 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 <namhyung@kernel.org> 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 <rostedt@goodmis.org>
> > >
> > > 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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-10-19 19:26 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-17 14:17 [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent Honggyu Kim
2016-10-17 14:17 ` [PATCH 2/3] tools lib traceevent: Check the return value of asprintf Honggyu Kim
2016-10-18 17:59 ` Arnaldo Carvalho de Melo
2016-10-19 0:25 ` Namhyung Kim
2016-10-17 14:17 ` [PATCH 3/3] tools lib traceevent: Fix to set uninitialized variables Honggyu Kim
2016-10-18 17:59 ` Arnaldo Carvalho de Melo
2016-10-18 2:01 ` [PATCH 1/3] tools lib traceevent: Add -O2 option to traceevent Namhyung Kim
2016-10-18 15:29 ` Steven Rostedt
2016-10-19 17:48 ` Arnaldo Carvalho de Melo
2016-10-19 18:05 ` Arnaldo Carvalho de Melo
2016-10-19 18:06 ` Arnaldo Carvalho de Melo
2016-10-19 19:26 ` Steven Rostedt
2016-10-19 19:21 ` Steven Rostedt
2016-10-19 18:21 ` Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox