Re: [tip:tracing/core] tracing: trace_output.c, fix false positive compiler warning

Steven Rostedt
2009-05-06T14:57:09Z

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