[PATCH] btrfs: Turn an 'else if' into an 'else' in btrfs_uuid_tree_add

[PATCH] btrfs: Turn an 'else if' into an 'else' in btrfs_uuid_tree_add

[Nathan Chancellor]

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(-)

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,
-- 
2.21.0

Re: [PATCH] btrfs: Turn an 'else if' into an 'else' in btrfs_uuid_tree_add

[David Sterba]

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.

Re: [PATCH] btrfs: Turn an 'else if' into an 'else' in btrfs_uuid_tree_add

[Nick Desaulniers]

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!
-- 
Thanks,
~Nick Desaulniers