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

PATCH Superseded Chen Gang 3 participants 10 messages Dec 9, 2013 – Dec 11, 2013

Subsystems (2)

Memory management zswap compressed swap caching

lore

Share & tools

lore

Chen Gang

author

2013-12-09 02:49 UTC #0 lore raw

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

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 <[email protected]>

---
 mm/zswap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

mm/zswap.c +2 -0

index 5a63f78..bfd1807 100644
@@ -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 */

Signature · 7 lines

1.7.7.6

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

James Hogan

2013-12-09 09:54 UTC #1 lore raw

Quoted 20 lines · 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 <[email protected]>
---
 mm/zswap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
mm/zswap.c +0 -0
index 5a63f78..bfd1807 100644

mm/zswap.c static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle) · line 589

Quoted 5 lines

mm/zswap.c +2 -0

@@ -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

Chen Gang

author

2013-12-09 10:08 UTC #2 lore raw

Quoted 22 lines · James Hogan

Quoted 21 lines · 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 <[email protected]>
---
 mm/zswap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
mm/zswap.c +0 -0
index 5a63f78..bfd1807 100644
@@ -585,6 +585,8 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
 

Quoted 8 lines

mm/zswap.c static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle) · line 589
Quoted 4 lines
mm/zswap.c +2 -0
 		/* 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.

Quoted 5 lines

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.

Signature · 9 lines

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 [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

James Hogan

2013-12-09 10:19 UTC #3 lore raw

Quoted 9 lines · Chen Gang

Quoted 5 lines
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

Chen Gang

author

2013-12-09 11:19 UTC #4 lore raw

Quoted 7 lines · James Hogan

Quoted 6 lines · Chen Gang
Quoted 5 lines
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.

Quoted 8 lines

Quoted 3 lines
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.

Signature · 3 lines

Chen Gang

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

mm/zswap.c +1 -2

index 5a63f78..1853ef4 100644
@@ -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.
 		 */

James Hogan

2013-12-09 11:41 UTC #5 lore raw

Quoted 3 lines · Chen Gang

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

Chen Gang

author

2013-12-09 13:34 UTC #6 lore raw

Quoted 9 lines · James Hogan

Quoted 3 lines · Chen Gang
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.

Quoted 4 lines

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

Signature · 7 lines

Chen Gang

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

Seth Jennings

2013-12-09 15:36 UTC #7 lore raw

In reply to Chen Gang

Quoted 20 lines · 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 <[email protected]>
---
 mm/zswap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
mm/zswap.c +0 -0
index 5a63f78..bfd1807 100644

mm/zswap.c static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle) · line 589

Quoted 5 lines

mm/zswap.c +2 -0

@@ -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

Quoted 11 lines

mm/zswap.c +0 -0

 	}
 
 	/* move it to the tail of the inactive list after end_writeback */

Signature · 7 lines

1.7.7.6

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

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

Chen Gang

author

2013-12-10 02:14 UTC #8 lore raw

Quoted 22 lines · Seth Jennings

Quoted 21 lines · 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 <[email protected]>
---
 mm/zswap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
mm/zswap.c +0 -0
index 5a63f78..bfd1807 100644
@@ -585,6 +585,8 @@ static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle)
 

Quoted 8 lines

mm/zswap.c static int zswap_writeback_entry(struct zbud_pool *pool, unsigned long handle) · line 589
Quoted 4 lines
mm/zswap.c +2 -0
 		/* 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.

Signature · 9 lines

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 [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Chen Gang

author

2013-12-11 02:09 UTC #9 lore raw

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

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 <[email protected]>

---
 mm/zswap.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

mm/zswap.c +4 -0

index 5a63f78..f58a001 100644
@@ -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 */

Signature · 7 lines

1.7.7.6

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