Messages in this thread | | | Date | Wed, 6 May 2009 10:49:30 -0400 (EDT) | From | Steven Rostedt <> | Subject | Re: [tip:tracing/core] tracing: trace_output.c, fix false positive compiler warning |
| |
On Wed, 6 May 2009, Ingo Molnar wrote: > > > --- > > > kernel/trace/trace_output.c | 2 +- > > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > > > diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c > > > index 5fc51f0..8bd9a2c 100644 > > > --- a/kernel/trace/trace_output.c > > > +++ b/kernel/trace/trace_output.c > > > @@ -541,7 +541,7 @@ int register_ftrace_event(struct trace_event *event) > > > INIT_LIST_HEAD(&event->list); > > > > > > if (!event->type) { > > > - struct list_head *list; > > > + struct list_head *list = NULL; > > > > Actually this is the wrong place to initialize. The correct place > > is in the function that is expected to. > > does it really matter? It's far more robust to initialize at the > definition site, because there we can be sure there's no > side-effects. This one: > > > /* Did we used up all 65 thousand events??? */ > > - if ((last + 1) > FTRACE_MAX_EVENT) > > + if ((last + 1) > FTRACE_MAX_EVENT) { > > + *list = NULL; > > return 0; > > + } > > Is correct too but needs a semantic check (and ongoing maintenance, > etc.).
Actually, to answer this, we need to look at the entire code. Just looking at the changes of a patch does not include the big picture.
The original code is:
static int trace_search_list(struct list_head **list) { struct trace_event *e; int last = __TRACE_LAST_TYPE;
if (list_empty(&ftrace_event_list)) { *list = &ftrace_event_list; return last + 1; }
/* * We used up all possible max events, * lets see if somebody freed one. */ list_for_each_entry(e, &ftrace_event_list, list) { if (e->type != last + 1) break; last++; }
/* Did we used up all 65 thousand events??? */ if ((last + 1) > FTRACE_MAX_EVENT) return 0;
*list = &e->list; return last + 1; }
[...] struct list_head *list;
if (next_event_type > FTRACE_MAX_EVENT) {
event->type = trace_search_list(&list); if (!event->type) goto out;
} else {
event->type = next_event_type++; list = &ftrace_event_list; }
if (WARN_ON(ftrace_find_event(event->type))) goto out;
list_add_tail(&event->list, list);
The caller is:
struct list_head *list;
if () { event->type = trace_seach_list(&list); } else { [...] list = &ftrace_event_list; }
This code shows that list is initialized by either trace_seach_list() or set manually.
Thus, my change makes trace_search_list always initialize the list variable. Thus if trace_search_list() is used someplace else, it will not cause us this error again.
If gcc can not figure out that trace_search_list initializes the code (from the original code), the
struct list_head *list = NULL;
will always be performed, because gcc thinks that's the only way to guarantee that it will be initialized.
My solution, gcc can easily see that trace_search_list will always initialize list, and will not set it needlessly to NULL.
-- Steve
| |