LinuxLists.cc - ftrace breaks sparc64 build

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Mon, 5 Jan 2009, Sam Ravnborg wrote:

> With an allmodconfig build on sparc and sparc64 I started
> to see warnings that become propagated to errors by -Werror.
>
> Example:
> CC arch/sparc/kernel/ldc.o
> arch/sparc/kernel/ldc.c: In function `process_control_frame':
> arch/sparc/kernel/ldc.c:627: warning: 'vap' might be used uninitialized in this function
>
>
> The code in question looks like this:
> static int process_ver_nack(struct ldc_channel *lp, struct ldc_version *vp)
> {
> struct ldc_version *vap;
>
> if ((vp->major == 0 && vp->minor == 0) ||
> !(vap = find_by_major(vp->major))) {
> return ldc_abort(lp);
> } else {
> struct ldc_packet *p;
> unsigned long new_tail;
>
> p = handshake_compose_ctrl(lp, LDC_INFO, LDC_VERS,
> vap, sizeof(*vap),
> &new_tail);
> if (p)
> return send_tx_packet(lp, p, new_tail);
> else
> return ldc_abort(lp);
> }
> }
>
> The else part will never be executed whitout assigning vap,
> and this code do not emit warnings in the normal case.
> [I am well aware that we recommend to move the assignment
> out of the if () - but this code worked as is before].
>
> This code gets expanded to:
>
> static int process_ver_nack(struct ldc_channel *lp, struct ldc_version *vp)
> {
> struct ldc_version *vap;
>
> if (__builtin_constant_p(((vp->major == 0 && vp->minor == 0) || !(vap = find_by_major(vp->major)))) ?
> !!((vp->major == 0 && vp->minor == 0) || !(vap = find_by_major(vp->major))) :
> ({
> int ______r;
> static struct ftrace_branch_data ______f = { .func = __func__, .file = "arch/sparc/kernel/ldc.c", .line = 630, };
> ______r = !!((vp->major == 0 && vp->minor == 0) || !(vap = find_by_major(vp->major)));
> if (______r)
> ______f.hit++;
> else
> ______f.miss++; ______r;
> })) {
>
> return ldc_abort(lp);
> } else {
> struct ldc_packet *p;
> unsigned long new_tail;
>
> p = handshake_compose_ctrl(lp, 0x01, 0x01, vap, sizeof(*vap), &new_tail);
> if (__builtin_constant_p((p)) ? !!(p) : ({
> int ______r;
> static struct ftrace_branch_data ______f = { .func = __func__, .file = "arch/sparc/kernel/ldc.c", .line = 639, };
> ______r = !!(p);
> if (______r)
> ______f.hit++;
> else ______f.miss++;
> ______r;
> }))
> return send_tx_packet(lp, p, new_tail);
> else
> return ldc_abort(lp);
> }
> }
> I have inserted newlines + tabs and removed a few __attribute__()
> to keep line lengths to a sensible level.
>
> My head started to spin with a dangerous speed trying to figure out
> the code snippet above.

My head spun a little by figuring out that vap is initialized in
the original code.

Honestly, that code is a little obfuscated, and would be better to write
it as:

if (vp->major == 0 && vp->minor=0)
return ldc_abort(lp);

vap = find_by_major(vp->major);
if (!vap)
return ldc_abort(lp);

[...]

This is much easier to read and we can remove the else statement
altogether. And I bet the warning will go away if we did it this way.

-- Steve

>
> On top of this some inlining occurs which is why gcc point at another
> function name.
>
>
> This is with following gcc version:
>
> sparc64-unknown-linux-gnu-gcc (GCC) 3.4.5
> Copyright (C) 2004 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>
> Build using crosstool.
>
> Is this a known issue?
>
> Any recommendations?
>
> Sam
>
>

2009-01-05 19:43:18

[permalink] [raw]

Subject: [PATCH] sparc: make proces_ver_nack a bit more readable

Impact: clean up

The code in process_ver_nack is a little obfuscated. This change
makes it a bit more readable by humans. It removes the complex
if statement and replaces it with a cleaner flow of control.

Signed-off-by: Steven Rostedt <[email protected]>

diff --git a/arch/sparc/kernel/ldc.c b/arch/sparc/kernel/ldc.c
index d689823..6ce5d25 100644
--- a/arch/sparc/kernel/ldc.c
+++ b/arch/sparc/kernel/ldc.c
@@ -625,22 +625,23 @@ static int process_ver_ack(struct ldc_channel *lp, struct ldc_version *vp)
static int process_ver_nack(struct ldc_channel *lp, struct ldc_version *vp)
{
struct ldc_version *vap;
+ struct ldc_packet *p;
+ unsigned long new_tail;

- if ((vp->major == 0 && vp->minor == 0) ||
- !(vap = find_by_major(vp->major))) {
+ if (vp->major == 0 && vp->minor == 0)
+ return ldc_abort(lp);
+
+ vap = find_by_major(vp->major);
+ if (!vap)
return ldc_abort(lp);
- } else {
- struct ldc_packet *p;
- unsigned long new_tail;

- p = handshake_compose_ctrl(lp, LDC_INFO, LDC_VERS,
+ p = handshake_compose_ctrl(lp, LDC_INFO, LDC_VERS,
vap, sizeof(*vap),
&new_tail);
- if (p)
- return send_tx_packet(lp, p, new_tail);
- else
- return ldc_abort(lp);
- }
+ if (!p)
+ return ldc_abort(lp);
+
+ return send_tx_packet(lp, p, new_tail);
}

static int process_version(struct ldc_channel *lp,

2009-01-05 19:46:35

[permalink] [raw]

Subject: Re: [PATCH] sparc: make proces_ver_nack a bit more readable

On Mon, 5 Jan 2009, Steven Rostedt wrote:

>
> Impact: clean up
>
> The code in process_ver_nack is a little obfuscated. This change
> makes it a bit more readable by humans. It removes the complex
> if statement and replaces it with a cleaner flow of control.

Note, I do not have a sparc compiler at my disposal, so I was not
able to compile (or boot) test this change.

Here's the original code:

static int process_ver_nack(struct ldc_channel *lp, struct ldc_version
*vp)
{
struct ldc_version *vap;

if ((vp->major == 0 && vp->minor == 0) ||
!(vap = find_by_major(vp->major))) {
return ldc_abort(lp);
} else {
struct ldc_packet *p;
unsigned long new_tail;

p = handshake_compose_ctrl(lp, LDC_INFO, LDC_VERS,
vap, sizeof(*vap),
&new_tail);
if (p)
return send_tx_packet(lp, p, new_tail);
else
return ldc_abort(lp);
}
}

And the new code:

static int process_ver_nack(struct ldc_channel *lp, struct ldc_version
*vp)
{
struct ldc_version *vap;
struct ldc_packet *p;
unsigned long new_tail;

if (vp->major == 0 && vp->minor == 0)
return ldc_abort(lp);

vap = find_by_major(vp->major);
if (!vap)
return ldc_abort(lp);

p = handshake_compose_ctrl(lp, LDC_INFO, LDC_VERS,
vap, sizeof(*vap),
&new_tail);
if (!p)
return ldc_abort(lp);

return send_tx_packet(lp, p, new_tail);
}

This should be binary the same. But as I said, I do not have a sparc
compiler to test.

Thanks,

-- Steve

2009-01-05 19:48:55

by Al Viro

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Mon, Jan 05, 2009 at 07:19:22PM +0100, Sam Ravnborg wrote:
> With an allmodconfig build on sparc and sparc64 I started
> to see warnings that become propagated to errors by -Werror.

While we are at it, sparc32 allmodconfig is broken by
commit 9bb482476c6c9d1ae033306440c51ceac93ea80c
Author: Jan Beulich <[email protected]>
Date: Tue Dec 16 11:30:08 2008 +0000

allow stripping of generated symbols under CONFIG_KALLSYMS_ALL

Results in
sparc-linux-objcopy: not stripping symbol `__ksymtab_strings' because it is named in a relocation
sparc-linux-objcopy: not stripping symbol `__crc_per_cpu__softirq_work_list' because it is named in a relocation
sparc-linux-objcopy: not stripping symbol `__initcall_end' because it is named in a relocation
sparc-linux-objcopy: not stripping symbol `__setup_end' because it is named in a relocation
sparc-linux-objcopy: not stripping symbol `__setup_start' because it is named in a relocation
sparc-linux-objcopy: not stripping symbol `__initcall_start' because it is named in a relocation
make: *** [.tmp_vmlinux1.stripped] Error 1

2009-01-05 19:52:52

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

Hi Steven.

>
> Honestly, that code is a little obfuscated, and would be better to write
> it as:
>
> if (vp->major == 0 && vp->minor=0)
> return ldc_abort(lp);
>
> vap = find_by_major(vp->major);
> if (!vap)
> return ldc_abort(lp);
>
> [...]
>
> This is much easier to read and we can remove the else statement
> altogether. And I bet the warning will go away if we did it this way.

Fully ageed on the readability.
I happen to trigger this as an error in the sparc code.
But I see the same warning also in generic code.

>From kernel/module.c:
/* Suck in entire file: we'll want most of it. */
/* vmalloc barfs on "unusual" numbers. Check here */
if (len > 64 * 1024 * 1024 || (hdr = vmalloc(len)) == NULL)
return ERR_PTR(-ENOMEM);

This gives following warning:
kernel/module.c: In function `load_module':
kernel/module.c:1842: warning: 'hdr' might be used uninitialized in this function

So this is not a pattern we seen only in sparc code and I wonder if this is
the first time it is brought up?

I can fix up the cases in sparc - no problem.
But it was a suprise to me _why_ these warnings started to creep
up and then it break my build.

Sam

2009-01-05 19:53:56

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

Added Jan.

And yes - I see the same but with a slightly different error message.

Sam

On Mon, Jan 05, 2009 at 07:48:44PM +0000, Al Viro wrote:
> On Mon, Jan 05, 2009 at 07:19:22PM +0100, Sam Ravnborg wrote:
> > With an allmodconfig build on sparc and sparc64 I started
> > to see warnings that become propagated to errors by -Werror.
>
> While we are at it, sparc32 allmodconfig is broken by
> commit 9bb482476c6c9d1ae033306440c51ceac93ea80c
> Author: Jan Beulich <[email protected]>
> Date: Tue Dec 16 11:30:08 2008 +0000
>
> allow stripping of generated symbols under CONFIG_KALLSYMS_ALL
>
> Results in
> sparc-linux-objcopy: not stripping symbol `__ksymtab_strings' because it is named in a relocation
> sparc-linux-objcopy: not stripping symbol `__crc_per_cpu__softirq_work_list' because it is named in a relocation
> sparc-linux-objcopy: not stripping symbol `__initcall_end' because it is named in a relocation
> sparc-linux-objcopy: not stripping symbol `__setup_end' because it is named in a relocation
> sparc-linux-objcopy: not stripping symbol `__setup_start' because it is named in a relocation
> sparc-linux-objcopy: not stripping symbol `__initcall_start' because it is named in a relocation
> make: *** [.tmp_vmlinux1.stripped] Error 1

2009-01-05 19:57:17

[permalink] [raw]

Subject: Re: [PATCH] sparc: make proces_ver_nack a bit more readable

On Mon, 5 Jan 2009, Steven Rostedt wrote:

>
> Impact: clean up
>
> The code in process_ver_nack is a little obfuscated. This change
> makes it a bit more readable by humans. It removes the complex
> if statement and replaces it with a cleaner flow of control.
>
> Signed-off-by: Steven Rostedt <[email protected]>

Sam,

Can you test to see if this patch makes the issue go away. Obviously, Dave
would need to be the one to pull it in, or at least ack it.

Thanks,

-- Steve

2009-01-05 20:05:37

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Mon, 5 Jan 2009, Sam Ravnborg wrote:

> Hi Steven.
>
> >
> > Honestly, that code is a little obfuscated, and would be better to write
> > it as:
> >
> > if (vp->major == 0 && vp->minor=0)
> > return ldc_abort(lp);
> >
> > vap = find_by_major(vp->major);
> > if (!vap)
> > return ldc_abort(lp);
> >
> > [...]
> >
> > This is much easier to read and we can remove the else statement
> > altogether. And I bet the warning will go away if we did it this way.
>
> Fully ageed on the readability.
> I happen to trigger this as an error in the sparc code.
> But I see the same warning also in generic code.
>
> >From kernel/module.c:
> /* Suck in entire file: we'll want most of it. */
> /* vmalloc barfs on "unusual" numbers. Check here */
> if (len > 64 * 1024 * 1024 || (hdr = vmalloc(len)) == NULL)
> return ERR_PTR(-ENOMEM);
>
>
> This gives following warning:
> kernel/module.c: In function `load_module':
> kernel/module.c:1842: warning: 'hdr' might be used uninitialized in this function

Probably the same issue. The problem is that the first use of a variable
is in the OR section of an if statement that does a return.

if (x || !(y = init_me())
return;

use_me(y);

IMHO I find this sloppy code. When reading the code it can cause reviewers
trouble, and wasted time, to see that y is indeed initialized. I'm
impressed that gcc was able to figure it out.

>
> So this is not a pattern we seen only in sparc code and I wonder if this is
> the first time it is brought up?
>
> I can fix up the cases in sparc - no problem.
> But it was a suprise to me _why_ these warnings started to creep
> up and then it break my build.

Have you always been compiling with -Werror?

The reason that gcc complains is because you have the "branch_tracer" on
that converts 'if ()' into a macro (as you saw in your -E compile). This
makes the if statement more complex, and goes beyond gcc's ability to know
that the above 'y' is initialized properly. I would work on fixing this in
the branch tracer, but honestly, I'm kind of glad that gcc barfs on it.
This will help us point out this kind of sloppy initializations (sorry if
I'm offending anybody about calling it sloppy). I just believe that it
makes the code a bit more obfuscated to initialize in an if statement, and
a second part of a complex if statement at that!

-- Steve

2009-01-05 20:06:28

[permalink] [raw]

Subject: Re: [PATCH] sparc: make proces_ver_nack a bit more readable

On Mon, Jan 05, 2009 at 02:56:58PM -0500, Steven Rostedt wrote:
>
> On Mon, 5 Jan 2009, Steven Rostedt wrote:
>
> >
> > Impact: clean up
> >
> > The code in process_ver_nack is a little obfuscated. This change
> > makes it a bit more readable by humans. It removes the complex
> > if statement and replaces it with a cleaner flow of control.
> >
> > Signed-off-by: Steven Rostedt <[email protected]>
>
> Sam,
>
> Can you test to see if this patch makes the issue go away. Obviously, Dave
> would need to be the one to pull it in, or at least ack it.

As expected the patch silence the warning.
The warning only triggers when we have an assignment
after a boolean || inside an if condition.

Sam

2009-01-05 20:07:24

[permalink] [raw]

Subject: Re: [PATCH] sparc: make proces_ver_nack a bit more readable

On Mon, Jan 05, 2009 at 02:42:59PM -0500, Steven Rostedt wrote:
>
> Impact: clean up
>
> The code in process_ver_nack is a little obfuscated. This change
> makes it a bit more readable by humans. It removes the complex
> if statement and replaces it with a cleaner flow of control.
>
> Signed-off-by: Steven Rostedt <[email protected]>
Reviewed-by: Sam Ravnborg <[email protected]>

And I can confirm the warning has dismissed with this patch.

Sam

2009-01-05 20:30:42

[permalink] [raw]

Subject: [PATCH] module: clean up initialization of variable

Impact: clean up

On Mon, 5 Jan 2009, Sam Ravnborg wrote:
>
> Fully ageed on the readability.
> I happen to trigger this as an error in the sparc code.
> But I see the same warning also in generic code.
>
> >From kernel/module.c:
> /* Suck in entire file: we'll want most of it. */
> /* vmalloc barfs on "unusual" numbers. Check here */
> if (len > 64 * 1024 * 1024 || (hdr = vmalloc(len)) == NULL)
> return ERR_PTR(-ENOMEM);
>
>
> This gives following warning:
> kernel/module.c: In function `load_module':
> kernel/module.c:1842: warning: 'hdr' might be used uninitialized in this function

The above is caused by having the branch tracer on and the following type
of initialization:

if (x || !(y = init_me())
return;

use_me(y);

This is sloppy initialization because it initializes, not only in an
if condition, but also as the second part of a complex conditional.

This patch makes the code a bit easier to read.

Signed-off-by: Steven Rostedt <[email protected]>
Reported-by: Sam Ravnborg <[email protected]>

diff --git a/kernel/module.c b/kernel/module.c
index 9712c52..112d6cd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1864,7 +1864,10 @@ static noinline struct module *load_module(void __user *umod,

/* Suck in entire file: we'll want most of it. */
/* vmalloc barfs on "unusual" numbers. Check here */
- if (len > 64 * 1024 * 1024 || (hdr = vmalloc(len)) == NULL)
+ if (len > 64 * 1024 * 1024)
+ return ERR_PTR(-ENOMEM);
+ hdr = vmalloc(len);
+ if (hdr == NULL)
return ERR_PTR(-ENOMEM);
if (copy_from_user(hdr, umod, len) != 0) {
err = -EFAULT;

2009-01-05 21:29:43

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

>
> Probably the same issue. The problem is that the first use of a variable
> is in the OR section of an if statement that does a return.
>
> if (x || !(y = init_me())
> return;
>
> use_me(y);
>

More warnings:
fs/partitions/check.c: In function `rescan_partitions':
fs/partitions/check.c:511: warning: 'state' might be used uninitialized in this function

=> follow same pattern

fs/compat_ioctl.c: In function `ppp_sock_fprog_ioctl_trans':
fs/compat_ioctl.c:859: warning: 'fptr32' might be used uninitialized in this function
fs/compat_ioctl.c: In function `do_fontx_ioctl':
fs/compat_ioctl.c:1082: warning: 'data' might be used uninitialized in this function
fs/compat_ioctl.c: In function `do_atmif_sioc':
fs/compat_ioctl.c:1302: warning: 'data' might be used uninitialized in this function
fs/compat_ioctl.c: In function `do_atm_ioctl':
fs/compat_ioctl.c:1272: warning: 'data' might be used uninitialized in this function
fs/compat_ioctl.c: In function `mtd_rw_oob':
fs/compat_ioctl.c:1421: warning: 'data' might be used uninitialized in this function
fs/compat_ioctl.c: In function `do_usbdevfs_discsignal':
fs/compat_ioctl.c:1650: warning: 'uctx' might be used uninitialized in this function

This is:
if (get_user(flen, &u_fprog32->len) ||
get_user(fptr32, &u_fprog32->filter))
return -EFAULT;

which is a common pattern. I have at least one in sparc64 code too.
I did not come up with a nice solution on this one.

drivers/char/vt_ioctl.c: In function `vt_ioctl':
drivers/char/vt_ioctl.c:945: warning: 'cc' might be used uninitialized in this function

get_user() || get_user() again.

net/core/skbuff.c: In function `___pskb_trim':
net/core/skbuff.c:1041: warning: 'err' might be used uninitialized in this function

if (xxx && err = foo())

net/core/dev.c: In function `skb_gso_segment':
net/core/dev.c:1537: warning: 'err' might be used uninitialized in this function
net/core/dev.c: In function `netif_receive_skb':
net/core/dev.c:2075: warning: 'port' might be used uninitialized in this function

if (xx && err = ...)
if (xx || port = ...)

net/core/neighbour.c: In function `neigh_resolve_output':
net/core/neighbour.c:1188: warning: 'neigh' might be used uninitialized in this function
net/core/neighbour.c: In function `neigh_create':
net/core/neighbour.c:411: warning: 'error' might be used uninitialized in this function

if (xx || neigh = ...)

net/ipv4/ip_output.c: In function `ip_append_data':
net/ipv4/ip_output.c:1006: warning: 'left' might be used uninitialized in this function

net/ipv4/tcp.c: In function `tcp_sendpage':
net/ipv4/tcp.c:687: warning: 'copy' might be used uninitialized in this function
net/ipv4/tcp.c: In function `tcp_sendmsg':
net/ipv4/tcp.c:862: warning: 'copy' might be used uninitialized in this function
net/ipv4/tcp.c: In function `tcp_recvmsg':
net/ipv4/tcp.c:1598: warning: 'chunk' might be used uninitialized in this function

net/ipv4/tcp_ipv4.c: In function `listening_get_next':
net/ipv4/tcp_ipv4.c:1874: warning: 'node' might be used uninitialized in this function
net/ipv4/tcp_ipv4.c: In function `established_get_next':
net/ipv4/tcp_ipv4.c:2012: warning: 'node' might be used uninitialized in this function

net/ipv4/raw.c: In function `__raw_v4_lookup':
net/ipv4/raw.c:113: warning: 'node' might be used uninitialized in this function

net/ipv4/udp.c: In function `__udp4_lib_rcv':
net/ipv4/udp.c:325: warning: 'node' might be used uninitialized in this function
net/ipv4/udp.c:325: warning: 'node' might be used uninitialized in this function

net/ipv4/devinet.c: In function `inet_gifconf':
net/ipv4/devinet.c:821: warning: 'ifa' might be used uninitialized in this function

net/ipv4/ipmr.c: In function `ipmr_get_route':
net/ipv4/ipmr.c:1629: warning: 'vif' might be used uninitialized in this function

net/ipv4/syncookies.c: In function `cookie_v4_check':
net/ipv4/syncookies.c:263: warning: 'mss' might be used uninitialized in this function

I have two warnings in sparc code I will deal with.
One of them is the get_user() || get_user() pattern
so I dunno how to fix it atm.

The above is from a:
make ARCH=sparc64 allmodconfig
make ARCH=sparc64 vmlinux

I did not try to build modules...

Sam

2009-01-05 21:52:18

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Mon, 5 Jan 2009, Sam Ravnborg wrote:

> >
> > Probably the same issue. The problem is that the first use of a variable
> > is in the OR section of an if statement that does a return.
> >
> > if (x || !(y = init_me())
> > return;
> >
> > use_me(y);
> >
>
> More warnings:
> fs/partitions/check.c: In function `rescan_partitions':
> fs/partitions/check.c:511: warning: 'state' might be used uninitialized in this function
>
> => follow same pattern
>
> fs/compat_ioctl.c: In function `ppp_sock_fprog_ioctl_trans':
> fs/compat_ioctl.c:859: warning: 'fptr32' might be used uninitialized in this function
> fs/compat_ioctl.c: In function `do_fontx_ioctl':
> fs/compat_ioctl.c:1082: warning: 'data' might be used uninitialized in this function
> fs/compat_ioctl.c: In function `do_atmif_sioc':
> fs/compat_ioctl.c:1302: warning: 'data' might be used uninitialized in this function
> fs/compat_ioctl.c: In function `do_atm_ioctl':
> fs/compat_ioctl.c:1272: warning: 'data' might be used uninitialized in this function
> fs/compat_ioctl.c: In function `mtd_rw_oob':
> fs/compat_ioctl.c:1421: warning: 'data' might be used uninitialized in this function
> fs/compat_ioctl.c: In function `do_usbdevfs_discsignal':
> fs/compat_ioctl.c:1650: warning: 'uctx' might be used uninitialized in this function
>
> This is:
> if (get_user(flen, &u_fprog32->len) ||
> get_user(fptr32, &u_fprog32->filter))
> return -EFAULT;

Is this all sparc cross compiler? I'm trying to reproduce it on x86 with
no avail :-(

I would like to test other ways to change the macro, but to do so, I need
to get a compiler that will produce the warnings that you see. What
version of gcc are you using?

Thanks,

-- Steve

2009-01-05 22:00:32

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Mon, Jan 05, 2009 at 04:52:02PM -0500, Steven Rostedt wrote:
>
> On Mon, 5 Jan 2009, Sam Ravnborg wrote:
>
> > >
> > > Probably the same issue. The problem is that the first use of a variable
> > > is in the OR section of an if statement that does a return.
> > >
> > > if (x || !(y = init_me())
> > > return;
> > >
> > > use_me(y);
> > >
> >
> > More warnings:
> > fs/partitions/check.c: In function `rescan_partitions':
> > fs/partitions/check.c:511: warning: 'state' might be used uninitialized in this function
> >
> > => follow same pattern
> >
> > fs/compat_ioctl.c: In function `ppp_sock_fprog_ioctl_trans':
> > fs/compat_ioctl.c:859: warning: 'fptr32' might be used uninitialized in this function
> > fs/compat_ioctl.c: In function `do_fontx_ioctl':
> > fs/compat_ioctl.c:1082: warning: 'data' might be used uninitialized in this function
> > fs/compat_ioctl.c: In function `do_atmif_sioc':
> > fs/compat_ioctl.c:1302: warning: 'data' might be used uninitialized in this function
> > fs/compat_ioctl.c: In function `do_atm_ioctl':
> > fs/compat_ioctl.c:1272: warning: 'data' might be used uninitialized in this function
> > fs/compat_ioctl.c: In function `mtd_rw_oob':
> > fs/compat_ioctl.c:1421: warning: 'data' might be used uninitialized in this function
> > fs/compat_ioctl.c: In function `do_usbdevfs_discsignal':
> > fs/compat_ioctl.c:1650: warning: 'uctx' might be used uninitialized in this function
> >
> > This is:
> > if (get_user(flen, &u_fprog32->len) ||
> > get_user(fptr32, &u_fprog32->filter))
> > return -EFAULT;
>
> Is this all sparc cross compiler? I'm trying to reproduce it on x86 with
> no avail :-(
>
> I would like to test other ways to change the macro, but to do so, I need
> to get a compiler that will produce the warnings that you see. What
> version of gcc are you using?

I used crosstool to build a 3.4.5 gcc:
set -ex
TARBALLS_DIR=$HOME/downloads
RESULT_TOP=/opt/crosstool
export TARBALLS_DIR RESULT_TOP
GCC_LANGUAGES="c"
export GCC_LANGUAGES

eval `cat sparc64.dat gcc-3.4.5-glibc-2.3.6-tls.dat` sh all.sh --notest

It is running on a 32 bit target.
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 8
model name : Pentium III (Coppermine)
stepping : 10
cpu MHz : 851.970
cache size : 256 KB
fdiv_bug : no
hlt_bug : no
f00f_bug : no
coma_bug : no
fpu : yes
fpu_exception : yes
cpuid level : 2
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 mtrr pge mca cmov pat pse36 mmx fxsr sse
bogomips : 1704.50
clflush size : 32

Sam

2009-01-05 22:14:33

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Mon, 5 Jan 2009, Sam Ravnborg wrote:

> > Is this all sparc cross compiler? I'm trying to reproduce it on x86 with
> > no avail :-(
> >
> > I would like to test other ways to change the macro, but to do so, I need
> > to get a compiler that will produce the warnings that you see. What
> > version of gcc are you using?
>
> I used crosstool to build a 3.4.5 gcc:

Hmm, that's a pretty old compiler. I wonder if it wouldn't just help
if we just make the branch tracer dependent on the compiler used. That is.

#if defined(CONFIG_PROFILE_ALL_BRANCHES) && (__GNUC__ >= 4)
#define if(cond) ...

Or something :-/

-- Steve

2009-01-05 23:00:01

by Harvey Harrison

[permalink] [raw]

Subject: Re: [PATCH] module: clean up initialization of variable

On Mon, 2009-01-05 at 15:30 -0500, Steven Rostedt wrote:
> + hdr = vmalloc(len);
> + if (hdr == NULL)

A bit pedantic but,

if (!hdr) perhaps?

Cheers,

Harvey

2009-01-05 23:11:50

by Heiko Carstens

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Mon, Jan 05, 2009 at 05:14:22PM -0500, Steven Rostedt wrote:
>
> On Mon, 5 Jan 2009, Sam Ravnborg wrote:
>
> > > Is this all sparc cross compiler? I'm trying to reproduce it on x86 with
> > > no avail :-(
> > >
> > > I would like to test other ways to change the macro, but to do so, I need
> > > to get a compiler that will produce the warnings that you see. What
> > > version of gcc are you using?
> >
> > I used crosstool to build a 3.4.5 gcc:
>
> Hmm, that's a pretty old compiler. I wonder if it wouldn't just help
> if we just make the branch tracer dependent on the compiler used. That is.
>
> #if defined(CONFIG_PROFILE_ALL_BRANCHES) && (__GNUC__ >= 4)
> #define if(cond) ...
>
> Or something :-/

FWIW, on s390 with gcc 4.3.2 and an allyesconfig I get these:

CC arch/s390/mm/extmem.o
arch/s390/mm/extmem.c: In function 'segment_modify_shared':
arch/s390/mm/extmem.c:572: warning: 'end_addr' may be used uninitialized in this function
arch/s390/mm/extmem.c:572: warning: 'start_addr' may be used uninitialized in this function
arch/s390/mm/extmem.c: In function 'query_segment_type':
arch/s390/mm/extmem.c:259: warning: 'vmrc' may be used uninitialized in this function

Switching off PROFILE_ALL_BRANCHES makes the warnings go away again.

2009-01-06 01:23:16

by Rusty Russell

[permalink] [raw]

Subject: Re: [PATCH] module: clean up initialization of variable

On Tuesday 06 January 2009 07:00:25 Steven Rostedt wrote:
> This is sloppy initialization because it initializes, not only in an
> if condition, but also as the second part of a complex conditional.
>
> This patch makes the code a bit easier to read.
...
> /* Suck in entire file: we'll want most of it. */
> /* vmalloc barfs on "unusual" numbers. Check here */
> - if (len > 64 * 1024 * 1024 || (hdr = vmalloc(len)) == NULL)
> + if (len > 64 * 1024 * 1024)
> + return ERR_PTR(-ENOMEM);
> + hdr = vmalloc(len);
> + if (hdr == NULL)
> return ERR_PTR(-ENOMEM);
> if (copy_from_user(hdr, umod, len) != 0) {
> err = -EFAULT;

This line is not accidental nor casually written: the two statements are deliberately entwined. It is a succint complaint against the vagaries of vmalloc.

So this patch is a messup, not a cleanup.

But it's really upset me because it is lazy and timid: and too much kernel code is becoming mired in such scars. Instead of "how do I kill this warning and get it in the merge window" you should be thinking "how do I make the kernel better", and "I wonder if vmalloc still has this problem"...

And I so look forward to the warm fuzzies I get when applying a real cleanup patch.

Thanks,
Rusty.

2009-01-06 02:02:22

[permalink] [raw]

Subject: Re: [PATCH] module: clean up initialization of variable

On Tue, 6 Jan 2009, Rusty Russell wrote:

> On Tuesday 06 January 2009 07:00:25 Steven Rostedt wrote:
> > This is sloppy initialization because it initializes, not only in an
> > if condition, but also as the second part of a complex conditional.
> >
> > This patch makes the code a bit easier to read.
> ...
> > /* Suck in entire file: we'll want most of it. */
> > /* vmalloc barfs on "unusual" numbers. Check here */
> > - if (len > 64 * 1024 * 1024 || (hdr = vmalloc(len)) == NULL)
> > + if (len > 64 * 1024 * 1024)
> > + return ERR_PTR(-ENOMEM);
> > + hdr = vmalloc(len);
> > + if (hdr == NULL)
> > return ERR_PTR(-ENOMEM);
> > if (copy_from_user(hdr, umod, len) != 0) {
> > err = -EFAULT;
>
> This line is not accidental nor casually written: the two statements
> are deliberately entwined. It is a succint complaint against the
> vagaries of vmalloc.
>
> So this patch is a messup, not a cleanup.

It is not that much of a messup. I did not realize that the code was
a political protest against the horrors of vmalloc ;-)

>
> But it's really upset me because it is lazy and timid: and too much
> kernel code is becoming mired in such scars. Instead of "how do I kill
> this warning and get it in the merge window" you should be thinking "how
> do I make the kernel better", and "I wonder if vmalloc still has this
> problem"...
>
> And I so look forward to the warm fuzzies I get when applying a real
> cleanup patch.

Well, I'm not about to go solve the vmalloc issues (not today anyway). But
I'll go and see if I can get the branch tracer macro to work on all
versions of gcc.

Thanks,

-- Steve

2009-01-06 02:07:46

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Tue, 6 Jan 2009, Heiko Carstens wrote:

> On Mon, Jan 05, 2009 at 05:14:22PM -0500, Steven Rostedt wrote:
> >
> > On Mon, 5 Jan 2009, Sam Ravnborg wrote:
> >
> > > > Is this all sparc cross compiler? I'm trying to reproduce it on x86 with
> > > > no avail :-(
> > > >
> > > > I would like to test other ways to change the macro, but to do so, I need
> > > > to get a compiler that will produce the warnings that you see. What
> > > > version of gcc are you using?
> > >
> > > I used crosstool to build a 3.4.5 gcc:
> >
> > Hmm, that's a pretty old compiler. I wonder if it wouldn't just help
> > if we just make the branch tracer dependent on the compiler used. That is.
> >
> > #if defined(CONFIG_PROFILE_ALL_BRANCHES) && (__GNUC__ >= 4)
> > #define if(cond) ...
> >
> > Or something :-/
>
> FWIW, on s390 with gcc 4.3.2 and an allyesconfig I get these:
>
> CC arch/s390/mm/extmem.o
> arch/s390/mm/extmem.c: In function 'segment_modify_shared':
> arch/s390/mm/extmem.c:572: warning: 'end_addr' may be used uninitialized in this function
> arch/s390/mm/extmem.c:572: warning: 'start_addr' may be used uninitialized in this function
> arch/s390/mm/extmem.c: In function 'query_segment_type':
> arch/s390/mm/extmem.c:259: warning: 'vmrc' may be used uninitialized in this function
>
> Switching off PROFILE_ALL_BRANCHES makes the warnings go away again.

Now that is really interesting. Because end_addr and start_addr are
initialized via functions:

if (x)
init_me(a, &y);
else
init_me(b, &y);

Which actually does not make sense why turning off PROFILE_ALL_BRANCHES
would affect this :-/

-- Steve

2009-01-06 04:31:18

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Tue, 6 Jan 2009, Heiko Carstens wrote:

Sam and Heiko,

I'm trying to see if the (a ? b : c) construct is causing the issue. Can
you test this patch.

Thanks,

-- Steve

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index d95da10..e13ad24 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -113,7 +113,8 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
* "Define 'is'", Bill Clinton
* "Define 'if'", Steven Rostedt
*/
-#define if(cond) if (__builtin_constant_p((cond)) ? !!(cond) : \
+#define if(cond) if ((__builtin_constant_p((cond)) && !!(cond)) || \
+ (!__builtin_constant_p((cond)) && \
({ \
int ______r; \
static struct ftrace_branch_data \
@@ -130,7 +131,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
else \
______f.miss++; \
______r; \
- }))
+ })))
#endif /* CONFIG_PROFILE_ALL_BRANCHES */

#else

2009-01-06 07:52:48

by Jan Beulich

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

>On Mon, Jan 05, 2009 at 07:48:44PM +0000, Al Viro wrote:
>> On Mon, Jan 05, 2009 at 07:19:22PM +0100, Sam Ravnborg wrote:
>> > With an allmodconfig build on sparc and sparc64 I started
>> > to see warnings that become propagated to errors by -Werror.
>>
>> While we are at it, sparc32 allmodconfig is broken by
>> commit 9bb482476c6c9d1ae033306440c51ceac93ea80c
>> Author: Jan Beulich <[email protected]>
>> Date: Tue Dec 16 11:30:08 2008 +0000
>>
>> allow stripping of generated symbols under CONFIG_KALLSYMS_ALL
>>
>> Results in
>> sparc-linux-objcopy: not stripping symbol `__ksymtab_strings' because it is named in a relocation
>> sparc-linux-objcopy: not stripping symbol `__crc_per_cpu__softirq_work_list' because it is named in a relocation
>> sparc-linux-objcopy: not stripping symbol `__initcall_end' because it is named in a relocation
>> sparc-linux-objcopy: not stripping symbol `__setup_end' because it is named in a relocation
>> sparc-linux-objcopy: not stripping symbol `__setup_start' because it is named in a relocation
>> sparc-linux-objcopy: not stripping symbol `__initcall_start' because it is named in a relocation
>> make: *** [.tmp_vmlinux1.stripped] Error 1

The __crc_... reference is definitely bogus - none should survive with the
new .c->.o rule. Could you find out what object file they originate from?

The others look like a tools side behavioral difference, as I never saw any
such. Is this problem sparc32-specific (I tested x86 and ia64 only)? What's
the binutils version used?

Jan

2009-01-06 09:37:38

by Heiko Carstens

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Mon, Jan 05, 2009 at 09:07:05PM -0500, Steven Rostedt wrote:
> On Tue, 6 Jan 2009, Heiko Carstens wrote:
> > On Mon, Jan 05, 2009 at 05:14:22PM -0500, Steven Rostedt wrote:
> > > On Mon, 5 Jan 2009, Sam Ravnborg wrote:
> > >
> > > > > Is this all sparc cross compiler? I'm trying to reproduce it on x86 with
> > > > > no avail :-(
> > > > >
> > > > > I would like to test other ways to change the macro, but to do so, I need
> > > > > to get a compiler that will produce the warnings that you see. What
> > > > > version of gcc are you using?
> > > >
> > > > I used crosstool to build a 3.4.5 gcc:
> > >
> > > Hmm, that's a pretty old compiler. I wonder if it wouldn't just help
> > > if we just make the branch tracer dependent on the compiler used. That is.
> > >
> > > #if defined(CONFIG_PROFILE_ALL_BRANCHES) && (__GNUC__ >= 4)
> > > #define if(cond) ...
> > >
> > > Or something :-/
> >
> > FWIW, on s390 with gcc 4.3.2 and an allyesconfig I get these:
> >
> > CC arch/s390/mm/extmem.o
> > arch/s390/mm/extmem.c: In function 'segment_modify_shared':
> > arch/s390/mm/extmem.c:572: warning: 'end_addr' may be used uninitialized in this function
> > arch/s390/mm/extmem.c:572: warning: 'start_addr' may be used uninitialized in this function
> > arch/s390/mm/extmem.c: In function 'query_segment_type':
> > arch/s390/mm/extmem.c:259: warning: 'vmrc' may be used uninitialized in this function
> >
> > Switching off PROFILE_ALL_BRANCHES makes the warnings go away again.
>
> Now that is really interesting. Because end_addr and start_addr are
> initialized via functions:
>
> if (x)
> init_me(a, &y);
> else
> init_me(b, &y);
>
> Which actually does not make sense why turning off PROFILE_ALL_BRANCHES
> would affect this :-/

They are not necessarily initialized via 'initme'. Only if it returns with
a return value >= 0. So it's a bit more complex:

if (x)
rc = init_me(a, &y);
else
rc = init_me(b, &y);
if (rc < 0)
/* y unitialized */
return;
use(y);

2009-01-06 09:45:50

by Heiko Carstens

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Mon, Jan 05, 2009 at 11:30:58PM -0500, Steven Rostedt wrote:
> On Tue, 6 Jan 2009, Heiko Carstens wrote:
> Sam and Heiko,
>
> I'm trying to see if the (a ? b : c) construct is causing the issue. Can
> you test this patch.
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index d95da10..e13ad24 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -113,7 +113,8 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
> * "Define 'is'", Bill Clinton
> * "Define 'if'", Steven Rostedt
> */
> -#define if(cond) if (__builtin_constant_p((cond)) ? !!(cond) : \
> +#define if(cond) if ((__builtin_constant_p((cond)) && !!(cond)) || \
> + (!__builtin_constant_p((cond)) && \

Doesn't help unfortunately.

2009-01-06 11:36:25

by Al Viro

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Tue, Jan 06, 2009 at 07:53:04AM +0000, Jan Beulich wrote:

> The __crc_... reference is definitely bogus - none should survive with the
> new .c->.o rule. Could you find out what object file they originate from?

So can you, by use of arcane tool known as "grep"... It's in kernel/softirq.c
and that's genksyms parser being fucked in head. Look for TYPEOF_KEYW
in parse.y and you'll see. Especially amusing part is a kludge from
commit a89a0a2354ae666612968e254d650bfd04f11eb6...

> The others look like a tools side behavioral difference, as I never saw any
> such. Is this problem sparc32-specific (I tested x86 and ia64 only)? What's
> the binutils version used?

2.18.50.0.6.

And no, it's not tools side. What it is, AFAICT, is that sparc32 has
LDFLAGS_vmlinux = -r, which leaves a metric arseload of relocs that
wouldn't have survived into vmlinux otherwise. Look at .rela__ksymtab
in .tmp_vmlinux1, for example...

2009-01-06 12:39:41

by Jan Beulich

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

>>> Al Viro <[email protected]> 06.01.09 12:35 >>>
>On Tue, Jan 06, 2009 at 07:53:04AM +0000, Jan Beulich wrote:
>> The others look like a tools side behavioral difference, as I never saw any
>> such. Is this problem sparc32-specific (I tested x86 and ia64 only)? What's
>> the binutils version used?
>
>2.18.50.0.6.
>
>And no, it's not tools side. What it is, AFAICT, is that sparc32 has
>LDFLAGS_vmlinux = -r, which leaves a metric arseload of relocs that
>wouldn't have survived into vmlinux otherwise. Look at .rela__ksymtab
>in .tmp_vmlinux1, for example...

That I think can only be taken care of by disallowing the stripping for that
arch, i.e. adding a negative dependency to the Kconfig option. Or can
anyone think of any other solution?

Jan

2009-01-06 13:33:20

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Tue, Jan 06, 2009 at 11:35:43AM +0000, Al Viro wrote:
> On Tue, Jan 06, 2009 at 07:53:04AM +0000, Jan Beulich wrote:
>
> > The __crc_... reference is definitely bogus - none should survive with the
> > new .c->.o rule. Could you find out what object file they originate from?
>
> So can you, by use of arcane tool known as "grep"... It's in kernel/softirq.c
> and that's genksyms parser being fucked in head. Look for TYPEOF_KEYW
> in parse.y and you'll see. Especially amusing part is a kludge from
> commit a89a0a2354ae666612968e254d650bfd04f11eb6...

Any feedback on what to do to make it better?
Never used typeof myself so I do not know the exact syntax.

>
> > The others look like a tools side behavioral difference, as I never saw any
> > such. Is this problem sparc32-specific (I tested x86 and ia64 only)? What's
> > the binutils version used?
>
> 2.18.50.0.6.
>
> And no, it's not tools side. What it is, AFAICT, is that sparc32 has
> LDFLAGS_vmlinux = -r, which leaves a metric arseload of relocs that
> wouldn't have survived into vmlinux otherwise. Look at .rela__ksymtab
> in .tmp_vmlinux1, for example...

The use of -r is to support the btfixup magic.
I have been thinking of moving the btfixup phase so it happens _before_
the final link of vmlinux. This should help us here.

But I never managed to really understand what the btfixup thing is all
about and has then been sidetracked by funnier stuff.

Sam

2009-01-06 15:52:31

by Al Viro

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Tue, Jan 06, 2009 at 02:34:42PM +0100, Sam Ravnborg wrote:
> On Tue, Jan 06, 2009 at 11:35:43AM +0000, Al Viro wrote:
> > On Tue, Jan 06, 2009 at 07:53:04AM +0000, Jan Beulich wrote:
> >
> > > The __crc_... reference is definitely bogus - none should survive with the
> > > new .c->.o rule. Could you find out what object file they originate from?
> >
> > So can you, by use of arcane tool known as "grep"... It's in kernel/softirq.c
> > and that's genksyms parser being fucked in head. Look for TYPEOF_KEYW
> > in parse.y and you'll see. Especially amusing part is a kludge from
> > commit a89a0a2354ae666612968e254d650bfd04f11eb6...
>
> Any feedback on what to do to make it better?
> Never used typeof myself so I do not know the exact syntax.

Same as that of sizeof. I.e. typeof(<type-name>) or typeof <unary-expression>.

2009-01-06 18:23:42

[permalink] [raw]

Subject: Re: [PATCH] sparc: make proces_ver_nack a bit more readable

From: Sam Ravnborg <[email protected]>
Date: Mon, 5 Jan 2009 21:08:48 +0100

> On Mon, Jan 05, 2009 at 02:42:59PM -0500, Steven Rostedt wrote:
> >
> > Impact: clean up
> >
> > The code in process_ver_nack is a little obfuscated. This change
> > makes it a bit more readable by humans. It removes the complex
> > if statement and replaces it with a cleaner flow of control.
> >
> > Signed-off-by: Steven Rostedt <[email protected]>
> Reviewed-by: Sam Ravnborg <[email protected]>
>
> And I can confirm the warning has dismissed with this patch.

Applied, thanks everyone.

2009-01-06 18:32:55

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

From: Steven Rostedt <[email protected]>
Date: Mon, 5 Jan 2009 15:05:11 -0500 (EST)

> Probably the same issue. The problem is that the first use of a variable
> is in the OR section of an if statement that does a return.
>
> if (x || !(y = init_me())
> return;
>
> use_me(y);
>
> IMHO I find this sloppy code. When reading the code it can cause reviewers
> trouble, and wasted time, to see that y is indeed initialized. I'm
> impressed that gcc was able to figure it out.

I'm pretty much in firm disagreement here.

This code is pretty clear. The only way to get to use_me() is
by initializing 'y'. It's very straightforward.

There are many conditionals in the kernel where this order
of evaluation and side effects is depended upon. Some of them
just happen to warn now because of the branch tracer.

> Have you always been compiling with -Werror?

arch/sparc*/ builds with -Werror for 5+ years.

> The reason that gcc complains is because you have the
> "branch_tracer" on that converts 'if ()' into a macro (as you saw in
> your -E compile). This makes the if statement more complex, and goes
> beyond gcc's ability to know that the above 'y' is initialized
> properly. I would work on fixing this in the branch tracer, but
> honestly, I'm kind of glad that gcc barfs on it. This will help us
> point out this kind of sloppy initializations (sorry if I'm
> offending anybody about calling it sloppy). I just believe that it
> makes the code a bit more obfuscated to initialize in an if
> statement, and a second part of a complex if statement at that!

Keep in mind all this code was fine and built warning free before the
if() obfuscation done by the branch tracer. If I wrote the branch
tracer, I'd probably search for these kinds of excuses too :-)

2009-01-06 18:39:54

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

From: Sam Ravnborg <[email protected]>
Date: Tue, 6 Jan 2009 14:34:42 +0100

> But I never managed to really understand what the btfixup thing is all
> about and has then been sidetracked by funnier stuff.

It's recording relocations that get fixed up at boot time.

The way it works is that each btfixup emits a reference to a symbol
that will be unresolved.

The btfixup tool under arch/sparc/boot/ scans the unlinked kernel
image, generates dummy symbol definitions into a foo.s file so that
the kernel can be linked, and builds the btfixup tables so the kernel
can patch up these relocations at boot time.

2009-01-06 18:52:56

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Tue, 6 Jan 2009, David Miller wrote:

> From: Steven Rostedt <[email protected]>
> Date: Mon, 5 Jan 2009 15:05:11 -0500 (EST)
>
> > Probably the same issue. The problem is that the first use of a variable
> > is in the OR section of an if statement that does a return.
> >
> > if (x || !(y = init_me())
> > return;
> >
> > use_me(y);
> >
> > IMHO I find this sloppy code. When reading the code it can cause reviewers
> > trouble, and wasted time, to see that y is indeed initialized. I'm
> > impressed that gcc was able to figure it out.
>
> I'm pretty much in firm disagreement here.

fair enough.

>
> This code is pretty clear. The only way to get to use_me() is
> by initializing 'y'. It's very straightforward.
>
> There are many conditionals in the kernel where this order
> of evaluation and side effects is depended upon. Some of them
> just happen to warn now because of the branch tracer.
>
> > Have you always been compiling with -Werror?
>
> arch/sparc*/ builds with -Werror for 5+ years.
>
> > The reason that gcc complains is because you have the
> > "branch_tracer" on that converts 'if ()' into a macro (as you saw in
> > your -E compile). This makes the if statement more complex, and goes
> > beyond gcc's ability to know that the above 'y' is initialized
> > properly. I would work on fixing this in the branch tracer, but
> > honestly, I'm kind of glad that gcc barfs on it. This will help us
> > point out this kind of sloppy initializations (sorry if I'm
> > offending anybody about calling it sloppy). I just believe that it
> > makes the code a bit more obfuscated to initialize in an if
> > statement, and a second part of a complex if statement at that!
>
> Keep in mind all this code was fine and built warning free before the
> if() obfuscation done by the branch tracer. If I wrote the branch
> tracer, I'd probably search for these kinds of excuses too :-)

Yeah, I'm trying different ways to handle the if magic to see if I can
come up with a way to keep gcc happy. Unfortunately, every solution so far
still seems to confuse sparc (I don't know why it does not confuse x86 or
powerpc, although some of my variations do).

I would hate to black list archs just because it gives warnings. I've
analyzed the code greatly to make sure that it keeps the semantics of the
original code the same.

It's just that it pushes gcc beyond its threshold of knowing that a
variable is initialized. I do not blame gcc because it has to have a limit
to handle logic cases (otherwise it has solved the halting problem).

What bothers me is that some versions of gcc are not affected by the if
macro and some are. I'm impressed to hear that sparc has been compiling
fine for years with -Werror since I'm not sure other archs can say the
same.

I'll have to keep hacking at it. :-/

-- Steve

2009-01-06 19:01:22

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

From: Steven Rostedt <[email protected]>
Date: Tue, 6 Jan 2009 13:52:36 -0500 (EST)

> I would hate to black list archs just because it gives warnings.

At the very least arch/sparc should build cleanly now because
I took your ldc.c change

2009-01-06 19:50:36

[permalink] [raw]

Subject: Re: ftrace breaks sparc64 build

On Tue, Jan 06, 2009 at 11:01:14AM -0800, David Miller wrote:
> From: Steven Rostedt <[email protected]>
> Date: Tue, 6 Jan 2009 13:52:36 -0500 (EST)
>
> > I would hate to black list archs just because it gives warnings.
>
> At the very least arch/sparc should build cleanly now because
> I took your ldc.c change

Unfortunately not.
With sparc64 I saw warnings in three additional files.
I cooked up the following to avoid the warnings.

The patch to unaligned_64.c is just an ugly hack.
The patch in viohs.c makes it easier to read IMO.
The patch to init_64.c is likewise a nice cleanup.

I will re-review and submit the latter two as proper patches.
I see no way to refactor unaligned_64.c so it keep
current behaviour and is equal readable.

Sam

diff --git a/arch/sparc/kernel/unaligned_64.c b/arch/sparc/kernel/unaligned_64.c
index 203ddfa..f005799 100644
--- a/arch/sparc/kernel/unaligned_64.c
+++ b/arch/sparc/kernel/unaligned_64.c
@@ -601,11 +601,13 @@ void handle_lddfmna(struct pt_regs *regs, unsigned long sfar, unsigned long sfsr
pc = (u32)pc;
if (get_user(insn, (u32 __user *) pc) != -EFAULT) {
int asi = decode_asi(insn, regs);
+ int rfirst, rsecond;
if ((asi > ASI_SNFL) ||
(asi < ASI_P))
goto daex;
- if (get_user(first, (u32 __user *)sfar) ||
- get_user(second, (u32 __user *)(sfar + 4))) {
+ rfirst = get_user(first, (u32 __user *)sfar);
+ rsecond = get_user(second, (u32 __user *)(sfar + 4));
+ if (rfirst || rsecond) {
if (asi & 0x2) /* NF */ {
first = 0; second = 0;
} else
diff --git a/arch/sparc/kernel/viohs.c b/arch/sparc/kernel/viohs.c
index 708fa17..aa6ac70 100644
--- a/arch/sparc/kernel/viohs.c
+++ b/arch/sparc/kernel/viohs.c
@@ -337,8 +337,10 @@ static int process_ver_nack(struct vio_driver_state *vio,
viodbg(HS, "GOT VERSION NACK maj[%u] min[%u] devclass[%u]\n",
pkt->major, pkt->minor, pkt->dev_class);

- if ((pkt->major == 0 && pkt->minor == 0) ||
- !(nver = find_by_major(vio, pkt->major)))
+ if (pkt->major == 0 && pkt->minor == 0)
+ return handshake_failure(vio);
+ nver = find_by_major(vio, pkt->major);
+ if (!nver)
return handshake_failure(vio);

if (send_version(vio, nver->major, nver->minor) < 0)
diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
index c04b111..26f59df 100644
--- a/arch/sparc/mm/init_64.c
+++ b/arch/sparc/mm/init_64.c
@@ -259,21 +259,15 @@ static inline void tsb_insert(struct tsb *ent, unsigned long tag, unsigned long
unsigned long _PAGE_ALL_SZ_BITS __read_mostly;
unsigned long _PAGE_SZBITS __read_mostly;

-void update_mmu_cache(struct vm_area_struct *vma, unsigned long address, pte_t pte)
+static void try_flush_dcache(unsigned long pfn)
{
- struct mm_struct *mm;
- struct tsb *tsb;
- unsigned long tag, flags;
- unsigned long tsb_index, tsb_hash_shift;
+ unsigned long pg_flags;
+ struct page *page;

- if (tlb_type != hypervisor) {
- unsigned long pfn = pte_pfn(pte);
- unsigned long pg_flags;
- struct page *page;
-
- if (pfn_valid(pfn) &&
- (page = pfn_to_page(pfn), page_mapping(page)) &&
- ((pg_flags = page->flags) & (1UL << PG_dcache_dirty))) {
+ page = pfn_to_page(pfn);
+ if (page && page_mapping(page)) {
+ pg_flags = page->flags;
+ if (pg_flags & (1UL << PG_dcache_dirty)) {
int cpu = ((pg_flags >> PG_dcache_cpu_shift) &
PG_dcache_cpu_mask);
int this_cpu = get_cpu();
@@ -291,6 +285,21 @@ void update_mmu_cache(struct vm_area_struct *vma, unsigned long address, pte_t p
put_cpu();
}
}
+}
+
+void update_mmu_cache(struct vm_area_struct *vma, unsigned long address, pte_t pte)
+{
+ struct mm_struct *mm;
+ struct tsb *tsb;
+ unsigned long tag, flags;
+ unsigned long tsb_index, tsb_hash_shift;
+
+ if (tlb_type != hypervisor) {
+ unsigned long pfn = pte_pfn(pte);
+
+ if (pfn_valid(pfn))
+ try_flush_dcache(pfn);
+ }

mm = vma->vm_mm;

2009-01-06 20:02:41