On 12/28/21 10:55 AM, Nick Desaulniers wrote:

On Fri, Dec 24, 2021 at 6:01 AM Tom Rix [off-list ref] wrote:

quoted

On 12/23/21 12:30 PM, Nick Desaulniers wrote:

quoted

On Thu, Dec 23, 2021 at 8:29 AM [off-list ref] wrote:

quoted

From: Tom Rix <trix@redhat.com>

Clang static analysis reports this warnings

mlme.c:5332:7: warning: Branch condition evaluates to a
    garbage value
      have_higher_than_11mbit)
      ^~~~~~~~~~~~~~~~~~~~~~~

have_higher_than_11mbit is only set to true some of the time in
ieee80211_get_rates() but is checked all of the time.  So
have_higher_than_11mbit needs to be initialized to false.

LGTM. There's only one caller of ieee80211_get_rates() today; if there
were others, they could make a similar mistake in the future. An
alternate approach: ieee80211_get_rates() could unconditionally write
false before the loop that could later write true. Then call sites
don't need to worry about this conditional assignment. Perhaps that
would be preferable? If not:

The have_higher_than_11mbit variable had previously be initialized to false.

The commit 5d6a1b069b7f moved the variable without initializing.

I'm not disagreeing with that.

My point is that these sometimes uninitialized warnings you're
finding+fixing with clang static analyzer are demonstrating a
recurring pattern with code.

When _not_ using the static analyzer, -Wuninitialized and
-Wsometimes-uninitialized work in Clang by building a control flow
graph, but they only analyze a function locally.

For example, consider the following code:

_Bool is_thursday(void);
void hello(int);

void init (int* x) {
   if (is_thursday())
     *x = 1;
}

void foo (void) {
   int x;
   init(&x);
   hello(x);
}

(Clang+GCC today will warn on the above; x is considered to "escape"
the scope of foo as init could write the address of x to a global.
Instead clang's static analyzer will take the additional time to
analyze the callee.  But here's a spooky question: what happens when
init is in another translation unit? IIRC, the static analyzer doesn't
do cross TU analysis; I could be wrong though, I haven't run it in a
while.)

My point is that you're sending patches initializing x, when I think
it might be nicer to instead have functions like init always write a
value (unconditionally, rather than conditionally).  That way other
callers of init don't have to worry about sometimes initialized
variables.

The variable is passed to only to the static function ieee80211_get_rates().

Tom

quoted

Tom

quoted

Reviewed-by: Nick Desaulniers <redacted>

quoted

Fixes: 5d6a1b069b7f ("mac80211: set basic rates earlier")
Signed-off-by: Tom Rix <trix@redhat.com>
---
   net/mac80211/mlme.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 51f55c4ee3c6e..766cbbc9c3a72 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c

@@ -5279,7 +5279,7 @@ static int ieee80211_prep_connection(struct ieee80211_sub_if_data *sdata,
           */
          if (new_sta) {
                  u32 rates = 0, basic_rates = 0;
-               bool have_higher_than_11mbit;
+               bool have_higher_than_11mbit = false;
                  int min_rate = INT_MAX, min_rate_index = -1;
                  const struct cfg80211_bss_ies *ies;
                  int shift = ieee80211_vif_get_shift(&sdata->vif);
--

2.26.3