| Message ID | e0192694760936031cae7b4633ba738506eacdc1.1693930391.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some -Wmaybe-uninitialized errors | expand |
Tested-by: Jens Axboe <axboe@kernel.dk>
On 2023/9/6 00:15, Josef Bacik wrote: > Jens reported a compiler warning when using > CONFIG_CC_OPTIMIZE_FOR_SIZE=y that looks like this > > fs/btrfs/tree-log.c: In function ‘btrfs_log_prealloc_extents’: > fs/btrfs/tree-log.c:4828:23: warning: ‘start_slot’ may be used > uninitialized [-Wmaybe-uninitialized] > 4828 | ret = copy_items(trans, inode, dst_path, path, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 4829 | start_slot, ins_nr, 1, 0); > | ~~~~~~~~~~~~~~~~~~~~~~~~~ > fs/btrfs/tree-log.c:4725:13: note: ‘start_slot’ was declared here > 4725 | int start_slot; > | ^~~~~~~~~~ > > The compiler is incorrect, as we only use this code when ins_len > 0, > and when ins_len > 0 we have start_slot properly initialized. However > we generally find the -Wmaybe-uninitialized warnings valuable, so > initialize start_slot to get rid of the warning. > > Reported-by: Jens Axboe <axboe@kernel.dk> > Signed-off-by: Josef Bacik <josef@toxicpanda.com> I think we're in a dilemma, if we don't do the initialization, bad compiler may warn. But if we do the initialization, some static checker may also warn... Thanks, Qu > --- > 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 d1e46b839519..cbb17b542131 100644 > --- a/fs/btrfs/tree-log.c > +++ b/fs/btrfs/tree-log.c > @@ -4722,7 +4722,7 @@ static int btrfs_log_prealloc_extents(struct btrfs_trans_handle *trans, > struct extent_buffer *leaf; > int slot; > int ins_nr = 0; > - int start_slot; > + int start_slot = 0; > int ret; > > if (!(inode->flags & BTRFS_INODE_PREALLOC))
On Wed, Sep 06, 2023 at 06:51:26AM +0800, Qu Wenruo wrote: > > > On 2023/9/6 00:15, Josef Bacik wrote: > > Jens reported a compiler warning when using > > CONFIG_CC_OPTIMIZE_FOR_SIZE=y that looks like this > > > > fs/btrfs/tree-log.c: In function ‘btrfs_log_prealloc_extents’: > > fs/btrfs/tree-log.c:4828:23: warning: ‘start_slot’ may be used > > uninitialized [-Wmaybe-uninitialized] > > 4828 | ret = copy_items(trans, inode, dst_path, path, > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > 4829 | start_slot, ins_nr, 1, 0); > > | ~~~~~~~~~~~~~~~~~~~~~~~~~ > > fs/btrfs/tree-log.c:4725:13: note: ‘start_slot’ was declared here > > 4725 | int start_slot; > > | ^~~~~~~~~~ > > > > The compiler is incorrect, as we only use this code when ins_len > 0, > > and when ins_len > 0 we have start_slot properly initialized. However > > we generally find the -Wmaybe-uninitialized warnings valuable, so > > initialize start_slot to get rid of the warning. > > > > Reported-by: Jens Axboe <axboe@kernel.dk> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > I think we're in a dilemma, if we don't do the initialization, bad > compiler may warn. > > But if we do the initialization, some static checker may also warn... I've seen static checkers to accept variables initialized to 0 and unused, there's a lot of code that does that as a matter of coding style so there would be too many warnings for a generally good practice. If we see a warning from something else than compiler then we can reconsider the way it's fixed but I think we'd see more compiler reports than static checker reports.
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index d1e46b839519..cbb17b542131 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c @@ -4722,7 +4722,7 @@ static int btrfs_log_prealloc_extents(struct btrfs_trans_handle *trans, struct extent_buffer *leaf; int slot; int ins_nr = 0; - int start_slot; + int start_slot = 0; int ret; if (!(inode->flags & BTRFS_INODE_PREALLOC))
Jens reported a compiler warning when using CONFIG_CC_OPTIMIZE_FOR_SIZE=y that looks like this fs/btrfs/tree-log.c: In function ‘btrfs_log_prealloc_extents’: fs/btrfs/tree-log.c:4828:23: warning: ‘start_slot’ may be used uninitialized [-Wmaybe-uninitialized] 4828 | ret = copy_items(trans, inode, dst_path, path, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 4829 | start_slot, ins_nr, 1, 0); | ~~~~~~~~~~~~~~~~~~~~~~~~~ fs/btrfs/tree-log.c:4725:13: note: ‘start_slot’ was declared here 4725 | int start_slot; | ^~~~~~~~~~ The compiler is incorrect, as we only use this code when ins_len > 0, and when ins_len > 0 we have start_slot properly initialized. However we generally find the -Wmaybe-uninitialized warnings valuable, so initialize start_slot to get rid of the warning. Reported-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/tree-log.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)