This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Fix -Wconstant-logical-operand when LHS is a constant
ClosedPublic

Authored by xgupta on Jan 26 2023, 2:36 AM.

Details

Summary

This fix PR37919

The below code produces -Wconstant-logical-operand for the first statement,
but not the second.

void foo(int x) {
if (x && 5) {}
if (5 && x) {}
}

Diff Detail

Event Timeline

xgupta created this revision.Jan 26 2023, 2:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2023, 2:36 AM
xgupta requested review of this revision.Jan 26 2023, 2:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2023, 2:36 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
xgupta updated this revision to Diff 492374.Jan 26 2023, 2:59 AM

add test cases

WIP && TODO- Fixed test

Failed Tests (7):

Clang :: C/drs/dr4xx.c
Clang :: Modules/explicit-build-extra-files.cpp
Clang :: Parser/cxx2a-concept-declaration.cpp
Clang :: SemaCXX/expressions.cpp
Clang :: SemaCXX/warn-unsequenced.cpp
Clang :: SemaObjC/unguarded-availability.m
Clang :: SemaTemplate/dependent-expr.cpp

Thanks for the patch!

WIP && TODO- Fixed test

Failed Tests (7):

Clang :: C/drs/dr4xx.c
Clang :: Modules/explicit-build-extra-files.cpp
Clang :: Parser/cxx2a-concept-declaration.cpp
Clang :: SemaCXX/expressions.cpp
Clang :: SemaCXX/warn-unsequenced.cpp
Clang :: SemaObjC/unguarded-availability.m
Clang :: SemaTemplate/dependent-expr.cpp

If the current diff is causing test failures, please mark this "Changes Planned" in phabricator (Under the Add Action dropdown in the bottom left) so the reviewers know that there's outstanding issues. Or post but don't cc explicit reviewers yet.

clang/lib/Sema/SemaExpr.cpp
13596–13602

operands (plural)

13601

Please make sure to run git clang-format HEAD~ on your patches.

13611–13672

There seems to be a lot of duplication between the two; should we move these into a static function that's called twice? Perhaps Op1 and Op2 would be better identifiers than LHS RHS in that case?

xgupta planned changes to this revision.Jan 26 2023, 10:06 AM
xgupta updated this revision to Diff 492995.Jan 28 2023, 5:46 AM

update test cases

xgupta updated this revision to Diff 494629.Feb 3 2023, 7:19 AM

clang-format

xgupta updated this revision to Diff 494631.Feb 3 2023, 7:22 AM

remov blank

xgupta added inline comments.Feb 3 2023, 7:41 AM
clang/lib/Sema/SemaExpr.cpp
13611–13672

I have tried many ways, but couldn't figure out a way to do it with function, unrelated test cases started failing.

xbolva00 requested changes to this revision.Feb 3 2023, 9:54 AM
xbolva00 added a subscriber: xbolva00.

Huge code duplication.

This revision now requires changes to proceed.Feb 3 2023, 9:54 AM
xgupta updated this revision to Diff 495116.Feb 6 2023, 7:10 AM

use function to avoid code duplication

xgupta updated this revision to Diff 495122.Feb 6 2023, 7:18 AM

remove old code

nickdesaulniers added inline comments.Feb 6 2023, 9:21 AM
clang/lib/Sema/SemaExpr.cpp
13577

Instead of LHS and RHS, should we rename these to Op1 and Op2 (short for operand one and operand two)? That way there's no confusion since this is called twice with LHS/RHS swapped the second time?

13689

Hmm...I wonder if we now care about which ExprResult contained an enum constant in bool context?

13702–13704

Probably doesn't need a duplicated comment, even with RHS/LHS switched.

xgupta updated this revision to Diff 495375.Feb 6 2023, 9:51 PM

address comments

xgupta marked 5 inline comments as done.Feb 6 2023, 9:59 PM
xgupta added inline comments.
clang/lib/Sema/SemaExpr.cpp
13689

IIUC you mean EnumConstantInBoolContext is not required, but then test cases start failing if I will not use it in diagnoseLogicalInsteadOfBitwise above.
Like this -

$ build/bin/llvm-lit -v clang/test/Sema

Command Output (stderr):

error: 'warning' diagnostics seen but not expected:

File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 52: use of logical '||' with constant operand
File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 56: use of logical '||' with constant operand
File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 68: use of logical '&&' with constant operand

error: 'note' diagnostics seen but not expected:

File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 52: use '|' for a bitwise operation
File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 56: use '|' for a bitwise operation
File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 68: use '&' for a bitwise operation
File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 68: remove constant to silence this warning

7 errors generated.

Failed Tests (1):

Clang :: Sema/warn-int-in-bool-context.c

Testing Time: 0.04s

Failed: 1

gentle ping for review.

I did kernel builds of x86_64 and aarch64 defconfigs. This found new instance:
https://github.com/ClangBuiltLinux/linux/issues/1806
which looks like something we can fix in the kernel sources. Our CI will probably find more instances once this lands, but I'm happy with it.

clang/lib/Sema/SemaExpr.cpp
13578

Every reference to Op1 and Op2 immediately calls .get() on it. That's annoying. How about Sema::diagnoseLogicalInsteadOfBitwise accepts Expr* (or whatever ExprResult::get returns), and we call .get() in the caller?

13598

This is the only use of EnumConstantInBoolContext in Sema::diagnoseLogicalInsteadOfBitwise. It's being used to elide the entirety of the method. In that case, it seems wasteful to bother to pass it as a parameter. Instead, I don't think Sema::diagnoseLogicalInsteadOfBitwise should be called if EnumConstantInBoolContext is true.

If you remove the parameter EnumConstantInBoolContext then...

13598

Do you mind pulling the types into dedicated variables, then reusing them? I kind of hate seeing verbose OpX.get()->getType() so much in this method.

Type T1 = Op1.get()->getType();
Type T2 = Op2.get()->getType();

if (T1->isIntegerType() && !T1->isBooleanType() ...

...
13693–13701

...this can become:

if (EnumConstantInBoolContext) {
  Diag(...
} else {
  diagnoseLogicalInsteadOfBitwise(...
  diagnoseLogicalInsteadOfBitwise(...
}
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index e61b9252901d..47cfd0884911 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -12569,8 +12569,7 @@ public:
       BinaryOperatorKind Opc);
   void diagnoseLogicalInsteadOfBitwise(ExprResult &Op1, ExprResult &Op2,
                                        SourceLocation Loc,
-                                       BinaryOperatorKind Opc,
-                                       bool EnumConstantInBoolContext);
+                                       BinaryOperatorKind Opc);
   QualType CheckLogicalOperands( // C99 6.5.[13,14]
     ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
     BinaryOperatorKind Opc);
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 7acf6f41625e..fa64b0cdbe94 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -13583,9 +13583,8 @@ inline QualType Sema::CheckBitwiseOperands(ExprResult &LHS, ExprResult &RHS,
 // the other is a constant.
 void Sema::diagnoseLogicalInsteadOfBitwise(ExprResult &Op1, ExprResult &Op2,
                                            SourceLocation Loc,
-                                           BinaryOperatorKind Opc,
-                                           bool EnumConstantInBoolContext) {
-  if (!EnumConstantInBoolContext && Op1.get()->getType()->isIntegerType() &&
+                                           BinaryOperatorKind Opc) {
+  if (Op1.get()->getType()->isIntegerType() &&
       !Op1.get()->getType()->isBooleanType() &&
       Op2.get()->getType()->isIntegerType() && !Op2.get()->isValueDependent() &&
       // Don't warn in macros or template instantiations.
@@ -13639,13 +13638,12 @@ inline QualType Sema::CheckLogicalOperands(ExprResult &LHS, ExprResult &RHS,
 
   if (EnumConstantInBoolContext)
     Diag(Loc, diag::warn_enum_constant_in_bool_context);
-
-  // Diagnose cases where the user write a logical and/or but probably meant a
-  // bitwise one.
-  diagnoseLogicalInsteadOfBitwise(LHS, RHS, Loc, Opc,
-                                  EnumConstantInBoolContext);
-  diagnoseLogicalInsteadOfBitwise(RHS, LHS, Loc, Opc,
-                                  EnumConstantInBoolContext);
+  else {
+    // Diagnose cases where the user write a logical and/or but probably meant
+    // a bitwise one.
+    diagnoseLogicalInsteadOfBitwise(LHS, RHS, Loc, Opc);
+    diagnoseLogicalInsteadOfBitwise(RHS, LHS, Loc, Opc);
+  }
 
   if (!Context.getLangOpts().CPlusPlus) {
     // OpenCL v1.1 s6.3.g: The logical operators and (&&), or (||) do

(That diff probably needs to be reformatted...)

Hmm...looking at some of the commits to the related code, it might be very intentional that we don't warn symmetrically:

938533db602b32ab435078e723b656ac6e779a1b
e54ff6cc3e479523b71e4c7eb4bd13707d84de0f

clang/test/SemaCXX/expressions.cpp
146–148

So I think we'll want to change this test.

See commit d6eb2b9f4d4fc236376e3a5a7b8faa31e8dd427d that introduced it.

If we have a constant that was defined via macro, we DONT want to warn for it.

clang/test/SemaCXX/expressions.cpp
146–148

Another related issue is that sometimes we set these constants via -D flags. I wonder if that's a clang bug that those aren't considered as having a valid macro id?

See also https://github.com/ClangBuiltLinux/linux/issues/1806

xgupta updated this revision to Diff 540914.Jul 17 2023, 2:04 AM
xgupta marked 5 inline comments as done.

Address comments

clang/lib/Sema/SemaExpr.cpp
13598

Done by the first comment-
"Every reference to Op1 and Op2 immediately calls .get() on it. That's annoying. How about Sema::diagnoseLogicalInsteadOfBitwise accepts Expr* (or whatever ExprResult::get returns), and we call .get() in the caller?"

13689

I will mark this as done, EnumConstantInBoolContext is required for warn_enum_constant_in_bool_context warning in the below code.

clang/test/SemaCXX/expressions.cpp
146–148

So I think we'll want to change this test.

But it is not failing, what changes you want there?

I wonder if that's a clang bug that those aren't considered as having a valid macro id?

Will it require to address in this patch?

We should not warn for macros.

if (ENABLE_XYZ && x) {}
}

This pattern is used in real world codebases.

xgupta updated this revision to Diff 540922.Jul 17 2023, 2:45 AM

Address xbolva00's comments

xgupta marked 2 inline comments as done.Jul 17 2023, 2:45 AM

I took the most recent version for a spin against the Linux kernel. The couple of issues that a previous revision of the warning flagged (https://github.com/ClangBuiltLinux/linux/issues/1806, https://github.com/ClangBuiltLinux/linux/issues/1807) are no longer visible (that seems intentional because both of those came from macro expressions) but I see a new one along a similar line as those:

https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/gpu/drm/v3d/v3d_drv.h#L343

In file included from drivers/gpu/drm/v3d/v3d_bo.c:25:
drivers/gpu/drm/v3d/v3d_drv.h:343:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
  343 |         if (NSEC_PER_SEC % HZ &&
      |             ~~~~~~~~~~~~~~~~~ ^
drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: use '&' for a bitwise operation
  343 |         if (NSEC_PER_SEC % HZ &&
      |                               ^~
      |                               &
drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: remove constant to silence this warning
1 warning generated.

Another minimized example showing how the warning can trigger with different configurations:

$ cat x.c
#define A 1000
#define B 300
#define C 250
#define D 100

int bar(void);
int baz(void);

int foo(void)
{
        if (A % B && bar()) // 1000 % 300 = 100, warning
                return -3;

        if (A % C && bar()) // 1000 % 250 = 0, no warning
                return -2;

        if (A % D && bar()) // 1000 % 100 = 0, no warning
                return -1;

        return baz();
}

$ clang -Wall -fsyntax-only x.c
x.c:11:12: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
   11 |         if (A % B && bar())
      |             ~~~~~ ^
x.c:11:12: note: use '&' for a bitwise operation
   11 |         if (A % B && bar())
      |                   ^~
      |                   &
x.c:11:12: note: remove constant to silence this warning
1 warning generated.

I am sure we can send a patch making that a boolean expression (although it is duplicated in a few places it seems so multiple patches will be needed) but I figured I would report it just in case there was something that should be changed with the warning, since I see there was already some discussion around not warning for macros and this seems along a similar line.

I took the most recent version for a spin against the Linux kernel. The couple of issues that a previous revision of the warning flagged (https://github.com/ClangBuiltLinux/linux/issues/1806, https://github.com/ClangBuiltLinux/linux/issues/1807) are no longer visible (that seems intentional because both of those came from macro expressions) but I see a new one along a similar line as those:

https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/gpu/drm/v3d/v3d_drv.h#L343

In file included from drivers/gpu/drm/v3d/v3d_bo.c:25:
drivers/gpu/drm/v3d/v3d_drv.h:343:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
  343 |         if (NSEC_PER_SEC % HZ &&
      |             ~~~~~~~~~~~~~~~~~ ^
drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: use '&' for a bitwise operation
  343 |         if (NSEC_PER_SEC % HZ &&
      |                               ^~
      |                               &
drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: remove constant to silence this warning
1 warning generated.

Another minimized example showing how the warning can trigger with different configurations:

$ cat x.c
#define A 1000
#define B 300
#define C 250
#define D 100

int bar(void);
int baz(void);

int foo(void)
{
        if (A % B && bar()) // 1000 % 300 = 100, warning
                return -3;

        if (A % C && bar()) // 1000 % 250 = 0, no warning
                return -2;

        if (A % D && bar()) // 1000 % 100 = 0, no warning
                return -1;

        return baz();
}

$ clang -Wall -fsyntax-only x.c
x.c:11:12: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
   11 |         if (A % B && bar())
      |             ~~~~~ ^
x.c:11:12: note: use '&' for a bitwise operation
   11 |         if (A % B && bar())
      |                   ^~
      |                   &
x.c:11:12: note: remove constant to silence this warning
1 warning generated.

I am sure we can send a patch making that a boolean expression (although it is duplicated in a few places it seems so multiple patches will be needed) but I figured I would report it just in case there was something that should be changed with the warning, since I see there was already some discussion around not warning for macros and this seems along a similar line.

WDYT @xbolva00, It is a valid warning or more modification is required in the patch?

xbolva00 added a comment.EditedJul 18 2023, 7:33 AM

In my opinion it makes sense to adjust that kernel code based on this warning in the current inplementation state.

@aaron.ballman ?

In my opinion it makes sense to adjust that kernel code based on this warning in the current inplementation state.

@aaron.ballman ?

I think that use of macros for any of the constant values in the expression should silence the diagnostic on the assumption that the macro value changes with different configurations and so it's only constant in one configuration mode. Alternatively, I could see an argument to use a different diagnostic group when macros are involved so that users can silence macro-related constant warnings while not losing non-macro cases. WDYT?

In my opinion it makes sense to adjust that kernel code based on this warning in the current inplementation state.

@aaron.ballman ?

I think that use of macros for any of the constant values in the expression should silence the diagnostic on the assumption that the macro value changes with different configurations and so it's only constant in one configuration mode. Alternatively, I could see an argument to use a different diagnostic group when macros are involved so that users can silence macro-related constant warnings while not losing non-macro cases. WDYT?

IIUC in this patch I am making more like an NFC change and this could be a separate issue. WDYT @nickdesaulniers? Please review it so we can commit it soon.

I only see 4 instances of NSEC_PER_SEC % HZ in mainline. 2 already use the C preprocessor (inconsistently), and 2 don't. I think the 2 that don't could be converted to use the preprocessor, then that issue goes away. So I think we can clean that up on the kernel side.

@nathanchance were there many other instances of warnings triggered by this patch beyond that?

I only see 4 instances of NSEC_PER_SEC % HZ in mainline. 2 already use the C preprocessor (inconsistently), and 2 don't. I think the 2 that don't could be converted to use the preprocessor, then that issue goes away. So I think we can clean that up on the kernel side.

Is this diff what you had in mind?

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index 4a33ad2d122b..d7dd93da2511 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -186,9 +186,10 @@ i915_gem_object_wait(struct drm_i915_gem_object *obj,
 static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
 {
        /* nsecs_to_jiffies64() does not guard against overflow */
-       if (NSEC_PER_SEC % HZ &&
-           div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
+#if (NSEC_PER_SEC % HZ) != 0
+       if (div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
                return MAX_JIFFY_OFFSET;
+#endif

        return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
 }
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index b74b1351bfc8..af1470ee477d 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -340,9 +340,10 @@ struct v3d_submit_ext {
 static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
 {
        /* nsecs_to_jiffies64() does not guard against overflow */
-       if (NSEC_PER_SEC % HZ &&
-           div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
+#if (NSEC_PER_SEC % HZ) != 0
+       if (div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
                return MAX_JIFFY_OFFSET;
+#endif

        return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
 }

Not sure how the kernel maintainers will like that, they generally do not like to use preprocessing for hiding warnings. Alternatively, we could just turn the checks into booleans, which is what I tested earlier.

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index 4a33ad2d122b..d4b918fb11ce 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -186,7 +186,7 @@ i915_gem_object_wait(struct drm_i915_gem_object *obj,
 static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
 {
        /* nsecs_to_jiffies64() does not guard against overflow */
-       if (NSEC_PER_SEC % HZ &&
+       if ((NSEC_PER_SEC % HZ) != 0 &&
            div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
                return MAX_JIFFY_OFFSET;
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index b74b1351bfc8..7f664a4b2a75 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -340,7 +340,7 @@ struct v3d_submit_ext {
 static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
 {
        /* nsecs_to_jiffies64() does not guard against overflow */
-       if (NSEC_PER_SEC % HZ &&
+       if ((NSEC_PER_SEC % HZ) != 0 &&
            div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
                return MAX_JIFFY_OFFSET;

I don't really have a strong preference either way but warnings that show up with certain configuration values and not others are annoying for things like randconfig testing, although I suppose that is always the nature of certain warnings...

@nathanchance were there many other instances of warnings triggered by this patch beyond that?

No, those were the only two instances that I saw across my whole build matrix with the current version of the patch.

nickdesaulniers accepted this revision.Jul 18 2023, 12:27 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 18 2023, 11:43 PM
This revision was automatically updated to reflect the committed changes.

Thanks, @nickdesaulniers for reviewing and @nathanchance for testing the change.

@aaron.ballman, I also agree with @xbolva00 seems warning in kernel code is valid but I also agree with your comment about macro, may be better to track the macro-related issue with another GitHub issue.

Thanks, @nickdesaulniers for reviewing and @nathanchance for testing the change.

@aaron.ballman, I also agree with @xbolva00 seems warning in kernel code is valid but I also agree with your comment about macro, may be better to track the macro-related issue with another GitHub issue.

SGTM! I've filed it in https://github.com/llvm/llvm-project/issues/63963

Concerns have been raised in https://github.com/llvm/llvm-project/issues/64356 that this is an undesirable change in diagnostic behavior. The diagnostic is supposed to fire when misusing a logical operand that most likely should have been a bitwise operand. There's a feeling that true && expr is plausibly done intentionally more often than true & expr.

I think we should revert the changes from this patch and in the Clang 17.x branch so that we can reevaluate the approach taken in this patch. CC @porglezomp @cjdb

xgupta added a comment.Aug 7 2023, 8:05 PM

Concerns have been raised in https://github.com/llvm/llvm-project/issues/64356 that this is an undesirable change in diagnostic behavior. The diagnostic is supposed to fire when misusing a logical operand that most likely should have been a bitwise operand. There's a feeling that true && expr is plausibly done intentionally more often than true & expr.

I think we should revert the changes from this patch and in the Clang 17.x branch so that we can reevaluate the approach taken in this patch. CC @porglezomp @cjdb

This got reverted and cherry-pick request is made - https://github.com/llvm/llvm-project/issues/64515.

Concerns have been raised in https://github.com/llvm/llvm-project/issues/64356 that this is an undesirable change in diagnostic behavior. The diagnostic is supposed to fire when misusing a logical operand that most likely should have been a bitwise operand. There's a feeling that true && expr is plausibly done intentionally more often than true & expr.

I think we should revert the changes from this patch and in the Clang 17.x branch so that we can reevaluate the approach taken in this patch. CC @porglezomp @cjdb

This got reverted and cherry-pick request is made - https://github.com/llvm/llvm-project/issues/64515.

Thank you! There's been some more discussion about the design on the github issue, in case you're interested in continuing to pursue this patch.