* [PATCH 4/4] futex: warning corrections @ 2011-07-04 23:21 Vitaliy Ivanov 2011-07-06 17:00 ` Darren Hart 0 siblings, 1 reply; 8+ messages in thread From: Vitaliy Ivanov @ 2011-07-04 23:21 UTC (permalink / raw) To: Thomas Gleixner, Linus Torvalds, Darren Hart; +Cc: lkml >From 3f7997d71fe2b5cb16e2913928f68023855d786d Mon Sep 17 00:00:00 2001 From: Vitaliy Ivanov <vitalivanov@gmail.com> Date: Tue, 5 Jul 2011 02:07:42 +0300 Subject: [PATCH 4/4] futex: warning corrections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit kernel/futex.c: In function ‘fixup_pi_state_owner.clone.17’: kernel/futex.c:1582:6: warning: ‘curval’ may be used uninitialized in this function kernel/futex.c: In function ‘handle_futex_death’: kernel/futex.c:2486:6: warning: ‘nval’ may be used uninitialized in this function kernel/futex.c: In function ‘do_futex’: kernel/futex.c:863:11: warning: ‘curval’ may be used uninitialized in this function kernel/futex.c:828:6: note: ‘curval’ was declared here kernel/futex.c:898:5: warning: ‘oldval’ may be used uninitialized in this function kernel/futex.c:890:6: note: ‘oldval’ was declared here Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com> --- kernel/futex.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/kernel/futex.c b/kernel/futex.c index fe28dc2..516ebf9 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -825,7 +825,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this) { struct task_struct *new_owner; struct futex_pi_state *pi_state = this->pi_state; - u32 curval, newval; + u32 curval = 0, newval; if (!pi_state) return -EINVAL; @@ -887,7 +887,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this) static int unlock_futex_pi(u32 __user *uaddr, u32 uval) { - u32 oldval; + u32 oldval = 0; /* * There is no waiter, so we unlock the futex. The owner died @@ -1546,7 +1546,7 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, u32 newtid = task_pid_vnr(newowner) | FUTEX_WAITERS; struct futex_pi_state *pi_state = q->pi_state; struct task_struct *oldowner = pi_state->owner; - u32 uval, curval, newval; + u32 uval, curval = 0, newval; int ret; /* Owner died? */ @@ -2451,7 +2451,7 @@ err_unlock: */ int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi) { - u32 uval, nval, mval; + u32 uval, nval = 0, mval; retry: if (get_user(uval, uaddr)) -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] futex: warning corrections 2011-07-04 23:21 [PATCH 4/4] futex: warning corrections Vitaliy Ivanov @ 2011-07-06 17:00 ` Darren Hart 2011-07-06 21:11 ` Vitaliy Ivanov 0 siblings, 1 reply; 8+ messages in thread From: Darren Hart @ 2011-07-06 17:00 UTC (permalink / raw) To: vitalivanov; +Cc: Thomas Gleixner, Linus Torvalds, lkml On 07/04/2011 04:21 PM, Vitaliy Ivanov wrote: > From 3f7997d71fe2b5cb16e2913928f68023855d786d Mon Sep 17 00:00:00 2001 > From: Vitaliy Ivanov <vitalivanov@gmail.com> > Date: Tue, 5 Jul 2011 02:07:42 +0300 > Subject: [PATCH 4/4] futex: warning corrections > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > Hi Vitaliy, Thanks for looking to fix these warnings. Note that the compiler isn't always aware of some of the subtleties involved with things like cmpxchg. In cases where it thinks there may be an uninitialized usage, be sure to confirm it is possible before adding the overhead of an assignment to a hot path. None of these assignments are necessary. Consider using __maybe_unused instead. -- Darren > kernel/futex.c: In function ‘fixup_pi_state_owner.clone.17’: > kernel/futex.c:1582:6: warning: ‘curval’ may be used uninitialized in this function > kernel/futex.c: In function ‘handle_futex_death’: > kernel/futex.c:2486:6: warning: ‘nval’ may be used uninitialized in this function > kernel/futex.c: In function ‘do_futex’: > kernel/futex.c:863:11: warning: ‘curval’ may be used uninitialized in this function > kernel/futex.c:828:6: note: ‘curval’ was declared here > kernel/futex.c:898:5: warning: ‘oldval’ may be used uninitialized in this function > kernel/futex.c:890:6: note: ‘oldval’ was declared here > > Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com> > --- > kernel/futex.c | 8 ++++---- > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/kernel/futex.c b/kernel/futex.c > index fe28dc2..516ebf9 100644 > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -825,7 +825,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this) > { > struct task_struct *new_owner; > struct futex_pi_state *pi_state = this->pi_state; > - u32 curval, newval; > + u32 curval = 0, newval; > > if (!pi_state) > return -EINVAL; > @@ -887,7 +887,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this) > > static int unlock_futex_pi(u32 __user *uaddr, u32 uval) > { > - u32 oldval; > + u32 oldval = 0; > > /* > * There is no waiter, so we unlock the futex. The owner died > @@ -1546,7 +1546,7 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, > u32 newtid = task_pid_vnr(newowner) | FUTEX_WAITERS; > struct futex_pi_state *pi_state = q->pi_state; > struct task_struct *oldowner = pi_state->owner; > - u32 uval, curval, newval; > + u32 uval, curval = 0, newval; > int ret; > > /* Owner died? */ > @@ -2451,7 +2451,7 @@ err_unlock: > */ > int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi) > { > - u32 uval, nval, mval; > + u32 uval, nval = 0, mval; > > retry: > if (get_user(uval, uaddr)) -- Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] futex: warning corrections 2011-07-06 17:00 ` Darren Hart @ 2011-07-06 21:11 ` Vitaliy Ivanov 2011-07-06 22:29 ` Darren Hart 0 siblings, 1 reply; 8+ messages in thread From: Vitaliy Ivanov @ 2011-07-06 21:11 UTC (permalink / raw) To: Darren Hart, Jiri Kosina; +Cc: Thomas Gleixner, Linus Torvalds, lkml, trivial On Wed, 2011-07-06 at 10:00 -0700, Darren Hart wrote: > On 07/04/2011 04:21 PM, Vitaliy Ivanov wrote: > > From 3f7997d71fe2b5cb16e2913928f68023855d786d Mon Sep 17 00:00:00 2001 > > From: Vitaliy Ivanov <vitalivanov@gmail.com> > > Date: Tue, 5 Jul 2011 02:07:42 +0300 > > Subject: [PATCH 4/4] futex: warning corrections > Hi Vitaliy, > > Thanks for looking to fix these warnings. Note that the compiler isn't > always aware of some of the subtleties involved with things like > cmpxchg. In cases where it thinks there may be an uninitialized usage, > be sure to confirm it is possible before adding the overhead of an > assignment to a hot path. > > None of these assignments are necessary. Consider using __maybe_unused > instead. Darren, Actually unused and uninitialized are different issues. __maybe_unused won't help in this case but there is another trick used in kernel for this: #define uninitialized_var(x) x = x So, here is updated patch. >From 8eeaa5a97697bcc606aea23d32028aea7b271a96 Mon Sep 17 00:00:00 2001 From: Vitaliy Ivanov <vitalivanov@gmail.com> Date: Thu, 7 Jul 2011 00:05:05 +0300 Subject: [PATCH] futex: uninitialized warning corrections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit kernel/futex.c: In function ‘fixup_pi_state_owner.clone.17’: kernel/futex.c:1582:6: warning: ‘curval’ may be used uninitialized in this function kernel/futex.c: In function ‘handle_futex_death’: kernel/futex.c:2486:6: warning: ‘nval’ may be used uninitialized in this function kernel/futex.c: In function ‘do_futex’: kernel/futex.c:863:11: warning: ‘curval’ may be used uninitialized in this function kernel/futex.c:828:6: note: ‘curval’ was declared here kernel/futex.c:898:5: warning: ‘oldval’ may be used uninitialized in this function kernel/futex.c:890:6: note: ‘oldval’ was declared here Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com> --- kernel/futex.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/kernel/futex.c b/kernel/futex.c index fe28dc2..efb8e5b 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -825,7 +825,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this) { struct task_struct *new_owner; struct futex_pi_state *pi_state = this->pi_state; - u32 curval, newval; + u32 uninitialized_var(curval), newval; if (!pi_state) return -EINVAL; @@ -887,7 +887,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this) static int unlock_futex_pi(u32 __user *uaddr, u32 uval) { - u32 oldval; + u32 uninitialized_var(oldval); /* * There is no waiter, so we unlock the futex. The owner died @@ -1546,7 +1546,7 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, u32 newtid = task_pid_vnr(newowner) | FUTEX_WAITERS; struct futex_pi_state *pi_state = q->pi_state; struct task_struct *oldowner = pi_state->owner; - u32 uval, curval, newval; + u32 uval, uninitialized_var(curval), newval; int ret; /* Owner died? */ @@ -2451,7 +2451,7 @@ err_unlock: */ int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi) { - u32 uval, nval, mval; + u32 uval, uninitialized_var(nval), mval; retry: if (get_user(uval, uaddr)) -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] futex: warning corrections 2011-07-06 21:11 ` Vitaliy Ivanov @ 2011-07-06 22:29 ` Darren Hart 2011-07-07 12:39 ` Vitaliy Ivanov 0 siblings, 1 reply; 8+ messages in thread From: Darren Hart @ 2011-07-06 22:29 UTC (permalink / raw) To: vitalivanov; +Cc: Jiri Kosina, Thomas Gleixner, Linus Torvalds, lkml, trivial On 07/06/2011 02:11 PM, Vitaliy Ivanov wrote: > On Wed, 2011-07-06 at 10:00 -0700, Darren Hart wrote: >> On 07/04/2011 04:21 PM, Vitaliy Ivanov wrote: >>> From 3f7997d71fe2b5cb16e2913928f68023855d786d Mon Sep 17 00:00:00 2001 >>> From: Vitaliy Ivanov <vitalivanov@gmail.com> >>> Date: Tue, 5 Jul 2011 02:07:42 +0300 >>> Subject: [PATCH 4/4] futex: warning corrections >> Hi Vitaliy, >> >> Thanks for looking to fix these warnings. Note that the compiler isn't >> always aware of some of the subtleties involved with things like >> cmpxchg. In cases where it thinks there may be an uninitialized usage, >> be sure to confirm it is possible before adding the overhead of an >> assignment to a hot path. >> >> None of these assignments are necessary. Consider using __maybe_unused >> instead. > > Darren, > > Actually unused and uninitialized are different issues. __maybe_unused won't help in this case but there is another trick used in kernel for this: > Heh, duh. I recently worked with __maybe_unused and uninitialized_var(), and got them crossed in my head. > #define uninitialized_var(x) x = x Yes, this is the right approach. > > So, here is updated patch. > > From 8eeaa5a97697bcc606aea23d32028aea7b271a96 Mon Sep 17 00:00:00 2001 > From: Vitaliy Ivanov <vitalivanov@gmail.com> > Date: Thu, 7 Jul 2011 00:05:05 +0300 > Subject: [PATCH] futex: uninitialized warning corrections > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > kernel/futex.c: In function ‘fixup_pi_state_owner.clone.17’: > kernel/futex.c:1582:6: warning: ‘curval’ may be used uninitialized in this function > kernel/futex.c: In function ‘handle_futex_death’: > kernel/futex.c:2486:6: warning: ‘nval’ may be used uninitialized in this function > kernel/futex.c: In function ‘do_futex’: > kernel/futex.c:863:11: warning: ‘curval’ may be used uninitialized in this function > kernel/futex.c:828:6: note: ‘curval’ was declared here > kernel/futex.c:898:5: warning: ‘oldval’ may be used uninitialized in this function > kernel/futex.c:890:6: note: ‘oldval’ was declared here > > Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com> Please include a blurb in the commit message as to why you used uninitialized_var() rather than just assigning it. This will save people the time of wondering why, and me the time of nacking "it's simpler to just initialize to zero" patches :-) Acked-by: Darren Hart <dvhart@linux.intel.com> > --- > kernel/futex.c | 8 ++++---- > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/kernel/futex.c b/kernel/futex.c > index fe28dc2..efb8e5b 100644 > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -825,7 +825,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this) > { > struct task_struct *new_owner; > struct futex_pi_state *pi_state = this->pi_state; > - u32 curval, newval; > + u32 uninitialized_var(curval), newval; > > if (!pi_state) > return -EINVAL; > @@ -887,7 +887,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this) > > static int unlock_futex_pi(u32 __user *uaddr, u32 uval) > { > - u32 oldval; > + u32 uninitialized_var(oldval); > > /* > * There is no waiter, so we unlock the futex. The owner died > @@ -1546,7 +1546,7 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, > u32 newtid = task_pid_vnr(newowner) | FUTEX_WAITERS; > struct futex_pi_state *pi_state = q->pi_state; > struct task_struct *oldowner = pi_state->owner; > - u32 uval, curval, newval; > + u32 uval, uninitialized_var(curval), newval; > int ret; > > /* Owner died? */ > @@ -2451,7 +2451,7 @@ err_unlock: > */ > int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi) > { > - u32 uval, nval, mval; > + u32 uval, uninitialized_var(nval), mval; > > retry: > if (get_user(uval, uaddr)) -- Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] futex: warning corrections 2011-07-06 22:29 ` Darren Hart @ 2011-07-07 12:39 ` Vitaliy Ivanov 2011-07-07 18:06 ` Darren Hart 0 siblings, 1 reply; 8+ messages in thread From: Vitaliy Ivanov @ 2011-07-07 12:39 UTC (permalink / raw) To: Darren Hart, Jiri Kosina; +Cc: Thomas Gleixner, Linus Torvalds, lkml, trivial >> From 8eeaa5a97697bcc606aea23d32028aea7b271a96 Mon Sep 17 00:00:00 2001 >> From: Vitaliy Ivanov <vitalivanov@gmail.com> >> Date: Thu, 7 Jul 2011 00:05:05 +0300 >> Subject: [PATCH] futex: uninitialized warning corrections >> MIME-Version: 1.0 >> Content-Type: text/plain; charset=UTF-8 >> Content-Transfer-Encoding: 8bit >> >> kernel/futex.c: In function ‘fixup_pi_state_owner.clone.17’: >> kernel/futex.c:1582:6: warning: ‘curval’ may be used uninitialized in this function >> kernel/futex.c: In function ‘handle_futex_death’: >> kernel/futex.c:2486:6: warning: ‘nval’ may be used uninitialized in this function >> kernel/futex.c: In function ‘do_futex’: >> kernel/futex.c:863:11: warning: ‘curval’ may be used uninitialized in this function >> kernel/futex.c:828:6: note: ‘curval’ was declared here >> kernel/futex.c:898:5: warning: ‘oldval’ may be used uninitialized in this function >> kernel/futex.c:890:6: note: ‘oldval’ was declared here >> >> Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com> > > Please include a blurb in the commit message as to why you used > uninitialized_var() rather than just assigning it. This will save people > the time of wondering why, and me the time of nacking "it's simpler to > just initialize to zero" patches :-) > > Acked-by: Darren Hart <dvhart@linux.intel.com> Darren, Thanks for your comments. I think the description is pretty obvious here as I don't think any of these variables are affected by cmpxchg. There is simple assignment at the end. Seems like compiler simply doesn't follow all the return cases. Anyway, I'm going to resend patches to Jiri for his trivial-next tree. Vitaliy ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] futex: warning corrections 2011-07-07 12:39 ` Vitaliy Ivanov @ 2011-07-07 18:06 ` Darren Hart 2011-07-08 15:00 ` Vitaliy Ivanov 0 siblings, 1 reply; 8+ messages in thread From: Darren Hart @ 2011-07-07 18:06 UTC (permalink / raw) To: Vitaliy Ivanov Cc: Jiri Kosina, Thomas Gleixner, Linus Torvalds, lkml, trivial On 07/07/2011 05:39 AM, Vitaliy Ivanov wrote: >>> From 8eeaa5a97697bcc606aea23d32028aea7b271a96 Mon Sep 17 00:00:00 2001 >>> From: Vitaliy Ivanov <vitalivanov@gmail.com> >>> Date: Thu, 7 Jul 2011 00:05:05 +0300 >>> Subject: [PATCH] futex: uninitialized warning corrections >>> MIME-Version: 1.0 >>> Content-Type: text/plain; charset=UTF-8 >>> Content-Transfer-Encoding: 8bit >>> >>> kernel/futex.c: In function ‘fixup_pi_state_owner.clone.17’: >>> kernel/futex.c:1582:6: warning: ‘curval’ may be used uninitialized in this function >>> kernel/futex.c: In function ‘handle_futex_death’: >>> kernel/futex.c:2486:6: warning: ‘nval’ may be used uninitialized in this function >>> kernel/futex.c: In function ‘do_futex’: >>> kernel/futex.c:863:11: warning: ‘curval’ may be used uninitialized in this function >>> kernel/futex.c:828:6: note: ‘curval’ was declared here >>> kernel/futex.c:898:5: warning: ‘oldval’ may be used uninitialized in this function >>> kernel/futex.c:890:6: note: ‘oldval’ was declared here >>> >>> Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com> >> >> Please include a blurb in the commit message as to why you used >> uninitialized_var() rather than just assigning it. This will save people >> the time of wondering why, and me the time of nacking "it's simpler to >> just initialize to zero" patches :-) >> >> Acked-by: Darren Hart <dvhart@linux.intel.com> > > Darren, > > Thanks for your comments. I think the description is pretty obvious > here as I don't think any of these variables are affected by cmpxchg. Not so. Consider the following: u32 curval; if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)) ret = -EFAULT; else if (curval != uval) ret = -EINVAL; the cmpxchg here assigns curval to newval if *uaddr==uval or to *uaddr otherwise. This is where curval gets assigned so that it can then be read in the following if block. gcc didn't recognize this as an assignment and is why it complained about it being used uninitialized. > There is simple assignment at the end. Seems like compiler simply > doesn't follow all the return cases. No, the compiler complained about the test of the value, this doesn't have anything to do with the return cases. -- Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] futex: warning corrections 2011-07-07 18:06 ` Darren Hart @ 2011-07-08 15:00 ` Vitaliy Ivanov 2011-07-08 17:07 ` Darren Hart 0 siblings, 1 reply; 8+ messages in thread From: Vitaliy Ivanov @ 2011-07-08 15:00 UTC (permalink / raw) To: Darren Hart; +Cc: Jiri Kosina, Thomas Gleixner, Linus Torvalds, lkml, trivial On Thu, 2011-07-07 at 11:06 -0700, Darren Hart wrote: > > On 07/07/2011 05:39 AM, Vitaliy Ivanov wrote: > >>> From 8eeaa5a97697bcc606aea23d32028aea7b271a96 Mon Sep 17 00:00:00 2001 > >>> From: Vitaliy Ivanov <vitalivanov@gmail.com> > >>> Date: Thu, 7 Jul 2011 00:05:05 +0300 > >>> Subject: [PATCH] futex: uninitialized warning corrections > >>> MIME-Version: 1.0 > >>> Content-Type: text/plain; charset=UTF-8 > >>> Content-Transfer-Encoding: 8bit > >>> > >>> kernel/futex.c: In function ‘fixup_pi_state_owner.clone.17’: > >>> kernel/futex.c:1582:6: warning: ‘curval’ may be used uninitialized in this function > >>> kernel/futex.c: In function ‘handle_futex_death’: > >>> kernel/futex.c:2486:6: warning: ‘nval’ may be used uninitialized in this function > >>> kernel/futex.c: In function ‘do_futex’: > >>> kernel/futex.c:863:11: warning: ‘curval’ may be used uninitialized in this function > >>> kernel/futex.c:828:6: note: ‘curval’ was declared here > >>> kernel/futex.c:898:5: warning: ‘oldval’ may be used uninitialized in this function > >>> kernel/futex.c:890:6: note: ‘oldval’ was declared here > >>> > >>> Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com> > >> > >> Please include a blurb in the commit message as to why you used > >> uninitialized_var() rather than just assigning it. This will save people > >> the time of wondering why, and me the time of nacking "it's simpler to > >> just initialize to zero" patches :-) > >> > >> Acked-by: Darren Hart <dvhart@linux.intel.com> > > > > Darren, > > > > Thanks for your comments. I think the description is pretty obvious > > here as I don't think any of these variables are affected by cmpxchg. > > Not so. Consider the following: > > u32 curval; > if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)) > ret = -EFAULT; > else if (curval != uval) > ret = -EINVAL; > > the cmpxchg here assigns curval to newval if *uaddr==uval or to *uaddr > otherwise. This is where curval gets assigned so that it can then be > read in the following if block. gcc didn't recognize this as an > assignment and is why it complained about it being used uninitialized. > > > > There is simple assignment at the end. Seems like compiler simply > > doesn't follow all the return cases. > > No, the compiler complained about the test of the value, this doesn't > have anything to do with the return cases. Here is what we have: ------------ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, struct task_struct *newowner) { u32 curval; ... retry: if (get_futex_value_locked(&uval, uaddr)) goto handle_fault; while (1) { newval = (uval & FUTEX_OWNER_DIED) | newtid; if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)) goto handle_fault; if (curval == uval) break; uval = curval; } ... } ------------ static int cmpxchg_futex_value_locked(u32 *curval, u32 __user *uaddr, u32 uval, u32 newval) { int ret; pagefault_disable(); ret = futex_atomic_cmpxchg_inatomic(curval, uaddr, uval, newval); pagefault_enable(); return ret; } ------------ And for x86: static inline int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval, u32 newval) { int ret = 0; #if defined(CONFIG_X86_32) && !defined(CONFIG_X86_BSWAP) /* Real i386 machines have no cmpxchg instruction */ if (boot_cpu_data.x86 == 3) return -ENOSYS; #endif if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))) return -EFAULT; asm volatile("1:\t" LOCK_PREFIX "cmpxchgl %4, %2\n" "2:\t.section .fixup, \"ax\"\n" "3:\tmov %3, %0\n" "\tjmp 2b\n" "\t.previous\n" _ASM_EXTABLE(1b, 3b) : "+r" (ret), "=a" (oldval), "+m" (*uaddr) : "i" (-EFAULT), "r" (newval), "1" (oldval) : "memory" ); *uval = oldval; <------------- uval is being changed here only. it's not modified by asm cmpxchgl. return ret; } Am I missing something? - Vitaliy ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] futex: warning corrections 2011-07-08 15:00 ` Vitaliy Ivanov @ 2011-07-08 17:07 ` Darren Hart 0 siblings, 0 replies; 8+ messages in thread From: Darren Hart @ 2011-07-08 17:07 UTC (permalink / raw) To: Vitaliy Ivanov Cc: Jiri Kosina, Thomas Gleixner, Linus Torvalds, lkml, trivial On 07/08/2011 08:00 AM, Vitaliy Ivanov wrote: > On Thu, 2011-07-07 at 11:06 -0700, Darren Hart wrote: >> >> On 07/07/2011 05:39 AM, Vitaliy Ivanov wrote: >>>>> From 8eeaa5a97697bcc606aea23d32028aea7b271a96 Mon Sep 17 00:00:00 2001 >>>>> From: Vitaliy Ivanov <vitalivanov@gmail.com> >>>>> Date: Thu, 7 Jul 2011 00:05:05 +0300 >>>>> Subject: [PATCH] futex: uninitialized warning corrections >>>>> MIME-Version: 1.0 >>>>> Content-Type: text/plain; charset=UTF-8 >>>>> Content-Transfer-Encoding: 8bit >>>>> >>>>> kernel/futex.c: In function ‘fixup_pi_state_owner.clone.17’: >>>>> kernel/futex.c:1582:6: warning: ‘curval’ may be used uninitialized in this function >>>>> kernel/futex.c: In function ‘handle_futex_death’: >>>>> kernel/futex.c:2486:6: warning: ‘nval’ may be used uninitialized in this function >>>>> kernel/futex.c: In function ‘do_futex’: >>>>> kernel/futex.c:863:11: warning: ‘curval’ may be used uninitialized in this function >>>>> kernel/futex.c:828:6: note: ‘curval’ was declared here >>>>> kernel/futex.c:898:5: warning: ‘oldval’ may be used uninitialized in this function >>>>> kernel/futex.c:890:6: note: ‘oldval’ was declared here >>>>> >>>>> Signed-off-by: Vitaliy Ivanov <vitalivanov@gmail.com> >>>> >>>> Please include a blurb in the commit message as to why you used >>>> uninitialized_var() rather than just assigning it. This will save people >>>> the time of wondering why, and me the time of nacking "it's simpler to >>>> just initialize to zero" patches :-) >>>> >>>> Acked-by: Darren Hart <dvhart@linux.intel.com> >>> >>> Darren, >>> >>> Thanks for your comments. I think the description is pretty obvious >>> here as I don't think any of these variables are affected by cmpxchg. >> >> Not so. Consider the following: >> >> u32 curval; >> if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)) >> ret = -EFAULT; >> else if (curval != uval) >> ret = -EINVAL; >> >> the cmpxchg here assigns curval to newval if *uaddr==uval or to *uaddr >> otherwise. This is where curval gets assigned so that it can then be >> read in the following if block. gcc didn't recognize this as an >> assignment and is why it complained about it being used uninitialized. >> >> >>> There is simple assignment at the end. Seems like compiler simply >>> doesn't follow all the return cases. >> >> No, the compiler complained about the test of the value, this doesn't >> have anything to do with the return cases. > > Here is what we have: > > ------------ > static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, > struct task_struct *newowner) > { > u32 curval; > ... > retry: > if (get_futex_value_locked(&uval, uaddr)) > goto handle_fault; > > while (1) { > newval = (uval & FUTEX_OWNER_DIED) | newtid; > > if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)) > goto handle_fault; > if (curval == uval) > break; > uval = curval; > } > ... > } > > ------------ > static int cmpxchg_futex_value_locked(u32 *curval, u32 __user *uaddr, > u32 uval, u32 newval) > { > int ret; > > pagefault_disable(); > ret = futex_atomic_cmpxchg_inatomic(curval, uaddr, uval, newval); > pagefault_enable(); > > return ret; > } > ------------ > > And for x86: > > static inline int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, > u32 oldval, u32 newval) > { > int ret = 0; > > #if defined(CONFIG_X86_32) && !defined(CONFIG_X86_BSWAP) > /* Real i386 machines have no cmpxchg instruction */ > if (boot_cpu_data.x86 == 3) > return -ENOSYS; > #endif > > if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))) > return -EFAULT; > > asm volatile("1:\t" LOCK_PREFIX "cmpxchgl %4, %2\n" > "2:\t.section .fixup, \"ax\"\n" > "3:\tmov %3, %0\n" > "\tjmp 2b\n" > "\t.previous\n" > _ASM_EXTABLE(1b, 3b) > : "+r" (ret), "=a" (oldval), "+m" (*uaddr) > : "i" (-EFAULT), "r" (newval), "1" (oldval) > : "memory" > ); > > *uval = oldval; <------------- uval is being changed here only. it's not modified by asm cmpxchgl. > return ret; > } > > > Am I missing something? Nope, I was incorrect - I didn't realize that futex_atomic_cmpxchg_inatomic didn't set uval on EFAULT. gcc is not detecting that curval is only read if futex_atomic_cmpxchg_inatomic succeeds, so I believe the uninitialized_var() fix is correct. This is one of the reasons why we need to include the reasoning in the commit log - specifically that: "futex_atomic_cmpxchg_inatomic only assigns curval on success, but on failure curval is not read, so instruct gcc to ignore the uninitialized warning." -- Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-07-08 17:07 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-07-04 23:21 [PATCH 4/4] futex: warning corrections Vitaliy Ivanov 2011-07-06 17:00 ` Darren Hart 2011-07-06 21:11 ` Vitaliy Ivanov 2011-07-06 22:29 ` Darren Hart 2011-07-07 12:39 ` Vitaliy Ivanov 2011-07-07 18:06 ` Darren Hart 2011-07-08 15:00 ` Vitaliy Ivanov 2011-07-08 17:07 ` Darren Hart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).