| Message ID | 20190307163515.2168-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Turn an 'else if' into an 'else' in btrfs_uuid_tree_add | expand |
On Thu, Mar 07, 2019 at 09:35:15AM -0700, Nathan Chancellor wrote: > When building with -Wsometimes-uninitialized, Clang warns: > > fs/btrfs/uuid-tree.c:129:13: warning: variable 'eb' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] > fs/btrfs/uuid-tree.c:129:13: warning: variable 'offset' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] > > Clang can't tell that all cases are covered with this final else if. The chain of conditions is if (ret >= 0) else if (ret == -EEXIST) else if (ret < 0) I'd think that compiler should be able to somehow represent the coverage of the ranges and report if it's not complete and leading to some uninitialized variables. What if the last condition is 'ret < -EINVAL' obscuring the numerical value, it'd be easy to miss that there are 20 unhandled values. In such case the final 'else' would be the right thing to do. Anyway, I'll apply the patch because it makes the code easier to follow. Thanks.
On Thu, Mar 7, 2019 at 9:54 AM David Sterba <dsterba@suse.cz> wrote: > > On Thu, Mar 07, 2019 at 09:35:15AM -0700, Nathan Chancellor wrote: > > When building with -Wsometimes-uninitialized, Clang warns: > > > > fs/btrfs/uuid-tree.c:129:13: warning: variable 'eb' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] > > fs/btrfs/uuid-tree.c:129:13: warning: variable 'offset' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] > > > > Clang can't tell that all cases are covered with this final else if. > > The chain of conditions is > > if (ret >= 0) > else if (ret == -EEXIST) > else if (ret < 0) In the few cases we looked at, it seemed that the compiler's heuristic for coverage doesn't try very hard. I assume once you start having more complicated expressions is gets quite difficult to prove. Thanks for the patch Nathan!
diff --git a/fs/btrfs/uuid-tree.c b/fs/btrfs/uuid-tree.c index 3b2ae342e649..c1cc9a5c0024 100644 --- a/fs/btrfs/uuid-tree.c +++ b/fs/btrfs/uuid-tree.c @@ -126,7 +126,7 @@ int btrfs_uuid_tree_add(struct btrfs_trans_handle *trans, u8 *uuid, u8 type, slot = path->slots[0]; offset = btrfs_item_ptr_offset(eb, slot); offset += btrfs_item_size_nr(eb, slot) - sizeof(subid_le); - } else if (ret < 0) { + } else { btrfs_warn(fs_info, "insert uuid item failed %d (0x%016llx, 0x%016llx) type %u!", ret, (unsigned long long)key.objectid,
When building with -Wsometimes-uninitialized, Clang warns: fs/btrfs/uuid-tree.c:129:13: warning: variable 'eb' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] fs/btrfs/uuid-tree.c:129:13: warning: variable 'offset' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] Clang can't tell that all cases are covered with this final else if. Just turn it into an else so that it is clear. Link: https://github.com/ClangBuiltLinux/linux/issues/385 Suggested-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- fs/btrfs/uuid-tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)