[PATCH v2 0/2] eliminate the uninitialized compilation warning

[PATCH v2 0/2] eliminate the uninitialized compilation warning

[Hongbo Li]

Eliminating the uninitialized compilation warning in bcachefs-tools.

Hongbo Li (2):
  bcachefs: eliminate the uninitialized compilation warning in
    bch2_reconstruct_snapshots
  bcachefs: eliminate the uninitialized compilation warning in
    __do_six_trylock

 fs/bcachefs/six.c      | 5 ++---
 fs/bcachefs/snapshot.c | 3 ++-
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots

[Hongbo Li]

When compiling the bcachefs-tools, the following compilation warning
is reported:
    libbcachefs/snapshot.c: In function ‘bch2_reconstruct_snapshots’:
    libbcachefs/snapshot.c:915:19: warning: ‘tree_id’ may be used uninitialized in this function [-Wmaybe-uninitialized]
      915 |  snapshot->v.tree = cpu_to_le32(tree_id);
    libbcachefs/snapshot.c:903:6: note: ‘tree_id’ was declared here
      903 |  u32 tree_id;
       |      ^~~~~~~

This is a false alert, because @tree_id is changed in
bch2_snapshot_tree_create after it returns 0. And if this function
returns other value, @tree_id wouldn't be used. Thus there should
be nothing wrong in logical.

Although the report itself is a false alert, we can still make it more
explicit by setting the initial value of @tree_id to 0 (an invalid
tree ID).

Fixes: a292be3b68f3 ("bcachefs: Reconstruct missing snapshot nodes")
Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
v2:
  - Revise the code based on Kent's reviews.

v1: https://lore.kernel.org/all/20240419074851.1583392-2-lihongbo22@huawei.com/T/#u
---
---
 fs/bcachefs/snapshot.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/bcachefs/snapshot.c b/fs/bcachefs/snapshot.c
index 2368218070d4..3aa4686cc71f 100644
--- a/fs/bcachefs/snapshot.c
+++ b/fs/bcachefs/snapshot.c
@@ -900,7 +900,8 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id)
 	if (bch2_snapshot_equiv(c, id))
 		return 0;
 
-	u32 tree_id;
+	/* 0 is an invalid tree ID */
+	u32 tree_id = 0;
 	int ret = bch2_snapshot_tree_create(trans, id, 0, &tree_id);
 	if (ret)
 		return ret;
-- 
2.34.1

[PATCH v2 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock

[Hongbo Li]

When compiling the bcachefs-tools, the following compilation warning
is reported:
    libbcachefs/six.c: In function ‘__do_six_trylock’:
    libbcachefs/six.c:90:12: warning: ‘old’ may be used uninitialized in this function [-Wmaybe-uninitialized]
       90 |  if (!(old & SIX_LOCK_HELD_intent)) {
        |       ~~~~~^~~~~~~~~~~~~~~~~~~~~~~

This is also a false altert. Only when @type=SIX_LOCK_write and @try=false
are passed in __do_six_trylock, the second condition branch would enter
which does not initialize the @old variable. But six_set_owner will not
use @old if @type is not SIX_LOCK_intent. There should be nothing wrong
in logical too.

Although the report itself is a false alert, we can elimate the unitialize
compilation warning by assigning @old in front.

Fixes: 84a37cbf62e0 ("six locks: Wakeup now takes lock on behalf of waiter")
Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
v2:
  - Fix the error about reading lock->state introduced in v1.

v1: https://lore.kernel.org/all/20240419074851.1583392-3-lihongbo22@huawei.com/T/#u
---
---
 fs/bcachefs/six.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/bcachefs/six.c b/fs/bcachefs/six.c
index 3a494c5d1247..8bfd22a6f771 100644
--- a/fs/bcachefs/six.c
+++ b/fs/bcachefs/six.c
@@ -118,11 +118,11 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type,
 			    struct task_struct *task, bool try)
 {
 	int ret;
-	u32 old;
+	u32 old = atomic_read(&lock->state);
 
 	EBUG_ON(type == SIX_LOCK_write && lock->owner != task);
 	EBUG_ON(type == SIX_LOCK_write &&
-		(try != !(atomic_read(&lock->state) & SIX_LOCK_HELD_write)));
+		(try != !(old & SIX_LOCK_HELD_write)));
 
 	/*
 	 * Percpu reader mode:
@@ -182,7 +182,6 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type,
 				ret = -1 - SIX_LOCK_read;
 		}
 	} else {
-		old = atomic_read(&lock->state);
 		do {
 			ret = !(old & l[type].lock_fail);
 			if (!ret || (type == SIX_LOCK_write && !try)) {
-- 
2.34.1

Re: [PATCH v2 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock

[Kent Overstreet]

On Fri, Apr 26, 2024 at 11:21:36AM +0800, Hongbo Li wrote:
> When compiling the bcachefs-tools, the following compilation warning
> is reported:
>     libbcachefs/six.c: In function ‘__do_six_trylock’:
>     libbcachefs/six.c:90:12: warning: ‘old’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>        90 |  if (!(old & SIX_LOCK_HELD_intent)) {
>         |       ~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> 
> This is also a false altert. Only when @type=SIX_LOCK_write and @try=false
> are passed in __do_six_trylock, the second condition branch would enter
> which does not initialize the @old variable. But six_set_owner will not
> use @old if @type is not SIX_LOCK_intent. There should be nothing wrong
> in logical too.
> 
> Although the report itself is a false alert, we can elimate the unitialize
> compilation warning by assigning @old in front.
> 
> Fixes: 84a37cbf62e0 ("six locks: Wakeup now takes lock on behalf of waiter")
> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>

let's just delete the assertion - I don't like moving around reading the
lock state for this.

> ---
> v2:
>   - Fix the error about reading lock->state introduced in v1.
> 
> v1: https://lore.kernel.org/all/20240419074851.1583392-3-lihongbo22@huawei.com/T/#u
> ---
> ---
>  fs/bcachefs/six.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/bcachefs/six.c b/fs/bcachefs/six.c
> index 3a494c5d1247..8bfd22a6f771 100644
> --- a/fs/bcachefs/six.c
> +++ b/fs/bcachefs/six.c
> @@ -118,11 +118,11 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type,
>  			    struct task_struct *task, bool try)
>  {
>  	int ret;
> -	u32 old;
> +	u32 old = atomic_read(&lock->state);
>  
>  	EBUG_ON(type == SIX_LOCK_write && lock->owner != task);
>  	EBUG_ON(type == SIX_LOCK_write &&
> -		(try != !(atomic_read(&lock->state) & SIX_LOCK_HELD_write)));
> +		(try != !(old & SIX_LOCK_HELD_write)));
>  
>  	/*
>  	 * Percpu reader mode:
> @@ -182,7 +182,6 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type,
>  				ret = -1 - SIX_LOCK_read;
>  		}
>  	} else {
> -		old = atomic_read(&lock->state);
>  		do {
>  			ret = !(old & l[type].lock_fail);
>  			if (!ret || (type == SIX_LOCK_write && !try)) {
> -- 
> 2.34.1
> 

Re: [PATCH v2 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots

[Kent Overstreet]

On Fri, Apr 26, 2024 at 11:21:35AM +0800, Hongbo Li wrote:
> When compiling the bcachefs-tools, the following compilation warning
> is reported:
>     libbcachefs/snapshot.c: In function ‘bch2_reconstruct_snapshots’:
>     libbcachefs/snapshot.c:915:19: warning: ‘tree_id’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>       915 |  snapshot->v.tree = cpu_to_le32(tree_id);
>     libbcachefs/snapshot.c:903:6: note: ‘tree_id’ was declared here
>       903 |  u32 tree_id;
>        |      ^~~~~~~
> 
> This is a false alert, because @tree_id is changed in
> bch2_snapshot_tree_create after it returns 0. And if this function
> returns other value, @tree_id wouldn't be used. Thus there should
> be nothing wrong in logical.
> 
> Although the report itself is a false alert, we can still make it more
> explicit by setting the initial value of @tree_id to 0 (an invalid
> tree ID).
> 
> Fixes: a292be3b68f3 ("bcachefs: Reconstruct missing snapshot nodes")
> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>

this works - applied

> ---
> v2:
>   - Revise the code based on Kent's reviews.
> 
> v1: https://lore.kernel.org/all/20240419074851.1583392-2-lihongbo22@huawei.com/T/#u
> ---
> ---
>  fs/bcachefs/snapshot.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/bcachefs/snapshot.c b/fs/bcachefs/snapshot.c
> index 2368218070d4..3aa4686cc71f 100644
> --- a/fs/bcachefs/snapshot.c
> +++ b/fs/bcachefs/snapshot.c
> @@ -900,7 +900,8 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id)
>  	if (bch2_snapshot_equiv(c, id))
>  		return 0;
>  
> -	u32 tree_id;
> +	/* 0 is an invalid tree ID */
> +	u32 tree_id = 0;
>  	int ret = bch2_snapshot_tree_create(trans, id, 0, &tree_id);
>  	if (ret)
>  		return ret;
> -- 
> 2.34.1
> 

Re: [PATCH v2 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock

[Hongbo Li]

On 2024/4/26 12:04, Kent Overstreet wrote:
> On Fri, Apr 26, 2024 at 11:21:36AM +0800, Hongbo Li wrote:
>> When compiling the bcachefs-tools, the following compilation warning
>> is reported:
>>      libbcachefs/six.c: In function ‘__do_six_trylock’:
>>      libbcachefs/six.c:90:12: warning: ‘old’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>         90 |  if (!(old & SIX_LOCK_HELD_intent)) {
>>          |       ~~~~~^~~~~~~~~~~~~~~~~~~~~~~
>>
>> This is also a false altert. Only when @type=SIX_LOCK_write and @try=false
>> are passed in __do_six_trylock, the second condition branch would enter
>> which does not initialize the @old variable. But six_set_owner will not
>> use @old if @type is not SIX_LOCK_intent. There should be nothing wrong
>> in logical too.
>>
>> Although the report itself is a false alert, we can elimate the unitialize
>> compilation warning by assigning @old in front.
>>
>> Fixes: 84a37cbf62e0 ("six locks: Wakeup now takes lock on behalf of waiter")
>> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> 
> let's just delete the assertion - I don't like moving around reading the
> lock state for this.
ok, thank you, and I will keep "old = atomic_read(&lock->state);" in the 
following else branch for next version.
> 
>> ---
>> v2:
>>    - Fix the error about reading lock->state introduced in v1.
>>
>> v1: https://lore.kernel.org/all/20240419074851.1583392-3-lihongbo22@huawei.com/T/#u
>> ---
>> ---
>>   fs/bcachefs/six.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/bcachefs/six.c b/fs/bcachefs/six.c
>> index 3a494c5d1247..8bfd22a6f771 100644
>> --- a/fs/bcachefs/six.c
>> +++ b/fs/bcachefs/six.c
>> @@ -118,11 +118,11 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type,
>>   			    struct task_struct *task, bool try)
>>   {
>>   	int ret;
>> -	u32 old;
>> +	u32 old = atomic_read(&lock->state);
>>   
>>   	EBUG_ON(type == SIX_LOCK_write && lock->owner != task);
>>   	EBUG_ON(type == SIX_LOCK_write &&
>> -		(try != !(atomic_read(&lock->state) & SIX_LOCK_HELD_write)));
>> +		(try != !(old & SIX_LOCK_HELD_write)));
>>   
>>   	/*
>>   	 * Percpu reader mode:
>> @@ -182,7 +182,6 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type,
>>   				ret = -1 - SIX_LOCK_read;
>>   		}
>>   	} else {
>> -		old = atomic_read(&lock->state);
>>   		do {
>>   			ret = !(old & l[type].lock_fail);
>>   			if (!ret || (type == SIX_LOCK_write && !try)) {
>> -- 
>> 2.34.1
>>
>