[PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[zhangshida]

From: Shida Zhang <zhangshida@kylinos.cn>

From: Shida Zhang <zhangshida@kylinos.cn>

This fixes the following warning reported by gcc 10 under x86_64:

../fs/btrfs/tree-log.c: In function ‘btrfs_log_inode’:
../fs/btrfs/tree-log.c:6211:9: error: ‘last_range_start’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 6211 |   ret = insert_dir_log_key(trans, log, path, key.objectid,
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 6212 |       first_dir_index, last_dir_index);
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../fs/btrfs/tree-log.c:6161:6: note: ‘last_range_start’ was declared here
 6161 |  u64 last_range_start;
      |      ^~~~~~~~~~~~~~~~

Reported-by: k2ci <kernel-bot@kylinos.cn>
Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
---
 fs/btrfs/tree-log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 9b212e8c70cc..d2755d5e338b 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -6158,7 +6158,7 @@ static int log_delayed_deletions_incremental(struct btrfs_trans_handle *trans,
 {
 	struct btrfs_root *log = inode->root->log_root;
 	const struct btrfs_delayed_item *curr;
-	u64 last_range_start;
+	u64 last_range_start = 0;
 	u64 last_range_end = 0;
 	struct btrfs_key key;
 
-- 
2.27.0

Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[Anand Jain]

On 16/5/23 09:34, zhangshida wrote:
> From: Shida Zhang <zhangshida@kylinos.cn>
> 
> From: Shida Zhang <zhangshida@kylinos.cn>
> 
> This fixes the following warning reported by gcc 10 under x86_64:
> 
> ../fs/btrfs/tree-log.c: In function ‘btrfs_log_inode’:
> ../fs/btrfs/tree-log.c:6211:9: error: ‘last_range_start’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   6211 |   ret = insert_dir_log_key(trans, log, path, key.objectid,
>        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   6212 |       first_dir_index, last_dir_index);
>        |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../fs/btrfs/tree-log.c:6161:6: note: ‘last_range_start’ was declared here
>   6161 |  u64 last_range_start;
>        |      ^~~~~~~~~~~~~~~~
> 
> Reported-by: k2ci <kernel-bot@kylinos.cn>
> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
> ---
>   fs/btrfs/tree-log.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 9b212e8c70cc..d2755d5e338b 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -6158,7 +6158,7 @@ static int log_delayed_deletions_incremental(struct btrfs_trans_handle *trans,
>   {
>   	struct btrfs_root *log = inode->root->log_root;
>   	const struct btrfs_delayed_item *curr;
> -	u64 last_range_start;
> +	u64 last_range_start = 0;

  Reviewed-by: Anand Jain <anand.jain@oracle.com>



>   	u64 last_range_end = 0;
>   	struct btrfs_key key;
>   

Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[Qu Wenruo]

On 2023/5/16 09:34, zhangshida wrote:
> From: Shida Zhang <zhangshida@kylinos.cn>
>
> From: Shida Zhang <zhangshida@kylinos.cn>
>
> This fixes the following warning reported by gcc 10 under x86_64:

Full gcc version please.
Especially you need to check if your gcc10 is the latest release.

If newer gcc (12.2.1) tested without such error, it may very possible to
be a false alert.

And in fact it is.

@first_dir_index would only be assigned to @last_range_start if
last_range_end != 0.

Thus the loop must have to be executed once, and @last_range_start won't
be zero.

Please do check your environment (especially your gcc version and
backports), before sending such trivial patches.
Under most cases, it helps nobody.

Thanks,
Qu

>
> ../fs/btrfs/tree-log.c: In function ‘btrfs_log_inode’:
> ../fs/btrfs/tree-log.c:6211:9: error: ‘last_range_start’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   6211 |   ret = insert_dir_log_key(trans, log, path, key.objectid,
>        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   6212 |       first_dir_index, last_dir_index);
>        |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../fs/btrfs/tree-log.c:6161:6: note: ‘last_range_start’ was declared here
>   6161 |  u64 last_range_start;
>        |      ^~~~~~~~~~~~~~~~
>
> Reported-by: k2ci <kernel-bot@kylinos.cn>
> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
> ---
>   fs/btrfs/tree-log.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 9b212e8c70cc..d2755d5e338b 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -6158,7 +6158,7 @@ static int log_delayed_deletions_incremental(struct btrfs_trans_handle *trans,
>   {
>   	struct btrfs_root *log = inode->root->log_root;
>   	const struct btrfs_delayed_item *curr;
> -	u64 last_range_start;
> +	u64 last_range_start = 0;
>   	u64 last_range_end = 0;
>   	struct btrfs_key key;
>

Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[Stephen Zhang]

Qu Wenruo <quwenruo.btrfs@gmx.com> 于2023年5月17日周三 15:47写道：
>
>
>
> On 2023/5/16 09:34, zhangshida wrote:
> > From: Shida Zhang <zhangshida@kylinos.cn>
> >
> > From: Shida Zhang <zhangshida@kylinos.cn>
> >
> > This fixes the following warning reported by gcc 10 under x86_64:
>
> Full gcc version please.

it's "gcc (Debian 10.2.1-6) 10.2.1 20210110".

> Especially you need to check if your gcc10 is the latest release.
>
> If newer gcc (12.2.1) tested without such error, it may very possible to
> be a false alert.
>
> And in fact it is.
>
> @first_dir_index would only be assigned to @last_range_start if
> last_range_end != 0.
>
> Thus the loop must have to be executed once, and @last_range_start won't
> be zero.
>

Yup, I know it's a false positive. What I don't know is the criterion
that decides whether it is a good patch.
That is,
it doesn't look so good because it is a false alert and the latest gcc
can get rid of such warnings, based on what you said( if I understand
correctly).
Or,
It looks okay because the patch can make some older gcc get a cleaner
build and do no harm to the original code logic.
In fact, I've seen Linus complaining about the warning generated by
some gcc version in another thread.

https://lore.kernel.org/linux-xfs/168384265493.22863.2683852857659893778.pr-tracker-bot@kernel.org/T/#t

so it kinda make me feel confused :<

Nonetheless, I appreciate your review.

Thanks,
Shida

> Please do check your environment (especially your gcc version and
> backports), before sending such trivial patches.
> Under most cases, it helps nobody.
>
> Thanks,
> Qu
>
> >
> > ../fs/btrfs/tree-log.c: In function ‘btrfs_log_inode’:
> > ../fs/btrfs/tree-log.c:6211:9: error: ‘last_range_start’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >   6211 |   ret = insert_dir_log_key(trans, log, path, key.objectid,
> >        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   6212 |       first_dir_index, last_dir_index);
> >        |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ../fs/btrfs/tree-log.c:6161:6: note: ‘last_range_start’ was declared here
> >   6161 |  u64 last_range_start;
> >        |      ^~~~~~~~~~~~~~~~
> >
> > Reported-by: k2ci <kernel-bot@kylinos.cn>
> > Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
> > ---
> >   fs/btrfs/tree-log.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> > index 9b212e8c70cc..d2755d5e338b 100644
> > --- a/fs/btrfs/tree-log.c
> > +++ b/fs/btrfs/tree-log.c
> > @@ -6158,7 +6158,7 @@ static int log_delayed_deletions_incremental(struct btrfs_trans_handle *trans,
> >   {
> >       struct btrfs_root *log = inode->root->log_root;
> >       const struct btrfs_delayed_item *curr;
> > -     u64 last_range_start;
> > +     u64 last_range_start = 0;
> >       u64 last_range_end = 0;
> >       struct btrfs_key key;
> >

Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[Qu Wenruo]

On 2023/5/17 17:07, Stephen Zhang wrote:
> Qu Wenruo <quwenruo.btrfs@gmx.com> 于2023年5月17日周三 15:47写道：
>>
>>
>>
>> On 2023/5/16 09:34, zhangshida wrote:
>>> From: Shida Zhang <zhangshida@kylinos.cn>
>>>
>>> From: Shida Zhang <zhangshida@kylinos.cn>
>>>
>>> This fixes the following warning reported by gcc 10 under x86_64:
>>
>> Full gcc version please.
>
> it's "gcc (Debian 10.2.1-6) 10.2.1 20210110".

 From GCC 10 release notes:

 > GCC 10.4
 >     June 28, 2022 (changes, documentation)

Please upgrade to latest 10.x, at least Debian should already have gcc
10.4.x in their repos.

>
>> Especially you need to check if your gcc10 is the latest release.
>>
>> If newer gcc (12.2.1) tested without such error, it may very possible to
>> be a false alert.
>>
>> And in fact it is.
>>
>> @first_dir_index would only be assigned to @last_range_start if
>> last_range_end != 0.
>>
>> Thus the loop must have to be executed once, and @last_range_start won't
>> be zero.
>>
>
> Yup, I know it's a false positive. What I don't know is the criterion
> that decides whether it is a good patch.
> That is,
> it doesn't look so good because it is a false alert and the latest gcc
> can get rid of such warnings, based on what you said( if I understand
> correctly).
> Or,
> It looks okay because the patch can make some older gcc get a cleaner
> build and do no harm to the original code logic.

To me, we want every variable not to be initialized unless necessary, so
compiler can do us a favor to detect unexpected corner cases.

Just unconditionally silent it is not a good way to go, not to mention
the initial value may even be wrong for other cases (not this particular
case).


If you want to fix the situation (other than upgrading your tool chain),
please do it in a way that is also improving the code.

For this particular case, I don't have a particular good suggestion.

But I may introduce a bool to indicate if we have hit any ranges before,
like @has_last_range, then assign no initial value to  @last_range_end.

With this, we bind everything requiring the loop to be executed at least
once to a single bool variable, then maybe older compiler is also able
to detect it's a false alert.

Thanks,
Qu

> In fact, I've seen Linus complaining about the warning generated by
> some gcc version in another thread.
>
> https://lore.kernel.org/linux-xfs/168384265493.22863.2683852857659893778.pr-tracker-bot@kernel.org/T/#t
>
> so it kinda make me feel confused :<
>
> Nonetheless, I appreciate your review.
>
> Thanks,
> Shida
>
>> Please do check your environment (especially your gcc version and
>> backports), before sending such trivial patches.
>> Under most cases, it helps nobody.
>>
>> Thanks,
>> Qu
>>
>>>
>>> ../fs/btrfs/tree-log.c: In function ‘btrfs_log_inode’:
>>> ../fs/btrfs/tree-log.c:6211:9: error: ‘last_range_start’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>    6211 |   ret = insert_dir_log_key(trans, log, path, key.objectid,
>>>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>    6212 |       first_dir_index, last_dir_index);
>>>         |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> ../fs/btrfs/tree-log.c:6161:6: note: ‘last_range_start’ was declared here
>>>    6161 |  u64 last_range_start;
>>>         |      ^~~~~~~~~~~~~~~~
>>>
>>> Reported-by: k2ci <kernel-bot@kylinos.cn>
>>> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
>>> ---
>>>    fs/btrfs/tree-log.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>>> index 9b212e8c70cc..d2755d5e338b 100644
>>> --- a/fs/btrfs/tree-log.c
>>> +++ b/fs/btrfs/tree-log.c
>>> @@ -6158,7 +6158,7 @@ static int log_delayed_deletions_incremental(struct btrfs_trans_handle *trans,
>>>    {
>>>        struct btrfs_root *log = inode->root->log_root;
>>>        const struct btrfs_delayed_item *curr;
>>> -     u64 last_range_start;
>>> +     u64 last_range_start = 0;
>>>        u64 last_range_end = 0;
>>>        struct btrfs_key key;
>>>

Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[David Sterba]

On Wed, May 17, 2023 at 05:07:55PM +0800, Stephen Zhang wrote:
> Qu Wenruo <quwenruo.btrfs@gmx.com> 于2023年5月17日周三 15:47写道：
> > On 2023/5/16 09:34, zhangshida wrote:
> > > From: Shida Zhang <zhangshida@kylinos.cn>
> > >
> > > This fixes the following warning reported by gcc 10 under x86_64:
> >
> > Full gcc version please.
> 
> it's "gcc (Debian 10.2.1-6) 10.2.1 20210110".
> 
> > Especially you need to check if your gcc10 is the latest release.
> >
> > If newer gcc (12.2.1) tested without such error, it may very possible to
> > be a false alert.
> >
> > And in fact it is.
> >
> > @first_dir_index would only be assigned to @last_range_start if
> > last_range_end != 0.
> >
> > Thus the loop must have to be executed once, and @last_range_start won't
> > be zero.
> >
> 
> Yup, I know it's a false positive. What I don't know is the criterion
> that decides whether it is a good patch.

If you have analyzed the code and found out that it was indeed a false
positive then please state that in the changelog. Fixing it still makes
sense so the compiler version and briefly explaining why you fix it that
way makes it a good patch.

> That is,
> it doesn't look so good because it is a false alert and the latest gcc
> can get rid of such warnings, based on what you said( if I understand
> correctly).
> Or,
> It looks okay because the patch can make some older gcc get a cleaner
> build and do no harm to the original code logic.

In general I agree here.

> In fact, I've seen Linus complaining about the warning generated by
> some gcc version in another thread.
> 
> https://lore.kernel.org/linux-xfs/168384265493.22863.2683852857659893778.pr-tracker-bot@kernel.org/T/#t

I share the POV for warning fixes, I'd rather see new reports after
fixing the previous ones than reminding everybody to update.

Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[Qu Wenruo]

On 2023/5/23 05:51, David Sterba wrote:
> On Wed, May 17, 2023 at 05:07:55PM +0800, Stephen Zhang wrote:
>> Qu Wenruo <quwenruo.btrfs@gmx.com> 于2023年5月17日周三 15:47写道：
>>> On 2023/5/16 09:34, zhangshida wrote:
>>>> From: Shida Zhang <zhangshida@kylinos.cn>
>>>>
>>>> This fixes the following warning reported by gcc 10 under x86_64:
>>>
>>> Full gcc version please.
>>
>> it's "gcc (Debian 10.2.1-6) 10.2.1 20210110".
>>
>>> Especially you need to check if your gcc10 is the latest release.
>>>
>>> If newer gcc (12.2.1) tested without such error, it may very possible to
>>> be a false alert.
>>>
>>> And in fact it is.
>>>
>>> @first_dir_index would only be assigned to @last_range_start if
>>> last_range_end != 0.
>>>
>>> Thus the loop must have to be executed once, and @last_range_start won't
>>> be zero.
>>>
>>
>> Yup, I know it's a false positive. What I don't know is the criterion
>> that decides whether it is a good patch.
>
> If you have analyzed the code and found out that it was indeed a false
> positive then please state that in the changelog. Fixing it still makes
> sense so the compiler version and briefly explaining why you fix it that
> way makes it a good patch.
>
>> That is,
>> it doesn't look so good because it is a false alert and the latest gcc
>> can get rid of such warnings, based on what you said( if I understand
>> correctly).
>> Or,
>> It looks okay because the patch can make some older gcc get a cleaner
>> build and do no harm to the original code logic.
>
> In general I agree here.
>
>> In fact, I've seen Linus complaining about the warning generated by
>> some gcc version in another thread.
>>
>> https://lore.kernel.org/linux-xfs/168384265493.22863.2683852857659893778.pr-tracker-bot@kernel.org/T/#t
>
> I share the POV for warning fixes, I'd rather see new reports after
> fixing the previous ones than reminding everybody to update.

Or can we only enable -Wmaybe-uninitialized only for certain builds?
Like binding it with CONFIG_BTRFS_DEBUG?

So far all warning are false alerts, and I'm really not a fan of false
alerts.

The -Wmaybe-uninitialized option doesn't look that reliable on older
compilers, and for developers we're more or less using uptodate
toolchains anyway.

Thanks,
Qu

Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[David Sterba]

On Tue, May 23, 2023 at 06:47:39PM +0800, Qu Wenruo wrote:
> On 2023/5/23 05:51, David Sterba wrote:
> > On Wed, May 17, 2023 at 05:07:55PM +0800, Stephen Zhang wrote:
> >> Qu Wenruo <quwenruo.btrfs@gmx.com> 于2023年5月17日周三 15:47写道：
> >>> On 2023/5/16 09:34, zhangshida wrote:
> >>>> From: Shida Zhang <zhangshida@kylinos.cn>
> >>>>
> >>>> This fixes the following warning reported by gcc 10 under x86_64:
> >>>
> >>> Full gcc version please.
> >>
> >> it's "gcc (Debian 10.2.1-6) 10.2.1 20210110".
> >>
> >>> Especially you need to check if your gcc10 is the latest release.
> >>>
> >>> If newer gcc (12.2.1) tested without such error, it may very possible to
> >>> be a false alert.
> >>>
> >>> And in fact it is.
> >>>
> >>> @first_dir_index would only be assigned to @last_range_start if
> >>> last_range_end != 0.
> >>>
> >>> Thus the loop must have to be executed once, and @last_range_start won't
> >>> be zero.
> >>>
> >>
> >> Yup, I know it's a false positive. What I don't know is the criterion
> >> that decides whether it is a good patch.
> >
> > If you have analyzed the code and found out that it was indeed a false
> > positive then please state that in the changelog. Fixing it still makes
> > sense so the compiler version and briefly explaining why you fix it that
> > way makes it a good patch.
> >
> >> That is,
> >> it doesn't look so good because it is a false alert and the latest gcc
> >> can get rid of such warnings, based on what you said( if I understand
> >> correctly).
> >> Or,
> >> It looks okay because the patch can make some older gcc get a cleaner
> >> build and do no harm to the original code logic.
> >
> > In general I agree here.
> >
> >> In fact, I've seen Linus complaining about the warning generated by
> >> some gcc version in another thread.
> >>
> >> https://lore.kernel.org/linux-xfs/168384265493.22863.2683852857659893778.pr-tracker-bot@kernel.org/T/#t
> >
> > I share the POV for warning fixes, I'd rather see new reports after
> > fixing the previous ones than reminding everybody to update.
> 
> Or can we only enable -Wmaybe-uninitialized only for certain builds?
> Like binding it with CONFIG_BTRFS_DEBUG?
> 
> So far all warning are false alerts, and I'm really not a fan of false
> alerts.

Josef found some real bugs with this warning enabled and then we decided
it would be a good idea to have it enabled for all builds. If we don't
do it by default then the chances that people will use it are low.

> The -Wmaybe-uninitialized option doesn't look that reliable on older
> compilers, and for developers we're more or less using uptodate
> toolchains anyway.

Yeah the warning is speculative so the compilers can report more false
positives. I still have some very old test setups and the compiler gets
updated rarely, IIRC I went from 4.x to 7.x and now there's 10.x so I
might send some warning fixes myself.  Over time I'd like to enable more
warnings just for fs/btrfs/.

Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[Anand Jain]

On 17/5/23 15:46, Qu Wenruo wrote:
> 
> 
> On 2023/5/16 09:34, zhangshida wrote:
>> From: Shida Zhang <zhangshida@kylinos.cn>
>>
>> From: Shida Zhang <zhangshida@kylinos.cn>
>>
>> This fixes the following warning reported by gcc 10 under x86_64:
> 
> Full gcc version please.
> Especially you need to check if your gcc10 is the latest release.
> 
> If newer gcc (12.2.1) tested without such error, it may very possible to
> be a false alert.
> 
> And in fact it is.
> 

I also noticed that last_range_start is not actually uninitialized.
However, it is acceptable to initialize a variable to silence the
compiler if necessary, right? We have done something similar in the
past.

Thanks, Anand

> @first_dir_index would only be assigned to @last_range_start if
> last_range_end != 0.
> 
> Thus the loop must have to be executed once, and @last_range_start won't
> be zero.
> 
> Please do check your environment (especially your gcc version and
> backports), before sending such trivial patches.
> Under most cases, it helps nobody.
> 
> Thanks,
> Qu
> 
>>
>> ../fs/btrfs/tree-log.c: In function ‘btrfs_log_inode’:
>> ../fs/btrfs/tree-log.c:6211:9: error: ‘last_range_start’ may be used 
>> uninitialized in this function [-Werror=maybe-uninitialized]
>>   6211 |   ret = insert_dir_log_key(trans, log, path, key.objectid,
>>        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   6212 |       first_dir_index, last_dir_index);
>>        |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../fs/btrfs/tree-log.c:6161:6: note: ‘last_range_start’ was declared here
>>   6161 |  u64 last_range_start;
>>        |      ^~~~~~~~~~~~~~~~
>>
>> Reported-by: k2ci <kernel-bot@kylinos.cn>
>> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
>> ---
>>   fs/btrfs/tree-log.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>> index 9b212e8c70cc..d2755d5e338b 100644
>> --- a/fs/btrfs/tree-log.c
>> +++ b/fs/btrfs/tree-log.c
>> @@ -6158,7 +6158,7 @@ static int 
>> log_delayed_deletions_incremental(struct btrfs_trans_handle *trans,
>>   {
>>       struct btrfs_root *log = inode->root->log_root;
>>       const struct btrfs_delayed_item *curr;
>> -    u64 last_range_start;
>> +    u64 last_range_start = 0;
>>       u64 last_range_end = 0;
>>       struct btrfs_key key;
>>

Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[Qu Wenruo]

On 2023/5/17 17:13, Anand Jain wrote:
> On 17/5/23 15:46, Qu Wenruo wrote:
>>
>>
>> On 2023/5/16 09:34, zhangshida wrote:
>>> From: Shida Zhang <zhangshida@kylinos.cn>
>>>
>>> From: Shida Zhang <zhangshida@kylinos.cn>
>>>
>>> This fixes the following warning reported by gcc 10 under x86_64:
>>
>> Full gcc version please.
>> Especially you need to check if your gcc10 is the latest release.
>>
>> If newer gcc (12.2.1) tested without such error, it may very possible to
>> be a false alert.
>>
>> And in fact it is.
>>
>
> I also noticed that last_range_start is not actually uninitialized.
> However, it is acceptable to initialize a variable to silence the
> compiler if necessary, right? We have done something similar in the
> past.

I tend not to. Uninitialized variable warning itself is a good indicator
to let compiler help us to expose branches we didn't consider.

Without a no-brainer "int whatever = 0;" we may even hit bugs that some
corner cases may even use that initialized zero, but we didn't even
expect to go.

Thanks,
Qu
>
> Thanks, Anand
>
>> @first_dir_index would only be assigned to @last_range_start if
>> last_range_end != 0.
>>
>> Thus the loop must have to be executed once, and @last_range_start won't
>> be zero.
>>
>> Please do check your environment (especially your gcc version and
>> backports), before sending such trivial patches.
>> Under most cases, it helps nobody.
>>
>> Thanks,
>> Qu
>>
>>>
>>> ../fs/btrfs/tree-log.c: In function ‘btrfs_log_inode’:
>>> ../fs/btrfs/tree-log.c:6211:9: error: ‘last_range_start’ may be used
>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>   6211 |   ret = insert_dir_log_key(trans, log, path, key.objectid,
>>>        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>   6212 |       first_dir_index, last_dir_index);
>>>        |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> ../fs/btrfs/tree-log.c:6161:6: note: ‘last_range_start’ was declared
>>> here
>>>   6161 |  u64 last_range_start;
>>>        |      ^~~~~~~~~~~~~~~~
>>>
>>> Reported-by: k2ci <kernel-bot@kylinos.cn>
>>> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
>>> ---
>>>   fs/btrfs/tree-log.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>>> index 9b212e8c70cc..d2755d5e338b 100644
>>> --- a/fs/btrfs/tree-log.c
>>> +++ b/fs/btrfs/tree-log.c
>>> @@ -6158,7 +6158,7 @@ static int
>>> log_delayed_deletions_incremental(struct btrfs_trans_handle *trans,
>>>   {
>>>       struct btrfs_root *log = inode->root->log_root;
>>>       const struct btrfs_delayed_item *curr;
>>> -    u64 last_range_start;
>>> +    u64 last_range_start = 0;
>>>       u64 last_range_end = 0;
>>>       struct btrfs_key key;
>>>
>

Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[David Sterba]

On Wed, May 17, 2023 at 05:39:09PM +0800, Qu Wenruo wrote:
> On 2023/5/17 17:13, Anand Jain wrote:
> > On 17/5/23 15:46, Qu Wenruo wrote:
> >> On 2023/5/16 09:34, zhangshida wrote:
> >>> From: Shida Zhang <zhangshida@kylinos.cn>
> >>>
> >>> This fixes the following warning reported by gcc 10 under x86_64:
> >>
> >> Full gcc version please.
> >> Especially you need to check if your gcc10 is the latest release.
> >>
> >> If newer gcc (12.2.1) tested without such error, it may very possible to
> >> be a false alert.
> >>
> >> And in fact it is.
> >
> > I also noticed that last_range_start is not actually uninitialized.
> > However, it is acceptable to initialize a variable to silence the
> > compiler if necessary, right? We have done something similar in the
> > past.
> 
> I tend not to. Uninitialized variable warning itself is a good indicator
> to let compiler help us to expose branches we didn't consider.
> 
> Without a no-brainer "int whatever = 0;" we may even hit bugs that some
> corner cases may even use that initialized zero, but we didn't even
> expect to go.

We're in a situation that is not perfect, we got several reports that
were from old compilers but we know most of them were false positives. On
the other hand we want to enable more warnings that make sense to us
(because we can fix/work around them) but can't be yet enabled globally
for linux.

I take the reports or patches as the price for that, so far the number
hasn't been anything near unmanageable. I evaluate each fix in the
context of the code regardless of the compiler, i.e. the code logic must
be correct so it's not a 'no-brainer initialize to 0'. If 0 is the sane
default or detectable invalid value then yes, of course. It might need
some more code restructuring but I don't remember or have an example of
that. Usually just initializing the variable was sufficient.

We cannot expect that everybody has the most up to date version of the
compiler, the minimum currently required for gcc is 5.1
(Documentation/process/changes.rst). I take it as this is the minimum
and anything from that is relevant for reports and fixes. This is what
provides testing coverage, build and runtime.

You can simply skip the warning fix patches, I need to look at them
anyway and I don't mind. I might ask you for an opinion if the suggested
fix is correct in case it's the code I know you do understand well.

Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode

[David Sterba]

On Tue, May 16, 2023 at 09:34:30AM +0800, zhangshida wrote:
> From: Shida Zhang <zhangshida@kylinos.cn>
> 
> From: Shida Zhang <zhangshida@kylinos.cn>
> 
> This fixes the following warning reported by gcc 10 under x86_64:
> 
> ../fs/btrfs/tree-log.c: In function ‘btrfs_log_inode’:
> ../fs/btrfs/tree-log.c:6211:9: error: ‘last_range_start’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>  6211 |   ret = insert_dir_log_key(trans, log, path, key.objectid,
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  6212 |       first_dir_index, last_dir_index);
>       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../fs/btrfs/tree-log.c:6161:6: note: ‘last_range_start’ was declared here
>  6161 |  u64 last_range_start;
>       |      ^~~~~~~~~~~~~~~~
> 
> Reported-by: k2ci <kernel-bot@kylinos.cn>
> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>

Added to misc-next, thanks.