Page MenuHomePhabricator

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

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
13586–13590

operands (plural)

13591

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

13598–13648

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
13598–13648

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
13583

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?

13636

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

13649–13651

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
13636

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
13584

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?

13588

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...

13588

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() ...

...
13640–13648

...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