| Message ID | 20230516013430.2712449-1-zhangshida@kylinos.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix uninitialized warning in btrfs_log_inode | expand |
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; >
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; >
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; > >
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; >>
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; >>> >
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; >>>
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.
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.
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.
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
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/.
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;