[PATCH] mm/zswap.c: add BUG() for default case in zswap_writeback_entry()

[PATCH] mm/zswap.c: add BUG() for default case in zswap_writeback_entry()

[Chen Gang]

Recommend to add default case to avoid compiler's warning, although at
present, the original implementation is still correct.

The related warning (with allmodconfig for metag):

    CC      mm/zswap.o
  mm/zswap.c: In function 'zswap_writeback_entry':
  mm/zswap.c:537: warning: 'ret' may be used uninitialized in this function


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 mm/zswap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 5a63f78..bfd1807 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -585,6 +585,8 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
 
 		/* page is up to date */
 		SetPageUptodate(page);
+	default:
+		BUG();
 	}
 
 	/* move it to the tail of the inactive list after end_writeback */
-- 
1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap.c: add BUG() for default case in zswap_writeback_entry()

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

On 09/12/13 02:51, Chen Gang wrote:
> Recommend to add default case to avoid compiler's warning, although at
> present, the original implementation is still correct.
> 
> The related warning (with allmodconfig for metag):
> 
>     CC      mm/zswap.o
>   mm/zswap.c: In function 'zswap_writeback_entry':
>   mm/zswap.c:537: warning: 'ret' may be used uninitialized in this function
> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  mm/zswap.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 5a63f78..bfd1807 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -585,6 +585,8 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
>  
>  		/* page is up to date */
>  		SetPageUptodate(page);
> +	default:
> +		BUG();

This doesn't hide the warning when CONFIG_BUG=n since BUG() optimises
out completely.

Since the metag compiler is stuck on an old version (gcc 4.2.4), which
is wrong to warn in this case, and newer versions of gcc don't appear to
warn about it anyway (I just checked with gcc 4.7.2 x86_64), I have no
objection to this warning remaining in the metag build.

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mm/zswap.c: add BUG() for default case in zswap_writeback_entry()

[Chen Gang]

On 12/09/2013 05:54 PM, James Hogan wrote:
> On 09/12/13 02:51, Chen Gang wrote:
>> Recommend to add default case to avoid compiler's warning, although at
>> present, the original implementation is still correct.
>>
>> The related warning (with allmodconfig for metag):
>>
>>     CC      mm/zswap.o
>>   mm/zswap.c: In function 'zswap_writeback_entry':
>>   mm/zswap.c:537: warning: 'ret' may be used uninitialized in this function
>>
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  mm/zswap.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index 5a63f78..bfd1807 100644
>> --- a/mm/zswap.c
>> +++ b/mm/zswap.c
>> @@ -585,6 +585,8 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
>>  
>>  		/* page is up to date */
>>  		SetPageUptodate(page);
>> +	default:
>> +		BUG();
> 
> This doesn't hide the warning when CONFIG_BUG=n since BUG() optimises
> out completely.
> 

When "CONFIG_BUG=n", it will report many related warnings (for me,
CONFIG_BUG need be as architecture specific config feature, not a
generic config feature -- most architectures always enable it).

So for common generic users, we can assume it will always have effect.


> Since the metag compiler is stuck on an old version (gcc 4.2.4), which
> is wrong to warn in this case, and newer versions of gcc don't appear to
> warn about it anyway (I just checked with gcc 4.7.2 x86_64), I have no
> objection to this warning remaining in the metag build.
> 

Do you try "EXTRA_CFLAGS=-W" with gcc 4.7.2? I guess it will report the
warning too, I don't feel the compiler is smart enough (except it lets
the long function zswap_get_swap_cache_page really inline)  :-)


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water and life which God blessed

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap.c: add BUG() for default case in zswap_writeback_entry()

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 729 bytes --]

On 09/12/13 10:11, Chen Gang wrote:
>> Since the metag compiler is stuck on an old version (gcc 4.2.4), which
>> is wrong to warn in this case, and newer versions of gcc don't appear to
>> warn about it anyway (I just checked with gcc 4.7.2 x86_64), I have no
>> objection to this warning remaining in the metag build.
>>
> 
> Do you try "EXTRA_CFLAGS=-W" with gcc 4.7.2? I guess it will report the
> warning too, I don't feel the compiler is smart enough (except it lets
> the long function zswap_get_swap_cache_page really inline)  :-)

EXTRA_CFLAGS=-W on gcc 4.7.2 gives me plenty of pointless unused
parameter warnings when compiling mm/zswap.o, but not the warning you're
trying to silence.

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mm/zswap.c: add BUG() for default case in zswap_writeback_entry()

[Chen Gang]

[-- Attachment #1: Type: text/plain, Size: 1656 bytes --]

On 12/09/2013 06:18 PM, James Hogan wrote:
> On 09/12/13 10:11, Chen Gang wrote:
>>> Since the metag compiler is stuck on an old version (gcc 4.2.4), which
>>> is wrong to warn in this case, and newer versions of gcc don't appear to
>>> warn about it anyway (I just checked with gcc 4.7.2 x86_64), I have no
>>> objection to this warning remaining in the metag build.
>>>
>>
>> Do you try "EXTRA_CFLAGS=-W" with gcc 4.7.2? I guess it will report the
>> warning too, I don't feel the compiler is smart enough (except it lets
>> the long function zswap_get_swap_cache_page really inline)  :-)
> 
> EXTRA_CFLAGS=-W on gcc 4.7.2 gives me plenty of pointless unused
> parameter warnings when compiling mm/zswap.o, but not the warning you're
> trying to silence.
> 

Yeah, it will generate plenty of pointless warnings, although we still
can often find valuable bugs in these warnings.

Oh, I tried gcc 4.6.3-2 rhel version, get the same result as yours (do
not report warning), but for me, it is still a compiler's bug, it
*should* report a warning for it, we can try below:

 - modify zswap_get_swap_cache_page() to let it may return another value
   (one sample modification is in attachment)

 - compile again, it doesn't report related warning, either

 - in this case, it *should* report related warning.

Could you help to try it under gcc 4.7.2, thanks?


BTW: gcc really exists some bugs about uninitialized variable, e.g.

  one known bug: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501
  kernel related: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57856


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water and life which God blessed

[-- Attachment #2: diff.patch --]
[-- Type: text/x-patch, Size: 345 bytes --]

diff --git a/mm/zswap.c b/mm/zswap.c
index 5a63f78..1853ef4 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -469,8 +469,7 @@ static int zswap_get_swap_cache_page(swp_entry_t entry,
 		 */
 		err = radix_tree_preload(GFP_KERNEL);
 		if (err)
-			break;
-
+			return -4;
 		/*
 		 * Swap entry may have been freed since our caller observed it.
 		 */

Re: [PATCH] mm/zswap.c: add BUG() for default case in zswap_writeback_entry()

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 584 bytes --]

On 09/12/13 11:21, Chen Gang wrote:
> Oh, I tried gcc 4.6.3-2 rhel version, get the same result as yours (do
> not report warning), but for me, it is still a compiler's bug, it
> *should* report a warning for it, we can try below:

Not necessarily. You can't expect the compiler to detect and warn about
more complex bugs the programmer writes, so you have to draw the line
somewhere.

IMO missing some potential bugs is better than warning about code that
isn't buggy since that just makes people ignore the warnings or
carelessly try to silence them.

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mm/zswap.c: add BUG() for default case in zswap_writeback_entry()

[Chen Gang]

On 12/09/2013 07:40 PM, James Hogan wrote:
> On 09/12/13 11:21, Chen Gang wrote:
>> Oh, I tried gcc 4.6.3-2 rhel version, get the same result as yours (do
>> not report warning), but for me, it is still a compiler's bug, it
>> *should* report a warning for it, we can try below:
> 
> Not necessarily. You can't expect the compiler to detect and warn about
> more complex bugs the programmer writes, so you have to draw the line
> somewhere.
> 

Yeah, we can not only depend on compiler to help us finding bugs.


> IMO missing some potential bugs is better than warning about code that
> isn't buggy since that just makes people ignore the warnings or
> carelessly try to silence them.
> 

I can understand, every members have their own taste, so this patch is
depended on related maintainers' taste (so kernel provided
"EXTRA_CFLAGS=-W" to satisfy some of guys taste -- e.g. me).  ;-)


Thanks
-- 
Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap.c: add BUG() for default case in zswap_writeback_entry()

[Seth Jennings]

On Mon, Dec 09, 2013 at 10:51:16AM +0800, Chen Gang wrote:
> Recommend to add default case to avoid compiler's warning, although at
> present, the original implementation is still correct.
> 
> The related warning (with allmodconfig for metag):
> 
>     CC      mm/zswap.o
>   mm/zswap.c: In function 'zswap_writeback_entry':
>   mm/zswap.c:537: warning: 'ret' may be used uninitialized in this function
> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  mm/zswap.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 5a63f78..bfd1807 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -585,6 +585,8 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
>  
>  		/* page is up to date */
>  		SetPageUptodate(page);
> +	default:
> +		BUG();

Typically, the way you want to address this is initialize ret to 0
at declaration time if not every control path will set that value.

Seth

>  	}
>  
>  	/* move it to the tail of the inactive list after end_writeback */
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap.c: add BUG() for default case in zswap_writeback_entry()

[Chen Gang]

On 12/09/2013 11:36 PM, Seth Jennings wrote:
> On Mon, Dec 09, 2013 at 10:51:16AM +0800, Chen Gang wrote:
>> Recommend to add default case to avoid compiler's warning, although at
>> present, the original implementation is still correct.
>>
>> The related warning (with allmodconfig for metag):
>>
>>     CC      mm/zswap.o
>>   mm/zswap.c: In function 'zswap_writeback_entry':
>>   mm/zswap.c:537: warning: 'ret' may be used uninitialized in this function
>>
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  mm/zswap.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index 5a63f78..bfd1807 100644
>> --- a/mm/zswap.c
>> +++ b/mm/zswap.c
>> @@ -585,6 +585,8 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
>>  
>>  		/* page is up to date */
>>  		SetPageUptodate(page);
>> +	default:
>> +		BUG();
> 
> Typically, the way you want to address this is initialize ret to 0
> at declaration time if not every control path will set that value.
> 

At least, your suggestion sounds reasonable.

But I am also aware that normally need content 'default' for 'switch',
so I choose this fix way.

And sorry, current patch is incorrect (need 'break' before 'default'),
so if no additional suggestions or discussions or completions, I
will/should send patch v2 for it.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water and life which God blessed

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2] mm/zswap.c: add BUG() for default case in zswap_writeback_entry()

[Chen Gang]

Recommend to add default case to avoid compiler's warning, although at
present, the original implementation is still correct.

The related warning (with allmodconfig for metag):

    CC      mm/zswap.o
  mm/zswap.c: In function 'zswap_writeback_entry':
  mm/zswap.c:537: warning: 'ret' may be used uninitialized in this function


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 mm/zswap.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 5a63f78..f58a001 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -585,6 +585,10 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
 
 		/* page is up to date */
 		SetPageUptodate(page);
+		break;
+
+	default:
+		BUG();
 	}
 
 	/* move it to the tail of the inactive list after end_writeback */
-- 
1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>