[PATCH] mac80211: Fix clang warning about constant operand in logical operation

[PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Matthias Kaehlcke]

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:
 	kernel_param_unlock(THIS_MODULE);
 
 	return ops;
-- 
2.12.2.715.g7642488e1d-goog

[PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Matthias Kaehlcke]

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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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:
 	kernel_param_unlock(THIS_MODULE);
 
 	return ops;
-- 
2.12.2.715.g7642488e1d-goog

Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Johannes Berg]

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

Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Johannes Berg]

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

Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Matthias Kaehlcke]

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

Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Johannes Berg]

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

Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Johannes Berg]

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

Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Matthias Kaehlcke]

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

Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Johannes Berg]

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

Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Matthias Kaehlcke]

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

RE: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

[David Laight]

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