[1/1] btrfs-progs: fix compiler warning
diff mbox

Message ID 1401794959-25907-1-git-send-email-mail@eworm.de
State Rejected
Delegated to: David Sterba
Headers show

Commit Message

Christian Hesse June 3, 2014, 11:29 a.m. UTC
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(-)

Comments

David Sterba June 3, 2014, 4:52 p.m. UTC | #1
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Hesse June 3, 2014, 6:04 p.m. UTC | #2
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?
Qu Wenruo June 4, 2014, 6:48 a.m. UTC | #3
-------- 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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Hesse June 4, 2014, 7:19 a.m. UTC | #4
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
David Sterba June 4, 2014, 4:44 p.m. UTC | #5
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Hesse June 5, 2014, 8:59 a.m. UTC | #6
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?
David Sterba June 10, 2014, 11:05 a.m. UTC | #7
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;