-Wsometimes-uninitialized in net/wireless/util.c

[nickdesaulniers]

reported from #381 :

net/wireless/util.c:1223:11: error: variable 'result' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]

[nickdesaulniers]

final branch should just be an else (no condition), with the WARN then a return statement. Clang is unhappy because if WARN() expands to something that evaluates to false, then the function wont return correctly.

[nathanchance]

WARN(1, ...) should always be true according to its definition (last line):

With CONFIG_BUG:

#ifndef WARN
#define WARN(condition, format...) ({					\
	int __ret_warn_on = !!(condition);				\
	if (unlikely(__ret_warn_on))					\
		__WARN_printf(format);					\
	unlikely(__ret_warn_on);					\
})
#endif

Without CONFIG_BUG:

#ifndef WARN
#define WARN(condition, format...) ({					\
	int __ret_warn_on = !!(condition);				\
	no_printk(format);						\
	unlikely(__ret_warn_on);					\
})
#endif

However, I assume that all of this analysis occurs before optimization so Clang can't tell that the last else if will basically become an else (would help justify the change). The author was basically trying to conserve space by having the print out come from the conditional then the return be one line. We'll see how this change goes over:

diff --git a/net/wireless/util.c b/net/wireless/util.c
index e4b8db5e81ec..8c381ca22903 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1220,9 +1220,11 @@ static u32 cfg80211_calculate_bitrate_he(struct rate_info *rate)
        else if (rate->bw == RATE_INFO_BW_HE_RU &&
                 rate->he_ru_alloc == NL80211_RATE_INFO_HE_RU_ALLOC_26)
                result = rates_26[rate->he_gi];
-       else if (WARN(1, "invalid HE MCS: bw:%d, ru:%d\n",
-                     rate->bw, rate->he_ru_alloc))
+       else {
+               WARN(1, "invalid HE MCS: bw:%d, ru:%d\n",
+                     rate->bw, rate->he_ru_alloc);
                return 0;
+       }
 
        /* now scale to the appropriate MCS */
        tmp = result;

[nathanchance]

@nickdesaulniers Does the reasoning behind this make sense? Something tells me we're going to get some push back on some of these fixes so I want to make sure to convey that some of these warnings will be from how GCC and Clang differ in when semantic analysis and warnings occur:

From 1fa0abca80a0fdfec70d81ce79c54f03fe6189c7 Mon Sep 17 00:00:00 2001
From: Nathan Chancellor <natechancellor@gmail.com>
Date: Tue, 5 Mar 2019 13:37:00 -0700
Subject: [PATCH] cfg80211: Adjust an else if statement to avoid a clang
 warning

When building with -Wsometimes-uninitialized, Clang warns:

net/wireless/util.c:1223:11: warning: variable 'result' is used
uninitialized whenever 'if' condition is false
[-Wsometimes-uninitialized]
        else if (WARN(1, "invalid HE MCS: bw:%d, ru:%d\n",
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/bug.h:130:36: note: expanded from macro 'WARN'
#define WARN(condition, format...) ({
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/wireless/util.c:1228:8: note: uninitialized use occurs here
        tmp = result;
              ^~~~~~
net/wireless/util.c:1223:7: note: remove the 'if' if its condition is
always true
        else if (WARN(1, "invalid HE MCS: bw:%d, ru:%d\n",
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/wireless/util.c:1187:12: note: initialize the variable 'result' to
silence this warning
        u32 result;
                  ^
                   = 0
1 warning generated.

This is because Clang cannot evaluate at this point that WARN(1, ...)
always evaluates to true (which is why it suggests removing the 'if')
because semantic analysis happens in the front end before optimization
like constant folding occurs in the middle and backend. Turn this
'else if' into an else as it suggests to get rid of this warning.

Link: https://github.com/ClangBuiltLinux/linux/issues/382
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 net/wireless/util.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index e4b8db5e81ec..8c381ca22903 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1220,9 +1220,11 @@ static u32 cfg80211_calculate_bitrate_he(struct rate_info *rate)
 	else if (rate->bw == RATE_INFO_BW_HE_RU &&
 		 rate->he_ru_alloc == NL80211_RATE_INFO_HE_RU_ALLOC_26)
 		result = rates_26[rate->he_gi];
-	else if (WARN(1, "invalid HE MCS: bw:%d, ru:%d\n",
-		      rate->bw, rate->he_ru_alloc))
+	else {
+		WARN(1, "invalid HE MCS: bw:%d, ru:%d\n",
+		      rate->bw, rate->he_ru_alloc);
 		return 0;
+	}
 
 	/* now scale to the appropriate MCS */
 	tmp = result;
-- 
2.21.0

[nickdesaulniers]

I'd cut

This is because Clang cannot evaluate at this point that WARN(1, ...)
always evaluates to true (which is why it suggests removing the 'if')
because semantic analysis happens in the front end before optimization
like constant folding occurs in the middle and backend.

and say something to the effect that

if (WARN(... doesn't make a whole lot of sense and adds noise to otherwise helpful -Wsometimes-uninitialized warnings. else, rather than if else makes it clearer.

idk

[nathanchance]

I don't necessarily agree with the comment about WARN(1, ... not making sense since the condition evaluation (int __ret_warn_on = !!(condition);) is always last in the macro (unlikely(__ret_warn_on);) so it will always be true. I'd like to avoid people coming back and telling us to fix the compiler rather than changing the code but I suppose it's unavoidable in this line of work :) I'll try to come up with a different wording.

[nathanchance]

Patch sent: https://lore.kernel.org/lkml/20190307235735.31487-1-natechancellor@gmail.com/

[arndb]

I opened https://llvm.org/pr41197, I think this is one of those.

[nathanchance]

Patch accepted: https://git.kernel.org/next/linux-next/c/344c9719c508bb3ef4e9c134066c83ff00ab6206

[nathanchance]

Merged into mainline: https://git.kernel.org/torvalds/c/344c9719c508bb3ef4e9c134066c83ff00ab6206