[7/9] btrfs-progs: Fix Wmaybe-uninitialized warning
diff mbox series

Message ID 20181116075426.4142-8-wqu@suse.com
State New
Headers show
Series
  • btrfs-progs: Make W=1 clean (no "again")
Related show

Commit Message

Qu WenRuo Nov. 16, 2018, 7:54 a.m. UTC
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(-)

Comments

Nikolay Borisov Nov. 16, 2018, 8:13 a.m. UTC | #1
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;
>
Qu Wenruo Nov. 16, 2018, 8:16 a.m. UTC | #2
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;
>>
David Sterba Dec. 4, 2018, 12:17 p.m. UTC | #3
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.
Adam Borowski Dec. 4, 2018, 12:22 p.m. UTC | #4
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...
Nikolay Borisov Dec. 4, 2018, 12:42 p.m. UTC | #5
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()

>
Qu Wenruo Dec. 5, 2018, 5:38 a.m. UTC | #6
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

Patch
diff mbox series

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;