Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode
From: Qu Wenruo
Date: Wed May 17 2023 - 05:50:22 EST
On 2023/5/17 17:07, Stephen Zhang wrote:
Qu Wenruo <quwenruo.btrfs@xxxxxxx> 于2023年5月17日周三 15:47写道：
On 2023/5/16 09:34, zhangshida wrote:
From: Shida Zhang <zhangshida@xxxxxxxxxx>
From: Shida Zhang <zhangshida@xxxxxxxxxx>
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@xxxxxxxxxx/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@xxxxxxxxxx>
Signed-off-by: Shida Zhang <zhangshida@xxxxxxxxxx>
---
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;