Re: [PATCH] TRACING: Fix a copmile warning

[Paulo Marques <pmarques@grupopie.com>]

Jesper Juhl wrote:
> On Tue, 26 Jul 2011, Paulo Marques wrote:
> 
>> Steven Rostedt wrote:
>>> On Mon, 2011-07-18 at 17:40 +0800, stufever@gmail.com wrote:
>>>> From: Wang Shaoyan <wangshaoyan.pt@taobao.com>
>>>>
>>>> It's harmless but annyoing.
>>>>   kernel/trace/trace_printk.c: In function 'module_trace_bprintk_format_notify':
>>>>   kernel/trace/trace_printk.c:52: warning: 'fmt' may be used uninitialized in this function
>>> I prefer not to add this patch. Fix gcc. Actually some gcc's do not warn
>>> on this, others do. Here's the code that confuses gcc:
>>>
>>> 		tb_fmt = kmalloc(sizeof(*tb_fmt), GFP_KERNEL);
>>> 		if (tb_fmt)
>>> 			fmt = kmalloc(strlen(*iter) + 1, GFP_KERNEL);
>>> 		if (tb_fmt && fmt) {
>>> 			list_add_tail(&tb_fmt->list, &trace_bprintk_fmt_list);
>>> 			strcpy(fmt, *iter);
>>> 			tb_fmt->fmt = fmt;
>>> 			*iter = tb_fmt->fmt;
>>>
>>>
>>> fmt will never be looked at if tb_fmt is NULL, and fmt is initialized if
>>> tb_fmt is not NULL.
>> Yes, changing code just to please gcc is not nice. In this case,
>> changing the code to the more straightforward / naive implementation
>> might make it more readable (IMHO) and maybe even improve code
>> generation. I.e., something like this:
>>
>> tb_fmt = kmalloc(sizeof(*tb_fmt), GFP_KERNEL);
>> if (tb_fmt) {
>>         fmt = kmalloc(strlen(*iter) + 1, GFP_KERNEL);
>> 	if (fmt) {
>>         	list_add_tail(&tb_fmt->list, &trace_bprintk_fmt_list);
>>         	strcpy(fmt, *iter);
>>         	tb_fmt->fmt = fmt;
>>         	*iter = tb_fmt->fmt;
>>         } else {
>> 	        kfree(tb_fmt);
>>         	*iter = NULL;
>>         }
>> } else {
>>         *iter = NULL;
>> }
>>
>> The downside is that the "*iter = NULL" gets repeated twice...
>>
> 
> You could avoid that like this:
> 
> *iter = NULL;
> tb_fmt = kmalloc(sizeof(*tb_fmt), GFP_KERNEL);
> if (tb_fmt) {
>       fmt = kmalloc(strlen(*iter) + 1, GFP_KERNEL);
>       if (fmt) {
>               list_add_tail(&tb_fmt->list, &trace_bprintk_fmt_list);
>               strcpy(fmt, *iter);
>               tb_fmt->fmt = fmt;
>               *iter = tb_fmt->fmt;
>         } else {
>               kfree(tb_fmt);
>         }
> }

Yes, but this way you always set *iter to NULL, whereas in the previous
version that was the very unlikely case (kmalloc returning NULL).

Probably gcc is smart enough to generate the same code for both
versions, to avoid setting *iter twice for the likely case (and even if
it doesn't, then the cache will be hot and probably not written back
yet, yadda, yadda)...

-- 
Paulo Marques - www.grupopie.com

"I used to be indecisive, but now I'm not so sure."