| Message ID | 1401794959-25907-1-git-send-email-mail@eworm.de (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | David Sterba |
| Headers | show |
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
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?
-------- 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
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
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
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?
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
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;
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(-)