[PATCH 1/1] btrfs-progs: fix compiler warning

[PATCH 1/1] btrfs-progs: fix compiler warning

[Christian Hesse]

gcc 4.9.0 gives warnings about possibly uninitialized values when
compiling with function inlining and optimization level two enabled
(CFLAGS="-finline-functions -O2").

Initializing the values fixes the warning. Hope this is correct.

Signed-off-by: Christian Hesse <mail@eworm.de>
---
 cmds-send.c   | 2 +-
 send-stream.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmds-send.c b/cmds-send.c
index 1cd457d..0c3e96b 100644
--- a/cmds-send.c
+++ b/cmds-send.c
@@ -416,7 +416,7 @@ int cmd_send(int argc, char **argv)
 	u32 i;
 	char *mount_root = NULL;
 	char *snapshot_parent = NULL;
-	u64 root_id;
+	u64 root_id = 0;
 	u64 parent_root_id = 0;
 	int full_send = 1;
 	int new_end_cmd_semantic = 0;
diff --git a/send-stream.c b/send-stream.c
index 88e18e2..54edafe 100644
--- a/send-stream.c
+++ b/send-stream.c
@@ -216,7 +216,7 @@ static int tlv_get_string(struct btrfs_send_stream *s, int attr, char **str)
 {
 	int ret;
 	void *data;
-	int len;
+	int len = 0;
 
 	TLV_GET(s, attr, &data, &len);
 
-- 
2.0.0

[PATCH 1/1] btrfs-progs: fix compiler warning

[Christian Hesse]

gcc 4.9.0 gives a warning: format ‘%d’ expects argument of type ‘int’,
but argument 2 has type ‘u64’

Using %llu and casting to unsigned long long (same as bytenr) fixes this.

Signed-off-by: Christian Hesse <mail@eworm.de>
---
 btrfs-select-super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/btrfs-select-super.c b/btrfs-select-super.c
index 15e6921..d7cd187 100644
--- a/btrfs-select-super.c
+++ b/btrfs-select-super.c
@@ -100,8 +100,8 @@ int main(int ac, char **av)
 	/* we don't close the ctree or anything, because we don't want a real
 	 * transaction commit.  We just want the super copy we pulled off the
 	 * disk to overwrite all the other copies
-	 */ 
-	printf("using SB copy %d, bytenr %llu\n", num,
+	 */
+	printf("using SB copy %llu, bytenr %llu\n", (unsigned long long)num,
 	       (unsigned long long)bytenr);
 	return ret;
 }
-- 
2.0.0

[PATCH 1/1] btrfs-progs: fix compiler warning

[Christian Hesse]

gcc 4.9.0 gives a warning: array subscript is above array bounds

Checking for "greater or equal" instead of just "equal" fixes this.

Signed-off-by: Christian Hesse <mail@eworm.de>
---
 cmds-restore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmds-restore.c b/cmds-restore.c
index 96b97e1..534a49e 100644
--- a/cmds-restore.c
+++ b/cmds-restore.c
@@ -169,7 +169,7 @@ again:
 			break;
 	}
 
-	if (level == BTRFS_MAX_LEVEL)
+	if (level >= BTRFS_MAX_LEVEL)
 		return 1;
 
 	slot = path->slots[level] + 1;
-- 
2.0.0

Re: [PATCH 1/1] btrfs-progs: fix compiler warning

[David Sterba]

On Tue, Jun 03, 2014 at 01:29:19PM +0200, Christian Hesse wrote:
> gcc 4.9.0 gives a warning: array subscript is above array bounds
> 
> Checking for "greater or equal" instead of just "equal" fixes this.

That fixes the warning, but I don't see the code path that leads to
 level >= BTRFS_MAX_LEVEL

On the first pass, when level = 1 and the first while() reaches at most
BTRFS_MAX_LEVEL, the equality test is enough. So it has to go through
the second while() where level is decremented and in the range
[1..BTRFS_MAX_LEVEL-1] before 'goto again' jumps to the first while
again.

Re: [PATCH 1/1] btrfs-progs: fix compiler warning

[Christian Hesse]

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

David Sterba <dsterba@suse.cz> on Tue, 2014/06/03 18:52:
> On Tue, Jun 03, 2014 at 01:29:19PM +0200, Christian Hesse wrote:
> > gcc 4.9.0 gives a warning: array subscript is above array bounds
> > 
> > Checking for "greater or equal" instead of just "equal" fixes this.
> 
> That fixes the warning, but I don't see the code path that leads to
>  level >= BTRFS_MAX_LEVEL
> 
> On the first pass, when level = 1 and the first while() reaches at most
> BTRFS_MAX_LEVEL, the equality test is enough. So it has to go through
> the second while() where level is decremented and in the range
> [1..BTRFS_MAX_LEVEL-1] before 'goto again' jumps to the first while
> again.

I suppose gcc does not know how much level can be increased within
function next_leaf(). level > BTRFS_MAX_LEVEL is never met at runtime, but
this way gcc knows that level can never be bigger than BTRFS_MAX_LEVEL.

Any better way to fix this?
-- 
Schoene Gruesse
Chris
                         O< ascii ribbon campaign
                   stop html mail - www.asciiribbon.org

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

Re: [PATCH 1/1] btrfs-progs: fix compiler warning

[Qu Wenruo]

-------- Original Message --------
Subject: [PATCH 1/1] btrfs-progs: fix compiler warning
From: Christian Hesse <mail@eworm.de>
To: linux-btrfs@vger.kernel.org
Date: 2014年06月03日 19:29
> gcc 4.9.0 gives a warning: array subscript is above array bounds
>
> Checking for "greater or equal" instead of just "equal" fixes this.
>
> Signed-off-by: Christian Hesse <mail@eworm.de>
> ---
>   cmds-restore.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cmds-restore.c b/cmds-restore.c
> index 96b97e1..534a49e 100644
> --- a/cmds-restore.c
> +++ b/cmds-restore.c
> @@ -169,7 +169,7 @@ again:
>   			break;
>   	}
>   
> -	if (level == BTRFS_MAX_LEVEL)
> +	if (level >= BTRFS_MAX_LEVEL)
>   		return 1;
>   
>   	slot = path->slots[level] + 1;
Also I faied to reproduce the bug.
Using gcc-4.9.0-3 from Archlinux core repo.

It seems to be related to default gcc flags from distribution?

Thanks,
Qu

Re: [PATCH 1/1] btrfs-progs: fix compiler warning

[Christian Hesse]

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

Qu Wenruo <quwenruo@cn.fujitsu.com> on Wed, 2014/06/04 14:48:
> 
> -------- Original Message --------
> Subject: [PATCH 1/1] btrfs-progs: fix compiler warning
> From: Christian Hesse <mail@eworm.de>
> To: linux-btrfs@vger.kernel.org
> Date: 2014年06月03日 19:29
> > gcc 4.9.0 gives a warning: array subscript is above array bounds
> >
> > Checking for "greater or equal" instead of just "equal" fixes this.
> >
> > Signed-off-by: Christian Hesse <mail@eworm.de>
> > ---
> >   cmds-restore.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/cmds-restore.c b/cmds-restore.c
> > index 96b97e1..534a49e 100644
> > --- a/cmds-restore.c
> > +++ b/cmds-restore.c
> > @@ -169,7 +169,7 @@ again:
> >   			break;
> >   	}
> >   
> > -	if (level == BTRFS_MAX_LEVEL)
> > +	if (level >= BTRFS_MAX_LEVEL)
> >   		return 1;
> >   
> >   	slot = path->slots[level] + 1;
>
> Also I faied to reproduce the bug.
> Using gcc-4.9.0-3 from Archlinux core repo.

Exactly the same here. ;)

> It seems to be related to default gcc flags from distribution?

Probably. I did compile with optimization, so adding -O2 may do the trick:

make CFLAGS="${CFLAGS} -O2" all
-- 
Schoene Gruesse
Chris
                         O< ascii ribbon campaign
                   stop html mail - www.asciiribbon.org

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

Re: [PATCH 1/1] btrfs-progs: fix compiler warning

[David Sterba]

On Wed, Jun 04, 2014 at 09:19:26AM +0200, Christian Hesse wrote:
> > It seems to be related to default gcc flags from distribution?
> 
> Probably. I did compile with optimization, so adding -O2 may do the trick:
> 
> make CFLAGS="${CFLAGS} -O2" all

The warning appears with -O2, so the question is if gcc is not able to
reason about the values (ie. a false positive) or if there's a bug that
I don't see.

Re: [PATCH 1/1] btrfs-progs: fix compiler warning

[Christian Hesse]

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

David Sterba <dsterba@suse.cz> on Wed, 2014/06/04 18:44:
> On Wed, Jun 04, 2014 at 09:19:26AM +0200, Christian Hesse wrote:
> > > It seems to be related to default gcc flags from distribution?
> > 
> > Probably. I did compile with optimization, so adding -O2 may do the trick:
> > 
> > make CFLAGS="${CFLAGS} -O2" all
> 
> The warning appears with -O2, so the question is if gcc is not able to
> reason about the values (ie. a false positive) or if there's a bug that
> I don't see.

I do not see a bug either. So probably this is a false positive...

Looks like the warning is triggered as soon as -ftree-vrp is added to CFLAGS.
From gcc man page:

-ftree-vrp
       Perform Value Range Propagation on trees.  This is similar to the
       constant propagation pass, but instead of values, ranges of values are
       propagated.  This allows the optimizers to remove unnecessary range
       checks like array bound checks and null pointer checks.  This is
       enabled by default at -O2 and higher.  Null pointer check elimination
       is only done if -fdelete-null-pointer-checks is enabled.

Is it possibly that gcc optimized away any checks?
-- 
Schoene Gruesse
Chris
                         O< ascii ribbon campaign
                   stop html mail - www.asciiribbon.org

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

Re: [PATCH 1/1] btrfs-progs: fix compiler warning

[David Sterba]

On Thu, Jun 05, 2014 at 10:59:37AM +0200, Christian Hesse wrote:
> David Sterba <dsterba@suse.cz> on Wed, 2014/06/04 18:44:
> > On Wed, Jun 04, 2014 at 09:19:26AM +0200, Christian Hesse wrote:
> > > > It seems to be related to default gcc flags from distribution?
> > > 
> > > Probably. I did compile with optimization, so adding -O2 may do the trick:
> > > 
> > > make CFLAGS="${CFLAGS} -O2" all
> > 
> > The warning appears with -O2, so the question is if gcc is not able to
> > reason about the values (ie. a false positive) or if there's a bug that
> > I don't see.
> 
> I do not see a bug either. So probably this is a false positive...

Yeah I think so.

> Looks like the warning is triggered as soon as -ftree-vrp is added to CFLAGS.

Good catch

$ make CFLAGS="${CFLAGS} -O2 -Wall -fno-tree-vrp" all

does not print the warning (gcc 4.8.1).

> From gcc man page:
> 
> -ftree-vrp
>        Perform Value Range Propagation on trees.  This is similar to the
>        constant propagation pass, but instead of values, ranges of values are
>        propagated.  This allows the optimizers to remove unnecessary range
>        checks like array bound checks and null pointer checks.  This is
>        enabled by default at -O2 and higher.  Null pointer check elimination
>        is only done if -fdelete-null-pointer-checks is enabled.
> 
> Is it possibly that gcc optimized away any checks?

I'm not sure, that way lies gcc optimization maze.

Looking at the assembly from tree-vrp and no-tree-vrp it's probably the
case, the vrp-optimized version is restructured (and shorter) and looks
almost the same. Some of the bounds checks are missing (at least cannot
be easily paired). Better ask gcc people.