[PATCH] TRACING: Fix a copmile warning

[PATCH] TRACING: Fix a copmile warning

[stufever]

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

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Wang Shaoyan <wangshaoyan.pt@taobao.com>

---
 kernel/trace/trace_printk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
index 1f06468..1a5fff8 100644
--- a/kernel/trace/trace_printk.c
+++ b/kernel/trace/trace_printk.c
@@ -49,7 +49,7 @@ static
 void hold_module_trace_bprintk_format(const char **start, const char **end)
 {
 	const char **iter;
-	char *fmt;
+	char *fmt = NULL;
 
 	mutex_lock(&btrace_mutex);
 	for (iter = start; iter < end; iter++) {
-- 
1.7.4.1

Re: [PATCH] TRACING: Fix a copmile warning

[Steven Rostedt]

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.

-- Steve

> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Signed-off-by: Wang Shaoyan <wangshaoyan.pt@taobao.com>
> 
> ---
>  kernel/trace/trace_printk.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
> index 1f06468..1a5fff8 100644
> --- a/kernel/trace/trace_printk.c
> +++ b/kernel/trace/trace_printk.c
> @@ -49,7 +49,7 @@ static
>  void hold_module_trace_bprintk_format(const char **start, const char **end)
>  {
>  	const char **iter;
> -	char *fmt;
> +	char *fmt = NULL;
>  
>  	mutex_lock(&btrace_mutex);
>  	for (iter = start; iter < end; iter++) {

Re: [PATCH] TRACING: Fix a copmile warning

[Arnaud Lacombe]

Hi,

On Mon, Jul 25, 2011 at 2:32 PM, Steven Rostedt <rostedt@goodmis.org> 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.
>
Actually, we have a special uninitialized_var(x) macro to handle such
false positive. From include/linux/compiler-gcc.h:

/*
 * A trick to suppress uninitialized variable warning without generating any
 * code
 */
#define uninitialized_var(x) x = x

 - Arnaud

> -- Steve
>
>>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Signed-off-by: Wang Shaoyan <wangshaoyan.pt@taobao.com>
>>
>> ---
>>  kernel/trace/trace_printk.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
>> index 1f06468..1a5fff8 100644
>> --- a/kernel/trace/trace_printk.c
>> +++ b/kernel/trace/trace_printk.c
>> @@ -49,7 +49,7 @@ static
>>  void hold_module_trace_bprintk_format(const char **start, const char **end)
>>  {
>>       const char **iter;
>> -     char *fmt;
>> +     char *fmt = NULL;
>>
>>       mutex_lock(&btrace_mutex);
>>       for (iter = start; iter < end; iter++) {
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] TRACING: Fix a copmile warning

[Steven Rostedt]

On Mon, 2011-07-25 at 15:43 -0400, Arnaud Lacombe wrote:

> Actually, we have a special uninitialized_var(x) macro to handle such
> false positive. From include/linux/compiler-gcc.h:
> 
> /*
>  * A trick to suppress uninitialized variable warning without generating any
>  * code
>  */
> #define uninitialized_var(x) x = x

I'm aware of that too, but I think that is inappropriate as well. As I
said, some versions of gcc report it, others don't. Seems that gcc 4.6.0
says this is an error where 4.5.1 does not (I just tried both). Looks to
me like a regression in gcc. Why not fix it there?

-- Steve

Re: [PATCH] TRACING: Fix a copmile warning

[Arnaud Lacombe]

Hi,

On Mon, Jul 25, 2011 at 4:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 2011-07-25 at 15:43 -0400, Arnaud Lacombe wrote:
>
>> Actually, we have a special uninitialized_var(x) macro to handle such
>> false positive. From include/linux/compiler-gcc.h:
>>
>> /*
>>  * A trick to suppress uninitialized variable warning without generating any
>>  * code
>>  */
>> #define uninitialized_var(x) x = x
>
> I'm aware of that too, but I think that is inappropriate as well. As I
> said, some versions of gcc report it, others don't. Seems that gcc 4.6.0
> says this is an error where 4.5.1 does not (I just tried both). Looks to
> me like a regression in gcc. Why not fix it there?
>
It should, but gcc 4.6.x has already been shipped out there, so we
will have to live with that for a while. Marking this as a false
positive now will avoid having multiple submission for this in the
future.

 - Arnaud

> -- Steve
>
>
>

Re: [PATCH] TRACING: Fix a copmile warning

[Arnaud Lacombe]

Hi,

On Mon, Jul 25, 2011 at 4:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 2011-07-25 at 15:43 -0400, Arnaud Lacombe wrote:
>
>> Actually, we have a special uninitialized_var(x) macro to handle such
>> false positive. From include/linux/compiler-gcc.h:
>>
>> /*
>>  * A trick to suppress uninitialized variable warning without generating any
>>  * code
>>  */
>> #define uninitialized_var(x) x = x
>
> I'm aware of that too, but I think that is inappropriate as well. As I
> said, some versions of gcc report it, others don't. Seems that gcc 4.6.0
> says this is an error where 4.5.1 does not (I just tried both).
>
did you ? gcc 4.5.1 from Fedora 14 definitively shows the warning:

# make ARCH=i386 kernel/trace/trace_printk.o V=1
[...]
gcc -Wp,-MD,kernel/trace/.trace_printk.o.d  -nostdinc -isystem
/usr/lib/gcc/x86_64-redhat-linux/4.5.1/include [...] -c -o
kernel/trace/trace_printk.o kernel/trace/trace_printk.c
/src/linux/linux/kernel/trace/trace_printk.c: In function
'hold_module_trace_bprintk_format':
/src/linux/linux/kernel/trace/trace_printk.c:52:8: warning: 'fmt' may
be used uninitialized in this function

# gcc -v
gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/4.5.1/lto-wrapper
Target: x86_64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man
--infodir=/usr/share/info
--with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap
--enable-shared --enable-threads=posix --enable-checking=release
--with-system-zlib --enable-__cxa_atexit
--disable-libunwind-exceptions --enable-gnu-unique-object
--enable-linker-build-id
--enable-languages=c,c++,objc,obj-c++,java,fortran,ada,lto
--enable-plugin --enable-java-awt=gtk --disable-dssi
--with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre
--enable-libgcj-multifile --enable-java-maintainer-mode
--with-ecj-jar=/usr/share/java/eclipse-ecj.jar
--disable-libjava-multilib --with-ppl --with-cloog --with-tune=generic
--with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
gcc version 4.5.1 20100924 (Red Hat 4.5.1-4) (GCC)

 - Arnaud

Re: [PATCH] TRACING: Fix a copmile warning

[Steven Rostedt]

On Mon, 2011-07-25 at 18:38 -0400, Arnaud Lacombe wrote:
> Hi,
> 
> On Mon, Jul 25, 2011 at 4:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Mon, 2011-07-25 at 15:43 -0400, Arnaud Lacombe wrote:
> >
> >> Actually, we have a special uninitialized_var(x) macro to handle such
> >> false positive. From include/linux/compiler-gcc.h:
> >>
> >> /*
> >>  * A trick to suppress uninitialized variable warning without generating any
> >>  * code
> >>  */
> >> #define uninitialized_var(x) x = x
> >
> > I'm aware of that too, but I think that is inappropriate as well. As I
> > said, some versions of gcc report it, others don't. Seems that gcc 4.6.0
> > says this is an error where 4.5.1 does not (I just tried both).
> >
> did you ? gcc 4.5.1 from Fedora 14 definitively shows the warning:

Heh, I didn't use Fedora's version. I wonder if they added a patch or
built it differently. I built my own 4.5.1 as well as my own 4.6.0.

-- Steve

> 
> # make ARCH=i386 kernel/trace/trace_printk.o V=1
> [...]
> gcc -Wp,-MD,kernel/trace/.trace_printk.o.d  -nostdinc -isystem
> /usr/lib/gcc/x86_64-redhat-linux/4.5.1/include [...] -c -o
> kernel/trace/trace_printk.o kernel/trace/trace_printk.c
> /src/linux/linux/kernel/trace/trace_printk.c: In function
> 'hold_module_trace_bprintk_format':
> /src/linux/linux/kernel/trace/trace_printk.c:52:8: warning: 'fmt' may
> be used uninitialized in this function
> 
> # gcc -v
> gcc -v
> Using built-in specs.
> COLLECT_GCC=gcc
> COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/4.5.1/lto-wrapper
> Target: x86_64-redhat-linux
> Configured with: ../configure --prefix=/usr --mandir=/usr/share/man
> --infodir=/usr/share/info
> --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap
> --enable-shared --enable-threads=posix --enable-checking=release
> --with-system-zlib --enable-__cxa_atexit
> --disable-libunwind-exceptions --enable-gnu-unique-object
> --enable-linker-build-id
> --enable-languages=c,c++,objc,obj-c++,java,fortran,ada,lto
> --enable-plugin --enable-java-awt=gtk --disable-dssi
> --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre
> --enable-libgcj-multifile --enable-java-maintainer-mode
> --with-ecj-jar=/usr/share/java/eclipse-ecj.jar
> --disable-libjava-multilib --with-ppl --with-cloog --with-tune=generic
> --with-arch_32=i686 --build=x86_64-redhat-linux
> Thread model: posix
> gcc version 4.5.1 20100924 (Red Hat 4.5.1-4) (GCC)
> 
>  - Arnaud

Re: [PATCH] TRACING: Fix a copmile warning

[Arnaud Lacombe]

Hi,

[adding gcc-help@ to the Cc: list]

On Mon, Jul 25, 2011 at 6:38 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> Hi,
>
> On Mon, Jul 25, 2011 at 4:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Mon, 2011-07-25 at 15:43 -0400, Arnaud Lacombe wrote:
>>
>>> Actually, we have a special uninitialized_var(x) macro to handle such
>>> false positive. From include/linux/compiler-gcc.h:
>>>
>>> /*
>>>  * A trick to suppress uninitialized variable warning without generating any
>>>  * code
>>>  */
>>> #define uninitialized_var(x) x = x
>>
>> I'm aware of that too, but I think that is inappropriate as well. As I
>> said, some versions of gcc report it, others don't. Seems that gcc 4.6.0
>> says this is an error where 4.5.1 does not (I just tried both).
>>
gcc will only emits the warning at -Os. It seems to me that the
resulting code clearly ends-up testing an uninitialized value, ie.
assuming the following test-case:

extern void *e(void);
extern void *f(void);
extern void g(void);

void fn(void)
{
        void *b, *a;

        a = e();
        if (a != 0)
                b = f();
        if (a != 0 && b != 0)
                g();
}

gcc 4.5.1 will generates the following x86-32 assembly:

% gcc -m32 -Wall -Os -c -S -o - kernel/trace/trace_printk.c
        .file   "trace_printk.c"
kernel/trace/trace_printk.c: In function 'fn':
kernel/trace/trace_printk.c:7:8: warning: 'b' may be used
uninitialized in this function
        .text
.globl fn
        .type   fn, @function
fn:
        pushl   %ebp
        movl    %esp, %ebp
        pushl   %esi
        pushl   %ebx
        call    e
        testl   %eax, %eax
        movl    %eax, %ebx
        je      .L2
        call    f
        movl    %eax, %esi
.L2:
        testl   %esi, %esi
        je      .L1
        testl   %ebx, %ebx
        je      .L1
        popl    %ebx
        popl    %esi
        popl    %ebp
        jmp     g
.L1:
        popl    %ebx
        popl    %esi
        popl    %ebp
        ret
        .size   fn, .-fn
        .ident  "GCC: (GNU) 4.5.1 20100924 (Red Hat 4.5.1-4)"
        .section        .note.GNU-stack,"",@progbits

It seems gcc transforms the conditional from:

if (a != NULL && b != NULL) ...

to

if (b != NULL && a != NULL) ...

In which case the warning is fully valid. I'm not sure what's the C
standard guarantee in term of conditional test order. gcc 4.7.0 has
the same behavior.

 - Arnaud

Re: [PATCH] TRACING: Fix a copmile warning

[Arnaud Lacombe]

Hi,

On Mon, Jul 25, 2011 at 7:49 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 2011-07-25 at 18:38 -0400, Arnaud Lacombe wrote:
>> Hi,
>>
>> On Mon, Jul 25, 2011 at 4:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> > On Mon, 2011-07-25 at 15:43 -0400, Arnaud Lacombe wrote:
>> >
>> >> Actually, we have a special uninitialized_var(x) macro to handle such
>> >> false positive. From include/linux/compiler-gcc.h:
>> >>
>> >> /*
>> >>  * A trick to suppress uninitialized variable warning without generating any
>> >>  * code
>> >>  */
>> >> #define uninitialized_var(x) x = x
>> >
>> > I'm aware of that too, but I think that is inappropriate as well. As I
>> > said, some versions of gcc report it, others don't. Seems that gcc 4.6.0
>> > says this is an error where 4.5.1 does not (I just tried both).
>> >
>> did you ? gcc 4.5.1 from Fedora 14 definitively shows the warning:
>
> Heh, I didn't use Fedora's version. I wonder if they added a patch or
> built it differently. I built my own 4.5.1 as well as my own 4.6.0.
>
I'd assume you're building with -O2, the warning only shows up at -Os,
see my previous mail.

 - Arnaud

Re: [PATCH] TRACING: Fix a copmile warning

[Arnaud Lacombe]

Hi,

On Mon, Jul 25, 2011 at 7:50 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> Hi,
>
> [adding gcc-help@ to the Cc: list]
>
> On Mon, Jul 25, 2011 at 6:38 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
>> Hi,
>>
>> On Mon, Jul 25, 2011 at 4:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>>> On Mon, 2011-07-25 at 15:43 -0400, Arnaud Lacombe wrote:
>>>
>>>> Actually, we have a special uninitialized_var(x) macro to handle such
>>>> false positive. From include/linux/compiler-gcc.h:
>>>>
>>>> /*
>>>>  * A trick to suppress uninitialized variable warning without generating any
>>>>  * code
>>>>  */
>>>> #define uninitialized_var(x) x = x
>>>
>>> I'm aware of that too, but I think that is inappropriate as well. As I
>>> said, some versions of gcc report it, others don't. Seems that gcc 4.6.0
>>> says this is an error where 4.5.1 does not (I just tried both).
>>>
> [...]
> In which case the warning is fully valid. I'm not sure what's the C
> standard guarantee in term of conditional test order.
I'd assume that the following apply:

6.5 Expressions

3 The grouping of operators and operands is indicated by the
syntax.72) Except as specified
   later (for the function-call (), &&, ||, ?:, and comma operators),
the order of evaluation
  of subexpressions and the order in which side effects take place are
both unspecified.


in which case gcc is free to do whatever it wants :(

 - Arnaud

Re: [PATCH] TRACING: Fix a copmile warning

[Steven Rostedt]

On Mon, 2011-07-25 at 19:58 -0400, Arnaud Lacombe wrote:
> Hi,

> > [...]
> > In which case the warning is fully valid. I'm not sure what's the C
> > standard guarantee in term of conditional test order.
> I'd assume that the following apply:
> 
> 6.5 Expressions
> 
> 3 The grouping of operators and operands is indicated by the
> syntax.72) Except as specified
>    later (for the function-call (), &&, ||, ?:, and comma operators),
> the order of evaluation
>   of subexpressions and the order in which side effects take place are
> both unspecified.
> 
> 
> in which case gcc is free to do whatever it wants :(

No it does not! Read what you wrote: "Except as specified later (for the
function-call(), &&, ||, ?:...)"

&& and || must be short cuts. That is, it must evaluate the earlier
statements before the later, and exit when it can. We use that all over
the kernel (and in all C code):

	if (ptr && ptr->field)

If it were to switch that to:

	if (ptr->field && ptr)

we would have segfaults everywhere.


This looks like a serious gcc bug.

-- Steve

Re: [PATCH] TRACING: Fix a copmile warning

[Steven Rostedt]

On Mon, 2011-07-25 at 19:52 -0400, Arnaud Lacombe wrote:
> Hi,
> 
> On Mon, Jul 25, 2011 at 7:49 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Mon, 2011-07-25 at 18:38 -0400, Arnaud Lacombe wrote:
> >> Hi,
> >>
> >> On Mon, Jul 25, 2011 at 4:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >> > On Mon, 2011-07-25 at 15:43 -0400, Arnaud Lacombe wrote:
> >> >
> >> >> Actually, we have a special uninitialized_var(x) macro to handle such
> >> >> false positive. From include/linux/compiler-gcc.h:
> >> >>
> >> >> /*
> >> >>  * A trick to suppress uninitialized variable warning without generating any
> >> >>  * code
> >> >>  */
> >> >> #define uninitialized_var(x) x = x
> >> >
> >> > I'm aware of that too, but I think that is inappropriate as well. As I
> >> > said, some versions of gcc report it, others don't. Seems that gcc 4.6.0
> >> > says this is an error where 4.5.1 does not (I just tried both).
> >> >
> >> did you ? gcc 4.5.1 from Fedora 14 definitively shows the warning:
> >
> > Heh, I didn't use Fedora's version. I wonder if they added a patch or
> > built it differently. I built my own 4.5.1 as well as my own 4.6.0.
> >
> I'd assume you're building with -O2, the warning only shows up at -Os,
> see my previous mail.

No I had CONFIG_CC_OPTIMIZE_FOR_SIZE=y (which should do the -Os). I
could also compile with V=1 but I'm in the middle of other things at the
moment.

But I just realized you are doing i386, while I'm compiling with x86_64.
That could also be the difference.

Anyway, I'll look at what it produces and see if this is a bug or not.

-- Steve

Re: [PATCH] TRACING: Fix a copmile warning

[Steven Rostedt]

On Mon, 2011-07-25 at 19:50 -0400, Arnaud Lacombe wrote:

> gcc will only emits the warning at -Os. It seems to me that the
> resulting code clearly ends-up testing an uninitialized value, ie.
> assuming the following test-case:
> 
> extern void *e(void);
> extern void *f(void);
> extern void g(void);
> 
> void fn(void)
> {
>         void *b, *a;
> 
>         a = e();
>         if (a != 0)
>                 b = f();
>         if (a != 0 && b != 0)
>                 g();
> }
> 
> gcc 4.5.1 will generates the following x86-32 assembly:
> 
> % gcc -m32 -Wall -Os -c -S -o - kernel/trace/trace_printk.c
>         .file   "trace_printk.c"
> kernel/trace/trace_printk.c: In function 'fn':
> kernel/trace/trace_printk.c:7:8: warning: 'b' may be used
> uninitialized in this function
>         .text
> .globl fn
>         .type   fn, @function
> fn:
>         pushl   %ebp
>         movl    %esp, %ebp
>         pushl   %esi
>         pushl   %ebx
>         call    e
>         testl   %eax, %eax
>         movl    %eax, %ebx
>         je      .L2
>         call    f
>         movl    %eax, %esi
> .L2:
>         testl   %esi, %esi
>         je      .L1
>         testl   %ebx, %ebx
>         je      .L1
>         popl    %ebx
>         popl    %esi
>         popl    %ebp
>         jmp     g
> .L1:
>         popl    %ebx
>         popl    %esi
>         popl    %ebp
>         ret
>         .size   fn, .-fn
>         .ident  "GCC: (GNU) 4.5.1 20100924 (Red Hat 4.5.1-4)"
>         .section        .note.GNU-stack,"",@progbits
> 

I wrote a similar program and got the same results for both 4.5.1 and
4.6.0. but only with -Os and -O2 seems fine.

> It seems gcc transforms the conditional from:
> 
> if (a != NULL && b != NULL) ...
> 
> to
> 
> if (b != NULL && a != NULL) ...
> 
> In which case the warning is fully valid. I'm not sure what's the C
> standard guarantee in term of conditional test order. gcc 4.7.0 has
> the same behavior.

Yes it seems to be doing this :-/

This is a real bug!

-- Steve

Re: [PATCH] TRACING: Fix a copmile warning

[Ian Lance Taylor]

Arnaud Lacombe <lacombar@gmail.com> writes:

> gcc will only emits the warning at -Os. It seems to me that the
> resulting code clearly ends-up testing an uninitialized value, ie.
> assuming the following test-case:
>
> extern void *e(void);
> extern void *f(void);
> extern void g(void);
>
> void fn(void)
> {
>         void *b, *a;
>
>         a = e();
>         if (a != 0)
>                 b = f();
>         if (a != 0 && b != 0)
>                 g();
> }
>
> ...
>
> It seems gcc transforms the conditional from:
>
> if (a != NULL && b != NULL) ...
>
> to
>
> if (b != NULL && a != NULL) ...
>
> In which case the warning is fully valid. I'm not sure what's the C
> standard guarantee in term of conditional test order. gcc 4.7.0 has
> the same behavior.

Not quite.  C guarantees that && is executed in order.  In this case gcc
is generating

  a = e();
  if (a != NULL)
    b = f();
  if (a != NULL & b != NULL)
    g();

Note the change from && to & in the last conditional.  This
transformation is safe, in that it does not change the meaning of the
program.  However, it does cause a read of an uninitialized memory
location, and this is causing a later gcc pass to generate a false
positive warning.

Please consider filing a bug report about this false positive.  Thanks.

Ian

Re: [PATCH] TRACING: Fix a copmile warning

[Ian Lance Taylor]

Steven Rostedt <rostedt@goodmis.org> writes:

>> It seems gcc transforms the conditional from:
>> 
>> if (a != NULL && b != NULL) ...
>> 
>> to
>> 
>> if (b != NULL && a != NULL) ...
>> 
>> In which case the warning is fully valid. I'm not sure what's the C
>> standard guarantee in term of conditional test order. gcc 4.7.0 has
>> the same behavior.
>
> Yes it seems to be doing this :-/
>
> This is a real bug!

To be clear, gcc is not doing that, and this is not a code generation
bug.  It is a warning generation bug; the generated code is correct.

Ian

Re: [PATCH] TRACING: Fix a copmile warning

[Arnaud Lacombe]

Hi,

On Mon, Jul 25, 2011 at 8:41 PM, Ian Lance Taylor <iant@google.com> wrote:
> Arnaud Lacombe <lacombar@gmail.com> writes:
>
>> gcc will only emits the warning at -Os. It seems to me that the
>> resulting code clearly ends-up testing an uninitialized value, ie.
>> assuming the following test-case:
>>
>> extern void *e(void);
>> extern void *f(void);
>> extern void g(void);
>>
>> void fn(void)
>> {
>>         void *b, *a;
>>
>>         a = e();
>>         if (a != 0)
>>                 b = f();
>>         if (a != 0 && b != 0)
>>                 g();
>> }
>>
>> ...
>>
>> It seems gcc transforms the conditional from:
>>
>> if (a != NULL && b != NULL) ...
>>
>> to
>>
>> if (b != NULL && a != NULL) ...
>>
>> In which case the warning is fully valid. I'm not sure what's the C
>> standard guarantee in term of conditional test order. gcc 4.7.0 has
>> the same behavior.
>
> Not quite.  C guarantees that && is executed in order.  In this case gcc
> is generating
>
>  a = e();
>  if (a != NULL)
>    b = f();
>  if (a != NULL & b != NULL)
>    g();
>
> Note the change from && to & in the last conditional.
I've got some trouble linking the final optimized tree with the code
generated. That is, the final tree (generated by -fdump-tree-all) is:

foo ()
{
  void * b;
  void * a;
  _Bool D.1993;
  _Bool D.1992;
  _Bool D.1991;

<bb 2>:
  a_2 = e ();
  if (a_2 != 0B)
    goto <bb 3>;
  else
    goto <bb 4>;

<bb 3>:
  b_4 = f ();

<bb 4>:
  # b_1 = PHI <b_3(D)(2), b_4(3)>
  D.1991_5 = a_2 != 0B;
  D.1992_6 = b_1 != 0B;
  D.1993_7 = D.1992_6 & D.1991_5;
  if (D.1993_7 != 0)
    goto <bb 5>;
  else
    goto <bb 6>;

<bb 5>:
  g (); [tail call]

<bb 6>:
  return;

}

but the code generated seem to test %esi (`b', potentially
uninitialized) before %ebx (`a'). Am I still missing something ?

Thanks,
 - Arnaud

Re: [PATCH] TRACING: Fix a copmile warning

[Steven Rostedt]

On Mon, 2011-07-25 at 17:41 -0700, Ian Lance Taylor wrote:
> Arnaud Lacombe <lacombar@gmail.com> writes:

> > It seems gcc transforms the conditional from:
> >
> > if (a != NULL && b != NULL) ...
> >
> > to
> >
> > if (b != NULL && a != NULL) ...
> >
> > In which case the warning is fully valid. I'm not sure what's the C
> > standard guarantee in term of conditional test order. gcc 4.7.0 has
> > the same behavior.
> 
> Not quite.  C guarantees that && is executed in order.  In this case gcc
> is generating
> 
>   a = e();
>   if (a != NULL)
>     b = f();
>   if (a != NULL & b != NULL)
>     g();
> 
> Note the change from && to & in the last conditional.  This
> transformation is safe, in that it does not change the meaning of the
> program.  However, it does cause a read of an uninitialized memory
> location, and this is causing a later gcc pass to generate a false
> positive warning.
> 

Looking at the assembly again, and not knowing what gcc is doing
internally, it does seem to be:

	if (a != NULL)
		b = f();
	if (b != NULL && a != NULL)
		g();

But if the first conditional fails, then the second will never pass
regardless of what b is. In which case, it is the same as:

	if (a != NULL)
		b = f();
	if (a != NULL && b != NULL)
		g();

And it doesn't change the meaning of the code.

> Please consider filing a bug report about this false positive.  Thanks.

I agree that this is just a warning bug.


On a tangent:

Compiling with -O2 (which gives no warning) (x86_64) produces:

0000000000000000 <fn>:
   0:	48 83 ec 08          	sub    $0x8,%rsp
   4:	e8 00 00 00 00       	callq  9 <fn+0x9>
			5: R_X86_64_PC32	e-0x4
   9:	48 85 c0             	test   %rax,%rax
   c:	74 1a                	je     28 <fn+0x28>
   e:	e8 00 00 00 00       	callq  13 <fn+0x13>
			f: R_X86_64_PC32	f-0x4
  13:	48 85 c0             	test   %rax,%rax
  16:	74 10                	je     28 <fn+0x28>
  18:	48 83 c4 08          	add    $0x8,%rsp
  1c:	e9 00 00 00 00       	jmpq   21 <fn+0x21>
			1d: R_X86_64_PC32	g-0x4
  21:	0f 1f 80 00 00 00 00 	nopl   0x0(%rax)
  28:	48 83 c4 08          	add    $0x8,%rsp
  2c:	c3                   	retq   

and compiling with -Os:

0000000000000000 <fn>:
   0:	55                   	push   %rbp
   1:	53                   	push   %rbx
   2:	51                   	push   %rcx
   3:	e8 00 00 00 00       	callq  8 <fn+0x8>
			4: R_X86_64_PC32	e-0x4
   8:	48 85 c0             	test   %rax,%rax
   b:	48 89 c3             	mov    %rax,%rbx
   e:	74 08                	je     18 <fn+0x18>
  10:	e8 00 00 00 00       	callq  15 <fn+0x15>
			11: R_X86_64_PC32	f-0x4
  15:	48 89 c5             	mov    %rax,%rbp
  18:	48 85 ed             	test   %rbp,%rbp
  1b:	74 0d                	je     2a <fn+0x2a>
  1d:	48 85 db             	test   %rbx,%rbx
  20:	74 08                	je     2a <fn+0x2a>
  22:	5a                   	pop    %rdx
  23:	5b                   	pop    %rbx
  24:	5d                   	pop    %rbp
  25:	e9 00 00 00 00       	jmpq   2a <fn+0x2a>
			26: R_X86_64_PC32	g-0x4
  2a:	58                   	pop    %rax
  2b:	5b                   	pop    %rbx
  2c:	5d                   	pop    %rbp
  2d:	c3                   	retq   

Which is 1 byte more than -O2. I would think that -Os would be smaller.

-- Steve

Re: [PATCH] TRACING: Fix a copmile warning

[Steven Rostedt]

On Mon, 2011-07-25 at 21:08 -0400, Arnaud Lacombe wrote:
> Hi,
> 

> but the code generated seem to test %esi (`b', potentially
> uninitialized) before %ebx (`a'). Am I still missing something ?

But it tests 'a' again afterward. If 'a' is 0, it doesn't matter what
'b' was. So the uninitialized test is a wash. No harm done, except for
some wasted CPU cycles.

-- Steve

Re: [PATCH] TRACING: Fix a copmile warning

[Arnaud Lacombe]

Hi,

On Mon, Jul 25, 2011 at 9:12 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 2011-07-25 at 21:08 -0400, Arnaud Lacombe wrote:
>> Hi,
>>
>
>> but the code generated seem to test %esi (`b', potentially
>> uninitialized) before %ebx (`a'). Am I still missing something ?
>
> But it tests 'a' again afterward. If 'a' is 0, it doesn't matter what
> 'b' was. So the uninitialized test is a wash. No harm done, except for
> some wasted CPU cycles.
>
I see, even if `b' is junk, the final logic does not change.

 - Arnaud

Re: [PATCH] TRACING: Fix a copmile warning

[Ian Lance Taylor]

Steven Rostedt <rostedt@goodmis.org> writes:

> Compiling with -O2 (which gives no warning) (x86_64) produces:
>
> 0000000000000000 <fn>:
>    0:	48 83 ec 08          	sub    $0x8,%rsp
>    4:	e8 00 00 00 00       	callq  9 <fn+0x9>
> 			5: R_X86_64_PC32	e-0x4
>    9:	48 85 c0             	test   %rax,%rax
>    c:	74 1a                	je     28 <fn+0x28>
>    e:	e8 00 00 00 00       	callq  13 <fn+0x13>
> 			f: R_X86_64_PC32	f-0x4
>   13:	48 85 c0             	test   %rax,%rax
>   16:	74 10                	je     28 <fn+0x28>
>   18:	48 83 c4 08          	add    $0x8,%rsp
>   1c:	e9 00 00 00 00       	jmpq   21 <fn+0x21>
> 			1d: R_X86_64_PC32	g-0x4
>   21:	0f 1f 80 00 00 00 00 	nopl   0x0(%rax)
>   28:	48 83 c4 08          	add    $0x8,%rsp
>   2c:	c3                   	retq   
>
> and compiling with -Os:
>
> 0000000000000000 <fn>:
>    0:	55                   	push   %rbp
>    1:	53                   	push   %rbx
>    2:	51                   	push   %rcx
>    3:	e8 00 00 00 00       	callq  8 <fn+0x8>
> 			4: R_X86_64_PC32	e-0x4
>    8:	48 85 c0             	test   %rax,%rax
>    b:	48 89 c3             	mov    %rax,%rbx
>    e:	74 08                	je     18 <fn+0x18>
>   10:	e8 00 00 00 00       	callq  15 <fn+0x15>
> 			11: R_X86_64_PC32	f-0x4
>   15:	48 89 c5             	mov    %rax,%rbp
>   18:	48 85 ed             	test   %rbp,%rbp
>   1b:	74 0d                	je     2a <fn+0x2a>
>   1d:	48 85 db             	test   %rbx,%rbx
>   20:	74 08                	je     2a <fn+0x2a>
>   22:	5a                   	pop    %rdx
>   23:	5b                   	pop    %rbx
>   24:	5d                   	pop    %rbp
>   25:	e9 00 00 00 00       	jmpq   2a <fn+0x2a>
> 			26: R_X86_64_PC32	g-0x4
>   2a:	58                   	pop    %rax
>   2b:	5b                   	pop    %rbx
>   2c:	5d                   	pop    %rbp
>   2d:	c3                   	retq   
>
> Which is 1 byte more than -O2. I would think that -Os would be smaller.

Ideally, it should be, yes.  The -Os code would be smaller except that
it needs to save a register across a function call, which forces it to
push and pop %rbx, which in turn means that stack alignment adds yet
another push and two pop instructions.  It's a heuristic failure.

Ian

Re: [PATCH] TRACING: Fix a copmile warning

[Paulo Marques]

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...

-- 
Paulo Marques - www.grupopie.com

"This version has many new and good features. Sadly, the good ones are
not new and the new ones are not good."

Re: [PATCH] TRACING: Fix a copmile warning

[Jesper Juhl]

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);
        }
}


-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] TRACING: Fix a copmile warning

[Paulo Marques]

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."

Re: [PATCH] TRACING: Fix a copmile warning

[Steven Rostedt]

On Tue, 2011-07-26 at 14:32 +0100, Paulo Marques wrote:
> Jesper Juhl wrote:

> >> 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)...

gcc may not be allowed to optimize it as iter points to a global, and
external functions are called (kmalloc).

But what would work is:

static
void hold_module_trace_bprintk_format(const char **start, const char **end)
{
	const char **iter;
	char *fmt;

	mutex_lock(&btrace_mutex);
	for (iter = start; iter < end; iter++) {
		struct trace_bprintk_fmt *tb_fmt = lookup_format(*iter);
		if (tb_fmt) {
			*iter = tb_fmt->fmt;
			continue;
		}

		fmt = 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;
			} else
				kfree(tb_fmt);
		}
		*iter = fmt;
	}
	mutex_unlock(&btrace_mutex);
}

The above is easier to read, removes the false warning, and uses local
variables that gcc can optimize.

-- Steve

Re: [PATCH] TRACING: Fix a copmile warning

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/25/11 19:19, Arnaud Lacombe wrote:
> Hi,
> 
> On Mon, Jul 25, 2011 at 9:12 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Mon, 2011-07-25 at 21:08 -0400, Arnaud Lacombe wrote:
>>> Hi,
>>>
>>
>>> but the code generated seem to test %esi (`b', potentially
>>> uninitialized) before %ebx (`a'). Am I still missing something ?
>>
>> But it tests 'a' again afterward. If 'a' is 0, it doesn't matter what
>> 'b' was. So the uninitialized test is a wash. No harm done, except for
>> some wasted CPU cycles.
>>
> I see, even if `b' is junk, the final logic does not change.
Right.
Here's the .optimized dump I get for -Os:



<bb 2>:
  a_2 = e ();
  if (a_2 != 0B)
    goto <bb 3>;
  else
    goto <bb 4>;

<bb 3>:
  b_4 = f ();

<bb 4>:
  # b_1 = PHI <b_3(D)(2), b_4(3)>
  D.2708_5 = a_2 != 0B;
  D.2709_6 = b_1 != 0B;
  D.2710_7 = D.2709_6 & D.2708_5;
  if (D.2710_7 != 0)
    goto <bb 5>;
  else
    goto <bb 6>;

<bb 5>:
  g (); [tail call]

<bb 6>:
  return;

If we duplicate bb4 and isolate the paths leading out of bb2 we'd get
something like this:

<bb 2>:
  a_2 = e ();
  if (a_2 != 0B)
    goto <bb 3>;
  else
    goto <bb 7>;

<bb 3>:
  b_4 = f ();

<bb 4>:
  # b_1 = PHI  b_4(3)>
  D.2708_5 = a_2 != 0B;
  D.2709_6 = b_1 != 0B;
  D.2710_7 = D.2709_6 & D.2708_5;
  if (D.2710_7 != 0)
    goto <bb 5>;
  else
    goto <bb 6>;

<bb 5>:
  g (); [tail call]

<bb 6>:
  return;

<bb 7>:
  # b_1 = PHI <b_3(D)(2))>
  D.2708_5 = a_2 != 0B;
  D.2709_6 = b_1 != 0B;
  D.2710_7 = D.2709_6 & D.2708_5;
  if (D.2710_7 != 0)
    goto <bb 5>;
  else
    goto <bb 6>;


[I'm not going to fix all the SSA_NAMEs, I'm just showing roughly what
can be done with this code... ]

Then we can propagate the fact that a_2 == 0 into bb7 and a2 != 0 into
bb4.  That in turn allows bb7 to collapse into "goto bb6" and bb4
simplifies into if (b4 != 0) goto bb5 else goto bb6.  CFG
simplifications then eliminate the jump and combine bb3 & bb4 resulting in:




<bb 2>:
  a_2 = e ();
  if (a_2 != 0B)
    goto <bb 3>;
  else
    goto <bb 6>;

<bb 3>:
  b_4 = f ();
  if (b_4 != 0)
    goto <bb 5>;
  else
    goto <bb 6>;

<bb 5>:
  g (); [tail call]

<bb 6>:
  return;


Which just happens to be the code we get from -O2.  Unfortunately we
don't in general know when block duplication of this nature is going to
allow enough simplification to justify the possible code expansion.
Thus block duplication of this nature is severely limited when compiling
with -Os.

Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJOLycMAAoJEBRtltQi2kC7wbQH/RO2vAAEoHFuJKapNewMcL39
Fm404+DxN1BWwt6HT1BQYfH9tpZCepAJzDVcVsYWFlDOabLUKos9Sju/O8qkSkWK
gYdZ/XbhntB+vNaEQEXz+DdDnbX1OCPCTwM5bjtJ6Gwkjk5W7fa+GHFo694UZ0fH
fgKpAuTVFaCGj94whReGmsNDe0CbpBYS9+Erx4uUsasfkyy5sLDwGaM7gEtg0VS/
ZMz2bP1RDslk3WVkitllO9msq7i70dlR8qw8UgaUSyyvpNrw0HcRxXoI9ZB82mm4
QCtrRD5S92gxdzTtJ0Jg6co+tS2NadDv4D4f9Fn1Ox2oE/oYWvKBvLgkI8Up2pk=
=5lTt
-----END PGP SIGNATURE-----

[PATCH] TRACING: Fix a copmile warning

[stufever]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

From: Wang Shaoyan <wangshaoyan.pt@inc.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

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Wang Shaoyan <wangshaoyan.pt@taobao.com>

---
 kernel/trace/trace_printk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
index 1f06468..1a5fff8 100644
--- a/kernel/trace/trace_printk.c
+++ b/kernel/trace/trace_printk.c
@@ -49,7 +49,7 @@ static
 void hold_module_trace_bprintk_format(const char **start, const char **end)
 {
 	const char **iter;
-	char *fmt;
+	char *fmt = NULL;
 
 	mutex_lock(&btrace_mutex);
 	for (iter = start; iter < end; iter++) {
-- 
1.7.4.1