* [PATCH] TRACING: Fix a copmile warning
@ 2011-07-18 9:40 stufever
2011-07-25 18:32 ` Steven Rostedt
0 siblings, 1 reply; 26+ messages in thread
From: stufever @ 2011-07-18 9:40 UTC (permalink / raw)
To: linux-kernel
Cc: Wang Shaoyan, Steven Rostedt, Frederic Weisbecker, Ingo Molnar
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
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-18 9:40 [PATCH] TRACING: Fix a copmile warning stufever
@ 2011-07-25 18:32 ` Steven Rostedt
2011-07-25 19:43 ` Arnaud Lacombe
2011-07-26 12:00 ` Paulo Marques
0 siblings, 2 replies; 26+ messages in thread
From: Steven Rostedt @ 2011-07-25 18:32 UTC (permalink / raw)
To: stufever; +Cc: linux-kernel, Wang Shaoyan, Frederic Weisbecker, Ingo Molnar
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++) {
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 18:32 ` Steven Rostedt
@ 2011-07-25 19:43 ` Arnaud Lacombe
2011-07-25 20:19 ` Steven Rostedt
2011-07-26 12:00 ` Paulo Marques
1 sibling, 1 reply; 26+ messages in thread
From: Arnaud Lacombe @ 2011-07-25 19:43 UTC (permalink / raw)
To: Steven Rostedt
Cc: stufever, linux-kernel, Wang Shaoyan, Frederic Weisbecker,
Ingo Molnar
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/
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 19:43 ` Arnaud Lacombe
@ 2011-07-25 20:19 ` Steven Rostedt
2011-07-25 20:28 ` Arnaud Lacombe
2011-07-25 22:38 ` Arnaud Lacombe
0 siblings, 2 replies; 26+ messages in thread
From: Steven Rostedt @ 2011-07-25 20:19 UTC (permalink / raw)
To: Arnaud Lacombe
Cc: stufever, linux-kernel, Wang Shaoyan, Frederic Weisbecker,
Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 20:19 ` Steven Rostedt
@ 2011-07-25 20:28 ` Arnaud Lacombe
2011-07-25 22:38 ` Arnaud Lacombe
1 sibling, 0 replies; 26+ messages in thread
From: Arnaud Lacombe @ 2011-07-25 20:28 UTC (permalink / raw)
To: Steven Rostedt
Cc: stufever, linux-kernel, Wang Shaoyan, Frederic Weisbecker,
Ingo Molnar
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
>
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 20:19 ` Steven Rostedt
2011-07-25 20:28 ` Arnaud Lacombe
@ 2011-07-25 22:38 ` Arnaud Lacombe
2011-07-25 23:49 ` Steven Rostedt
2011-07-25 23:50 ` Arnaud Lacombe
1 sibling, 2 replies; 26+ messages in thread
From: Arnaud Lacombe @ 2011-07-25 22:38 UTC (permalink / raw)
To: Steven Rostedt
Cc: stufever, linux-kernel, Wang Shaoyan, Frederic Weisbecker,
Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 22:38 ` Arnaud Lacombe
@ 2011-07-25 23:49 ` Steven Rostedt
2011-07-25 23:52 ` Arnaud Lacombe
2011-07-25 23:50 ` Arnaud Lacombe
1 sibling, 1 reply; 26+ messages in thread
From: Steven Rostedt @ 2011-07-25 23:49 UTC (permalink / raw)
To: Arnaud Lacombe
Cc: stufever, linux-kernel, Wang Shaoyan, Frederic Weisbecker,
Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 23:49 ` Steven Rostedt
@ 2011-07-25 23:52 ` Arnaud Lacombe
2011-07-26 0:14 ` Steven Rostedt
0 siblings, 1 reply; 26+ messages in thread
From: Arnaud Lacombe @ 2011-07-25 23:52 UTC (permalink / raw)
To: Steven Rostedt
Cc: stufever, linux-kernel, Wang Shaoyan, Frederic Weisbecker,
Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 23:52 ` Arnaud Lacombe
@ 2011-07-26 0:14 ` Steven Rostedt
0 siblings, 0 replies; 26+ messages in thread
From: Steven Rostedt @ 2011-07-26 0:14 UTC (permalink / raw)
To: Arnaud Lacombe
Cc: stufever, linux-kernel, Wang Shaoyan, Frederic Weisbecker,
Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 22:38 ` Arnaud Lacombe
2011-07-25 23:49 ` Steven Rostedt
@ 2011-07-25 23:50 ` Arnaud Lacombe
2011-07-25 23:58 ` Arnaud Lacombe
` (2 more replies)
1 sibling, 3 replies; 26+ messages in thread
From: Arnaud Lacombe @ 2011-07-25 23:50 UTC (permalink / raw)
To: Steven Rostedt, gcc-help
Cc: stufever, linux-kernel, Wang Shaoyan, Frederic Weisbecker,
Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 23:50 ` Arnaud Lacombe
@ 2011-07-25 23:58 ` Arnaud Lacombe
2011-07-26 0:08 ` Steven Rostedt
2011-07-26 0:35 ` Steven Rostedt
2011-07-26 0:41 ` Ian Lance Taylor
2 siblings, 1 reply; 26+ messages in thread
From: Arnaud Lacombe @ 2011-07-25 23:58 UTC (permalink / raw)
To: Steven Rostedt, gcc-help
Cc: stufever, linux-kernel, Wang Shaoyan, Frederic Weisbecker,
Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 23:58 ` Arnaud Lacombe
@ 2011-07-26 0:08 ` Steven Rostedt
0 siblings, 0 replies; 26+ messages in thread
From: Steven Rostedt @ 2011-07-26 0:08 UTC (permalink / raw)
To: Arnaud Lacombe
Cc: gcc-help, stufever, linux-kernel, Wang Shaoyan,
Frederic Weisbecker, Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 23:50 ` Arnaud Lacombe
2011-07-25 23:58 ` Arnaud Lacombe
@ 2011-07-26 0:35 ` Steven Rostedt
2011-07-26 0:44 ` Ian Lance Taylor
2011-07-26 0:41 ` Ian Lance Taylor
2 siblings, 1 reply; 26+ messages in thread
From: Steven Rostedt @ 2011-07-26 0:35 UTC (permalink / raw)
To: Arnaud Lacombe
Cc: gcc-help, stufever, linux-kernel, Wang Shaoyan,
Frederic Weisbecker, Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-26 0:35 ` Steven Rostedt
@ 2011-07-26 0:44 ` Ian Lance Taylor
0 siblings, 0 replies; 26+ messages in thread
From: Ian Lance Taylor @ 2011-07-26 0:44 UTC (permalink / raw)
To: Steven Rostedt
Cc: Arnaud Lacombe, gcc-help, stufever, linux-kernel, Wang Shaoyan,
Frederic Weisbecker, Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 23:50 ` Arnaud Lacombe
2011-07-25 23:58 ` Arnaud Lacombe
2011-07-26 0:35 ` Steven Rostedt
@ 2011-07-26 0:41 ` Ian Lance Taylor
2011-07-26 1:08 ` Arnaud Lacombe
2011-07-26 1:10 ` Steven Rostedt
2 siblings, 2 replies; 26+ messages in thread
From: Ian Lance Taylor @ 2011-07-26 0:41 UTC (permalink / raw)
To: Arnaud Lacombe
Cc: Steven Rostedt, gcc-help, stufever, linux-kernel, Wang Shaoyan,
Frederic Weisbecker, Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-26 0:41 ` Ian Lance Taylor
@ 2011-07-26 1:08 ` Arnaud Lacombe
2011-07-26 1:12 ` Steven Rostedt
2011-07-26 1:10 ` Steven Rostedt
1 sibling, 1 reply; 26+ messages in thread
From: Arnaud Lacombe @ 2011-07-26 1:08 UTC (permalink / raw)
To: Ian Lance Taylor
Cc: Steven Rostedt, gcc-help, stufever, linux-kernel, Wang Shaoyan,
Frederic Weisbecker, Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-26 1:08 ` Arnaud Lacombe
@ 2011-07-26 1:12 ` Steven Rostedt
2011-07-26 1:19 ` Arnaud Lacombe
0 siblings, 1 reply; 26+ messages in thread
From: Steven Rostedt @ 2011-07-26 1:12 UTC (permalink / raw)
To: Arnaud Lacombe
Cc: Ian Lance Taylor, gcc-help, stufever, linux-kernel, Wang Shaoyan,
Frederic Weisbecker, Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-26 1:12 ` Steven Rostedt
@ 2011-07-26 1:19 ` Arnaud Lacombe
2011-07-26 20:43 ` Jeff Law
0 siblings, 1 reply; 26+ messages in thread
From: Arnaud Lacombe @ 2011-07-26 1:19 UTC (permalink / raw)
To: Steven Rostedt
Cc: Ian Lance Taylor, gcc-help, stufever, linux-kernel, Wang Shaoyan,
Frederic Weisbecker, Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-26 1:19 ` Arnaud Lacombe
@ 2011-07-26 20:43 ` Jeff Law
0 siblings, 0 replies; 26+ messages in thread
From: Jeff Law @ 2011-07-26 20:43 UTC (permalink / raw)
To: Arnaud Lacombe
Cc: Steven Rostedt, Ian Lance Taylor, gcc-help, stufever,
linux-kernel, Wang Shaoyan, Frederic Weisbecker, Ingo Molnar
-----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-----
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-26 0:41 ` Ian Lance Taylor
2011-07-26 1:08 ` Arnaud Lacombe
@ 2011-07-26 1:10 ` Steven Rostedt
2011-07-26 5:55 ` Ian Lance Taylor
1 sibling, 1 reply; 26+ messages in thread
From: Steven Rostedt @ 2011-07-26 1:10 UTC (permalink / raw)
To: Ian Lance Taylor
Cc: Arnaud Lacombe, gcc-help, stufever, linux-kernel, Wang Shaoyan,
Frederic Weisbecker, Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-26 1:10 ` Steven Rostedt
@ 2011-07-26 5:55 ` Ian Lance Taylor
0 siblings, 0 replies; 26+ messages in thread
From: Ian Lance Taylor @ 2011-07-26 5:55 UTC (permalink / raw)
To: Steven Rostedt
Cc: Arnaud Lacombe, gcc-help, stufever, linux-kernel, Wang Shaoyan,
Frederic Weisbecker, Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-25 18:32 ` Steven Rostedt
2011-07-25 19:43 ` Arnaud Lacombe
@ 2011-07-26 12:00 ` Paulo Marques
2011-07-26 13:18 ` Jesper Juhl
1 sibling, 1 reply; 26+ messages in thread
From: Paulo Marques @ 2011-07-26 12:00 UTC (permalink / raw)
To: Steven Rostedt
Cc: stufever, linux-kernel, Wang Shaoyan, Frederic Weisbecker,
Ingo Molnar
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."
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-26 12:00 ` Paulo Marques
@ 2011-07-26 13:18 ` Jesper Juhl
2011-07-26 13:32 ` Paulo Marques
0 siblings, 1 reply; 26+ messages in thread
From: Jesper Juhl @ 2011-07-26 13:18 UTC (permalink / raw)
To: Paulo Marques
Cc: Steven Rostedt, stufever, linux-kernel, Wang Shaoyan,
Frederic Weisbecker, Ingo Molnar
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.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-26 13:18 ` Jesper Juhl
@ 2011-07-26 13:32 ` Paulo Marques
2011-07-26 13:55 ` Steven Rostedt
0 siblings, 1 reply; 26+ messages in thread
From: Paulo Marques @ 2011-07-26 13:32 UTC (permalink / raw)
To: Jesper Juhl
Cc: Steven Rostedt, stufever, linux-kernel, Wang Shaoyan,
Frederic Weisbecker, Ingo Molnar
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."
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] TRACING: Fix a copmile warning
2011-07-26 13:32 ` Paulo Marques
@ 2011-07-26 13:55 ` Steven Rostedt
0 siblings, 0 replies; 26+ messages in thread
From: Steven Rostedt @ 2011-07-26 13:55 UTC (permalink / raw)
To: Paulo Marques
Cc: Jesper Juhl, stufever, linux-kernel, Wang Shaoyan,
Frederic Weisbecker, Ingo Molnar
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] TRACING: Fix a copmile warning
@ 2011-07-18 9:35 stufever
0 siblings, 0 replies; 26+ messages in thread
From: stufever @ 2011-07-18 9:35 UTC (permalink / raw)
To: linux-kernel
Cc: Wang Shaoyan, Steven Rostedt, Frederic Weisbecker, Ingo Molnar,
Wang Shaoyan
[-- 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
end of thread, other threads:[~2011-07-26 20:44 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-18 9:40 [PATCH] TRACING: Fix a copmile warning stufever
2011-07-25 18:32 ` Steven Rostedt
2011-07-25 19:43 ` Arnaud Lacombe
2011-07-25 20:19 ` Steven Rostedt
2011-07-25 20:28 ` Arnaud Lacombe
2011-07-25 22:38 ` Arnaud Lacombe
2011-07-25 23:49 ` Steven Rostedt
2011-07-25 23:52 ` Arnaud Lacombe
2011-07-26 0:14 ` Steven Rostedt
2011-07-25 23:50 ` Arnaud Lacombe
2011-07-25 23:58 ` Arnaud Lacombe
2011-07-26 0:08 ` Steven Rostedt
2011-07-26 0:35 ` Steven Rostedt
2011-07-26 0:44 ` Ian Lance Taylor
2011-07-26 0:41 ` Ian Lance Taylor
2011-07-26 1:08 ` Arnaud Lacombe
2011-07-26 1:12 ` Steven Rostedt
2011-07-26 1:19 ` Arnaud Lacombe
2011-07-26 20:43 ` Jeff Law
2011-07-26 1:10 ` Steven Rostedt
2011-07-26 5:55 ` Ian Lance Taylor
2011-07-26 12:00 ` Paulo Marques
2011-07-26 13:18 ` Jesper Juhl
2011-07-26 13:32 ` Paulo Marques
2011-07-26 13:55 ` Steven Rostedt
-- strict thread matches above, loose matches on Subject: below --
2011-07-18 9:35 stufever
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox