kfifo must_check warning

kfifo must_check warning

[Randy Dunlap]

In 2.6.36-rc8, I see this build warning:

drivers/char/n_gsm.c: In function 'gsm_dlci_alloc':
drivers/char/n_gsm.c:1580: warning: ignoring return value of '__kfifo_must_check_helper', declared with attribute warn_unused_result

The helper seems to be getting in the way (?).  The driver code does this:

	if (kfifo_alloc(&dlci->_fifo, 4096, GFP_KERNEL) < 0) {
		kfree(dlci);
		return NULL;
	}

Should the driver code be doing something else?
or should the kfifo_alloc() macro be checking the result of the helper?


thanks,
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: kfifo must_check warning

[Andrew Morton]

On Tue, 19 Oct 2010 15:10:48 -0700 Randy Dunlap <randy.dunlap@oracle.com> wrote:

> In 2.6.36-rc8, I see this build warning:
> 
> drivers/char/n_gsm.c: In function 'gsm_dlci_alloc':
> drivers/char/n_gsm.c:1580: warning: ignoring return value of '__kfifo_must_check_helper', declared with attribute warn_unused_result
> 
> The helper seems to be getting in the way (?).  The driver code does this:
> 
> 	if (kfifo_alloc(&dlci->_fifo, 4096, GFP_KERNEL) < 0) {
> 		kfree(dlci);
> 		return NULL;
> 	}
> 
> Should the driver code be doing something else?
> or should the kfifo_alloc() macro be checking the result of the helper?
> 

A gcc bug, I'd say.  The code looks OK and my gcc doesn't warn.

Perhaps see if you can find some code transformation in n_gsm.c which
makes it go away?  Add a new local variable or something.


I did see a probably unrelated bug in there though. 
__kfifo_must_check_helper() coerces its arg into an `unsigned int' and
returns an unsigned int.  Consequently if kfifo_alloc() tries to return
-EINVAL, the above-quoted code won't detect the error: it will see
-EINVAL as a large, positive unsigned value.

I don't see a simple fix for that apart from creating several flavours
of __kfifo_must_check_helper() and carefully going through each
instance and using the one which takes (and returns) the correct type.

A suitable temporary 2.6.37 patch would bee to just disable
__kfifo_must_check_helper():

--- a/include/linux/kfifo.h~a
+++ a/include/linux/kfifo.h
@@ -171,11 +171,7 @@ struct kfifo_rec_ptr_2 __STRUCT_KFIFO_PT
 	}
 
 
-static inline unsigned int __must_check
-__kfifo_must_check_helper(unsigned int val)
-{
-	return val;
-}
+#define __kfifo_must_check_helper(x) (x)
 
 /**
  * kfifo_initialized - Check if the fifo is initialized
_

Re: kfifo must_check warning (+ patch)

[Randy Dunlap]

On Tue, 19 Oct 2010 22:12:42 -0700 Andrew Morton wrote:

> On Tue, 19 Oct 2010 15:10:48 -0700 Randy Dunlap <randy.dunlap@oracle.com> wrote:
> 
> > In 2.6.36-rc8, I see this build warning:
> > 
> > drivers/char/n_gsm.c: In function 'gsm_dlci_alloc':
> > drivers/char/n_gsm.c:1580: warning: ignoring return value of '__kfifo_must_check_helper', declared with attribute warn_unused_result
> > 
> > The helper seems to be getting in the way (?).  The driver code does this:
> > 
> > 	if (kfifo_alloc(&dlci->_fifo, 4096, GFP_KERNEL) < 0) {
> > 		kfree(dlci);
> > 		return NULL;
> > 	}
> > 
> > Should the driver code be doing something else?
> > or should the kfifo_alloc() macro be checking the result of the helper?
> > 
> 
> A gcc bug, I'd say.  The code looks OK and my gcc doesn't warn.

OK.

> Perhaps see if you can find some code transformation in n_gsm.c which
> makes it go away?  Add a new local variable or something.

Yes, that works for me.  Patch is below.

> I did see a probably unrelated bug in there though. 
> __kfifo_must_check_helper() coerces its arg into an `unsigned int' and
> returns an unsigned int.  Consequently if kfifo_alloc() tries to return
> -EINVAL, the above-quoted code won't detect the error: it will see
> -EINVAL as a large, positive unsigned value.
> 
> I don't see a simple fix for that apart from creating several flavours
> of __kfifo_must_check_helper() and carefully going through each
> instance and using the one which takes (and returns) the correct type.
> 
> A suitable temporary 2.6.37 patch would bee to just disable
> __kfifo_must_check_helper():
> 
> --- a/include/linux/kfifo.h~a
> +++ a/include/linux/kfifo.h
> @@ -171,11 +171,7 @@ struct kfifo_rec_ptr_2 __STRUCT_KFIFO_PT
>  	}
>  
>  
> -static inline unsigned int __must_check
> -__kfifo_must_check_helper(unsigned int val)
> -{
> -	return val;
> -}
> +#define __kfifo_must_check_helper(x) (x)
>  
>  /**
>   * kfifo_initialized - Check if the fifo is initialized
> _
> 
> --


From: Randy Dunlap <randy.dunlap@oracle.com>

Some versions of gcc mysteriously report:
drivers/char/n_gsm.c:1580: warning: ignoring return value of '__kfifo_must_check_helper', declared with attribute warn_unused_result

This warning can be eliminated by using a local variable for the
returned result value, as suggested by Andrew Morton.

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 drivers/char/n_gsm.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- linux-next-20101019.orig/drivers/char/n_gsm.c
+++ linux-next-20101019/drivers/char/n_gsm.c
@@ -1573,11 +1573,14 @@ static void gsm_dlci_command(struct gsm_
 static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
 {
 	struct gsm_dlci *dlci = kzalloc(sizeof(struct gsm_dlci), GFP_ATOMIC);
+	int err;
+
 	if (dlci == NULL)
 		return NULL;
 	spin_lock_init(&dlci->lock);
 	dlci->fifo = &dlci->_fifo;
-	if (kfifo_alloc(&dlci->_fifo, 4096, GFP_KERNEL) < 0) {
+	err = kfifo_alloc(&dlci->_fifo, 4096, GFP_KERNEL);
+	if (err < 0) {
 		kfree(dlci);
 		return NULL;
 	}

Re: kfifo must_check warning (+ patch)

[miltonm]

> --- linux-next-20101019.orig/drivers/char/n_gsm.c
> +++ linux-next-20101019/drivers/char/n_gsm.c
> @@ -1573,11 +1573,14 @@  static void gsm_dlci_command(struct gsm_
>  static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
>  {
>  	struct gsm_dlci *dlci = kzalloc(sizeof(struct gsm_dlci), GFP_ATOMIC);
> +	int err;
> +
>  	if (dlci == NULL)
>  		return NULL;
>  	spin_lock_init(&dlci->lock);
>  	dlci->fifo = &dlci->_fifo;
> -	if (kfifo_alloc(&dlci->_fifo, 4096, GFP_KERNEL) < 0) {
> +	err = kfifo_alloc(&dlci->_fifo, 4096, GFP_KERNEL);
> +	if (err < 0) {
>  		kfree(dlci);
>  		return NULL;
>  	}
> 
> 


I think this is not a gcc bug but Randy's gcc beeing smarter
and showing a signed bug with the old code.

The earlier discussion had:

> > > Should the driver code be doing something else?
> > > or should the kfifo_alloc() macro be checking the result of the helper?
> > > 
> > 
> > A gcc bug, I'd say.  The code looks OK and my gcc doesn't warn.
> 
> OK.
> 
> > Perhaps see if you can find some code transformation in n_gsm.c which
> > makes it go away?  Add a new local variable or something.
> 
> Yes, that works for me.  Patch is below.
> 

> > I did see a probably unrelated bug in there though. 
> > __kfifo_must_check_helper() coerces its arg into an `unsigned int' and
> > returns an unsigned int.  Consequently if kfifo_alloc() tries to return
> > -EINVAL, the above-quoted code won't detect the error: it will see
> > -EINVAL as a large, positive unsigned value.
> > 

and then gcc decides that the check of < 0 is meaningless, and then
says that the result is unused.

Note: I'm a kernel hacker , not a gcc one.  This is pure speculation.

So to me the helper is the one causing the error!  ... and the patch is
undoing the damage at the caller.

So the correct fix is to change the return type of the helper,
and if someone wants the int to be unsigned then it needs to be
unsigned long so we can use an is_error type test.

milton

Re: kfifo must_check warning (+ patch)

[Alan Cox]

> Some versions of gcc mysteriously report:
> drivers/char/n_gsm.c:1580: warning: ignoring return value of '__kfifo_must_check_helper', declared with attribute warn_unused_result
> 
> This warning can be eliminated by using a local variable for the
> returned result value, as suggested by Andrew Morton.

But that just hides the bug in the helper surely - the helper has type
errors so needs fixing - not this. If you apply this then the underlying
bug is just going to get forgotten

Alan

Re: kfifo must_check warning (+ patch)

[Andrew Morton]

On Wed, 20 Oct 2010 19:37:02 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > Some versions of gcc mysteriously report:
> > drivers/char/n_gsm.c:1580: warning: ignoring return value of '__kfifo_must_check_helper', declared with attribute warn_unused_result
> > 
> > This warning can be eliminated by using a local variable for the
> > returned result value, as suggested by Andrew Morton.
> 
> But that just hides the bug in the helper surely - the helper has type
> errors so needs fixing - not this. If you apply this then the underlying
> bug is just going to get forgotten
> 

It won't get forgotten.

I agree that the warning is probably an artifact of the signedness
thing.  So for 2.6.36, disabling __kfifo_must_check_helper() will
suffice.  I'm sure that Stefani will fix up __kfifo_must_check_helper()
for real for 2.6.37, then we can see if the warning reoccurs.

Re: kfifo must_check warning (+ patch)

[Stefani Seibold]

The problem should be fixed. I renamed the __kfifo_must_check_helper()
into __kfifo_uint_must_check_helper() and add a new
__kfifo_int_must_check_helper() which is now used for kfifo_alloc().

A kernel linux 2.6.36-rc compiled with gcc 4.4.4 doesn't show the
warning.

There a only two functions which return a int: kfifo_alloc() and
kfifo_init(). The later one does not make use of
__kfifo_int_must_check_helper(), but this could be change if required.

I will send the patch in a separate mail.

Am Mittwoch, den 20.10.2010, 11:56 -0700 schrieb Andrew Morton:
> On Wed, 20 Oct 2010 19:37:02 +0100
> Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> 
> > > Some versions of gcc mysteriously report:
> > > drivers/char/n_gsm.c:1580: warning: ignoring return value of '__kfifo_must_check_helper', declared with attribute warn_unused_result
> > > 
> > > This warning can be eliminated by using a local variable for the
> > > returned result value, as suggested by Andrew Morton.
> > 
> > But that just hides the bug in the helper surely - the helper has type
> > errors so needs fixing - not this. If you apply this then the underlying
> > bug is just going to get forgotten
> > 
> 
> It won't get forgotten.
> 
> I agree that the warning is probably an artifact of the signedness
> thing.  So for 2.6.36, disabling __kfifo_must_check_helper() will
> suffice.  I'm sure that Stefani will fix up __kfifo_must_check_helper()
> for real for 2.6.37, then we can see if the warning reoccurs.