| Message ID | 20170406185633.91065-1-mka@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
On Thu, 2017-04-06 at 11:56 -0700, Matthias Kaehlcke wrote: > Clang raises a warning about the expression 'strlen(CONFIG_XXX)' > being > used in a logical operation. Clangs' builtin strlen function resolves > the > expression to a constant at compile time, which causes clang to > generate > a 'constant-logical-operand' warning. > > Split the if statement in two to avoid using the const expression in > a logical operation. > I don't really see all much point in doing this for the warning's sake... hopefully it doesn't actually generate worse code, but I think the code ends up looking worse and people will forever wonder what the goto is really doing there. johannes
Hi Johannes, thanks for your comments El Thu, Apr 06, 2017 at 09:11:18PM +0200 Johannes Berg ha dit: > On Thu, 2017-04-06 at 11:56 -0700, Matthias Kaehlcke wrote: > > Clang raises a warning about the expression 'strlen(CONFIG_XXX)' > > being > > used in a logical operation. Clangs' builtin strlen function resolves > > the > > expression to a constant at compile time, which causes clang to > > generate > > a 'constant-logical-operand' warning. > > > > Split the if statement in two to avoid using the const expression in > > a logical operation. > > > I don't really see all much point in doing this for the warning's > sake... hopefully it doesn't actually generate worse code, but I think > the code ends up looking worse and people will forever wonder what the > goto is really doing there. I agree that the code looks worse :( I hoped to find a fix using a preprocessor condition but wasn't successful. Some projects (like Chrome OS) build their kernel with all warnings being treated as errors. Besides changing the 'offending' code the alternatives are to disable the warning completely or to tell clang not to use the builtin(s). IMO changing the code is the preferable solution, especially since this is so far the only occurrence of the warning that I have encountered. I used goto instead of nested ifs since other functions in this file use the same pattern. If nested ifs are preferred I can change that. Cheers Matthias
On Thu, 2017-04-06 at 12:24 -0700, Matthias Kaehlcke wrote: > I agree that the code looks worse :( I hoped to find a fix using a > preprocessor condition but wasn't successful. It's actually easy - just remove the 'default ""' from Kconfig, and then the symbol won't be defined at all if it doesn't get a proper value. Then you can ifdef the whole thing. > Some projects (like Chrome OS) build their kernel with all warnings > being treated as errors. Besides changing the 'offending' code the > alternatives are to disable the warning completely or to tell clang > not to use the builtin(s). IMO changing the code is the preferable > solution, especially since this is so far the only occurrence of the > warning that I have encountered. > > I used goto instead of nested ifs since other functions in this file > use the same pattern. If nested ifs are preferred I can change that. I don't really buy either argument. The warning is simply bogus - I'm very surprised you don't hit it with more similar macros or cases, like for example CONFIG_ENABLED(). Try git grep 'IS_ENABLED(' | grep '&&' and you'll find lots of places that seem like they should trigger this warning. You're advocating to make the code worse - not very significantly in this case, but still - just to quiet a compiler warning. johannes
El Thu, Apr 06, 2017 at 11:12:25PM +0200 Johannes Berg ha dit: > On Thu, 2017-04-06 at 12:24 -0700, Matthias Kaehlcke wrote: > > > I agree that the code looks worse :( I hoped to find a fix using a > > preprocessor condition but wasn't successful. > > It's actually easy - just remove the 'default ""' from Kconfig, and > then the symbol won't be defined at all if it doesn't get a proper > value. Then you can ifdef the whole thing. Thanks, it would also require to move the initialization of ieee80211_default_rc_algo into an ifdef. If you can live with such a solution I'm happy to change it. > > Some projects (like Chrome OS) build their kernel with all warnings > > being treated as errors. Besides changing the 'offending' code the > > alternatives are to disable the warning completely or to tell clang > > not to use the builtin(s). IMO changing the code is the preferable > > solution, especially since this is so far the only occurrence of the > > warning that I have encountered. > > > > I used goto instead of nested ifs since other functions in this file > > use the same pattern. If nested ifs are preferred I can change that. > > I don't really buy either argument. The warning is simply bogus - I'm > very surprised you don't hit it with more similar macros or cases, like > for example CONFIG_ENABLED(). Try > > git grep 'IS_ENABLED(' | grep '&&' > > and you'll find lots of places that seem like they should trigger this > warning. Indeed the warning is not triggered by these constructs. It seems clang only emits the warning when the constant operand is not boolean. > You're advocating to make the code worse - not very significantly in > this case, but still - just to quiet a compiler warning. For Chrome OS we need to quiet the warning in one way or another, otherwise our builds will fail due to warnings being treated as errors. I'm reluctant to disable the warning completely since it can be useful to detect certain errors, e.g. the use of a logical operator when bitwise was intended. And yes, in this case I would favor the slight deterioration of a small section of code over losing a potentially useful check on the entire kernel code. I agree that this is a bit of a corner case, the code is definitely not buggy by itself, ideally clang would detect that the constant is a result of its own optimization and skip the warning. Obviously we can always apply a patch locally, but ideally we try to upstream changes that seem of general use so that others and our future selves can benefit from it. I have no intention to insist on this, we can live with a local patch if you don't think it is useful. Cheers Matthias
On Thu, 2017-04-06 at 15:42 -0700, Matthias Kaehlcke wrote: > > Thanks, it would also require to move the initialization of > ieee80211_default_rc_algo into an ifdef. If you can live with such a > solution I'm happy to change it. I think that'd be something I can live with, yeah. > > git grep 'IS_ENABLED(' | grep '&&' > > Indeed the warning is not triggered by these constructs. It seems > clang only emits the warning when the constant operand is not > boolean. That points to just adding "> 0" to the condition here as another alternative solution, I guess? With a comment to make sure it's not removed again, that'd seem like the best thing to do. johannes
El Fri, Apr 07, 2017 at 12:51:57AM +0200 Johannes Berg ha dit: > On Thu, 2017-04-06 at 15:42 -0700, Matthias Kaehlcke wrote: > > > > Thanks, it would also require to move the initialization of > > ieee80211_default_rc_algo into an ifdef. If you can live with such a > > solution I'm happy to change it. > > I think that'd be something I can live with, yeah. > > > > git grep 'IS_ENABLED(' | grep '&&' > > > > Indeed the warning is not triggered by these constructs. It seems > > clang only emits the warning when the constant operand is not > > boolean. > > That points to just adding "> 0" to the condition here as another > alternative solution, I guess? With a comment to make sure it's not > removed again, that'd seem like the best thing to do. Good point, that's more digestible. I'll send an updated change soon. Matthias
From: Matthias Kaehlcke > Sent: 06 April 2017 19:57 > Clang raises a warning about the expression 'strlen(CONFIG_XXX)' being > used in a logical operation. Clangs' builtin strlen function resolves the > expression to a constant at compile time, which causes clang to generate > a 'constant-logical-operand' warning. > > Split the if statement in two to avoid using the const expression in a > logical operation. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > net/mac80211/rate.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c > index 206698bc93f4..68ff202d6380 100644 > --- a/net/mac80211/rate.c > +++ b/net/mac80211/rate.c > @@ -173,9 +173,14 @@ ieee80211_rate_control_ops_get(const char *name) > /* try default if specific alg requested but not found */ > ops = ieee80211_try_rate_control_ops_get(ieee80211_default_rc_algo); > > + if (ops) > + goto unlock; > + > /* try built-in one if specific alg requested but not found */ > - if (!ops && strlen(CONFIG_MAC80211_RC_DEFAULT)) > + if (strlen(CONFIG_MAC80211_RC_DEFAULT)) > ops = ieee80211_try_rate_control_ops_get(CONFIG_MAC80211_RC_DEFAULT); > + > +unlock: Using the result of strlen() as a boolean should itself be a compilation error. You could change to code to the equivalent: if (!ops && CONFIG_MAC80211_RC_DEFAULT[0]) ops = ... David
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c index 206698bc93f4..68ff202d6380 100644 --- a/net/mac80211/rate.c +++ b/net/mac80211/rate.c @@ -173,9 +173,14 @@ ieee80211_rate_control_ops_get(const char *name) /* try default if specific alg requested but not found */ ops = ieee80211_try_rate_control_ops_get(ieee80211_default_rc_algo); + if (ops) + goto unlock; + /* try built-in one if specific alg requested but not found */ - if (!ops && strlen(CONFIG_MAC80211_RC_DEFAULT)) + if (strlen(CONFIG_MAC80211_RC_DEFAULT)) ops = ieee80211_try_rate_control_ops_get(CONFIG_MAC80211_RC_DEFAULT); + +unlock: kernel_param_unlock(THIS_MODULE); return ops;
Clang raises a warning about the expression 'strlen(CONFIG_XXX)' being used in a logical operation. Clangs' builtin strlen function resolves the expression to a constant at compile time, which causes clang to generate a 'constant-logical-operand' warning. Split the if statement in two to avoid using the const expression in a logical operation. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- net/mac80211/rate.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)