| Message ID | 20181116075426.4142-8-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: Make W=1 clean (no "again") | expand |
On 16.11.18 г. 9:54 ч., Qu Wenruo wrote: > The only location is the following code: > > int level = path->lowest_level + 1; > BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); > while(level < BTRFS_MAX_LEVEL) { > slot = path->slots[level] + 1; > ... > } > path->slots[level] = slot; > > Again, it's the stupid compiler needs some hint for the fact that > we will always enter the while loop for at least once, thus @slot should > always be initialized. > > Fix it by assign level after the BUG_ON(), so the stupid compiler knows > we will always go into the while loop. > > Signed-off-by: Qu Wenruo <wqu@suse.com> I don't get this warning on gcc 7.3. Also from your changelog it's not really clear what the compiler is confused about. Codewise lgtm: Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > ctree.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/ctree.c b/ctree.c > index 46e2ccedc0bf..9c9cb0dfdbf2 100644 > --- a/ctree.c > +++ b/ctree.c > @@ -2961,11 +2961,12 @@ int btrfs_next_sibling_tree_block(struct btrfs_fs_info *fs_info, > struct btrfs_path *path) > { > int slot; > - int level = path->lowest_level + 1; > + int level; > struct extent_buffer *c; > struct extent_buffer *next = NULL; > > BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); > + level = path->lowest_level + 1; > while(level < BTRFS_MAX_LEVEL) { > if (!path->nodes[level]) > return 1; >
On 2018/11/16 下午4:13, Nikolay Borisov wrote: > > > On 16.11.18 г. 9:54 ч., Qu Wenruo wrote: >> The only location is the following code: >> >> int level = path->lowest_level + 1; >> BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); >> while(level < BTRFS_MAX_LEVEL) { >> slot = path->slots[level] + 1; >> ... >> } >> path->slots[level] = slot; >> >> Again, it's the stupid compiler needs some hint for the fact that >> we will always enter the while loop for at least once, thus @slot should >> always be initialized. >> >> Fix it by assign level after the BUG_ON(), so the stupid compiler knows >> we will always go into the while loop. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > I don't get this warning on gcc 7.3. Also from your changelog it's not > really clear what the compiler is confused about. My gcc 8.2.1 reports it with "make W=1" I'll just update the commit message with the original report: ctree.c: In function 'btrfs_next_sibling_tree_block': ctree.c:2990:21: warning: 'slot' may be used uninitialized in this function [-Wmaybe-uninitialized] path->slots[level] = slot; ~~~~~~~~~~~~~~~~~~~^~~~~~ Thanks, Qu > > Codewise lgtm: > > Reviewed-by: Nikolay Borisov <nborisov@suse.com> > >> --- >> ctree.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/ctree.c b/ctree.c >> index 46e2ccedc0bf..9c9cb0dfdbf2 100644 >> --- a/ctree.c >> +++ b/ctree.c >> @@ -2961,11 +2961,12 @@ int btrfs_next_sibling_tree_block(struct btrfs_fs_info *fs_info, >> struct btrfs_path *path) >> { >> int slot; >> - int level = path->lowest_level + 1; >> + int level; >> struct extent_buffer *c; >> struct extent_buffer *next = NULL; >> >> BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); >> + level = path->lowest_level + 1; >> while(level < BTRFS_MAX_LEVEL) { >> if (!path->nodes[level]) >> return 1; >>
On Fri, Nov 16, 2018 at 03:54:24PM +0800, Qu Wenruo wrote: > The only location is the following code: > > int level = path->lowest_level + 1; > BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); > while(level < BTRFS_MAX_LEVEL) { > slot = path->slots[level] + 1; > ... > } > path->slots[level] = slot; > > Again, it's the stupid compiler needs some hint for the fact that > we will always enter the while loop for at least once, thus @slot should > always be initialized. Harsh words for the compiler, and I say not deserved. The same code pasted to kernel a built with the same version does not report the warning, so it's apparently a missing annotation of BUG_ON in btrfs-progs that does not give the right hint.
On Tue, Dec 04, 2018 at 01:17:04PM +0100, David Sterba wrote: > On Fri, Nov 16, 2018 at 03:54:24PM +0800, Qu Wenruo wrote: > > The only location is the following code: > > > > int level = path->lowest_level + 1; > > BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); > > while(level < BTRFS_MAX_LEVEL) { > > slot = path->slots[level] + 1; > > ... > > } > > path->slots[level] = slot; > > > > Again, it's the stupid compiler needs some hint for the fact that > > we will always enter the while loop for at least once, thus @slot should > > always be initialized. > > Harsh words for the compiler, and I say not deserved. The same code > pasted to kernel a built with the same version does not report the > warning, so it's apparently a missing annotation of BUG_ON in > btrfs-progs that does not give the right hint. It's be nice if the C language provided a kind of a while loop that executes at least once...
On 4.12.18 г. 14:22 ч., Adam Borowski wrote: > On Tue, Dec 04, 2018 at 01:17:04PM +0100, David Sterba wrote: >> On Fri, Nov 16, 2018 at 03:54:24PM +0800, Qu Wenruo wrote: >>> The only location is the following code: >>> >>> int level = path->lowest_level + 1; >>> BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); >>> while(level < BTRFS_MAX_LEVEL) { >>> slot = path->slots[level] + 1; >>> ... >>> } >>> path->slots[level] = slot; >>> >>> Again, it's the stupid compiler needs some hint for the fact that >>> we will always enter the while loop for at least once, thus @slot should >>> always be initialized. >> >> Harsh words for the compiler, and I say not deserved. The same code >> pasted to kernel a built with the same version does not report the >> warning, so it's apparently a missing annotation of BUG_ON in >> btrfs-progs that does not give the right hint. > > It's be nice if the C language provided a kind of a while loop that executes > at least once... But it does, it's called: do { } while() >
On 2018/12/4 下午8:17, David Sterba wrote: > On Fri, Nov 16, 2018 at 03:54:24PM +0800, Qu Wenruo wrote: >> The only location is the following code: >> >> int level = path->lowest_level + 1; >> BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); >> while(level < BTRFS_MAX_LEVEL) { >> slot = path->slots[level] + 1; >> ... >> } >> path->slots[level] = slot; >> >> Again, it's the stupid compiler needs some hint for the fact that >> we will always enter the while loop for at least once, thus @slot should >> always be initialized. > > Harsh words for the compiler, and I say not deserved. The same code > pasted to kernel a built with the same version does not report the > warning, so it's apparently a missing annotation of BUG_ON in > btrfs-progs that does not give the right hint. > Well, in fact after the recent gcc8 updates (god knows how many versions gcc8 get updated in Arch after the patchset), it doesn't report this error anymore. But your idea on the BUG_ON() lacking noreturn attribute makes sense. I'll just add some hint for kerncompact. Thanks, Qu
diff --git a/ctree.c b/ctree.c index 46e2ccedc0bf..9c9cb0dfdbf2 100644 --- a/ctree.c +++ b/ctree.c @@ -2961,11 +2961,12 @@ int btrfs_next_sibling_tree_block(struct btrfs_fs_info *fs_info, struct btrfs_path *path) { int slot; - int level = path->lowest_level + 1; + int level; struct extent_buffer *c; struct extent_buffer *next = NULL; BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); + level = path->lowest_level + 1; while(level < BTRFS_MAX_LEVEL) { if (!path->nodes[level]) return 1;
The only location is the following code: int level = path->lowest_level + 1; BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); while(level < BTRFS_MAX_LEVEL) { slot = path->slots[level] + 1; ... } path->slots[level] = slot; Again, it's the stupid compiler needs some hint for the fact that we will always enter the while loop for at least once, thus @slot should always be initialized. Fix it by assign level after the BUG_ON(), so the stupid compiler knows we will always go into the while loop. Signed-off-by: Qu Wenruo <wqu@suse.com> --- ctree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)