<div dir="ltr">Hi,<div><br></div><div>this results in a false positive on webrtc, on this code:</div><div><br></div><div><a href="https://cs.chromium.org/chromium/src/third_party/libwebp/src/enc/picture_csp_enc.c?q=picture_csp_enc.c&sq=package:chromium&dr&l=1002">https://cs.chromium.org/chromium/src/third_party/libwebp/src/enc/picture_csp_enc.c?q=picture_csp_enc.c&sq=package:chromium&dr&l=1002</a><br></div><div><br></div><div>The code does this:</div><div><br></div><div>#ifdef WORDS_BIGENDIAN<br>#define ALPHA_OFFSET 0 // uint32_t 0xff000000 is 0xff,00,00,00 in memory<br>#else<br>#define ALPHA_OFFSET 3 // uint32_t 0xff000000 is 0x00,00,00,ff in memory<br>#endif<br></div><div><br></div><div>// ...</div><div><br></div><div> const uint8_t* const argb = (const uint8_t*)picture->argb;<br> const uint8_t* const a = argb + (0 ^ ALPHA_OFFSET);<br> const uint8_t* const r = argb + (1 ^ ALPHA_OFFSET);<br> const uint8_t* const g = argb + (2 ^ ALPHA_OFFSET);<br> const uint8_t* const b = argb + (3 ^ ALPHA_OFFSET);<br></div><div><br></div><div>The idea is to get bytes 0, 1, 2, 3 as a, r, g, b if ALPHA_OFFSET is 0, or bytes 3, 2, 1, 0 if ALPHA_OFFSET is 3.</div><div><br></div><div>Maybe this shouldn't fire if the rhs is a macro?</div><div><br></div><div>(On all of Chromium, this fired 3 times; in the other 2 cases the rhs was a literal as well, and those two were true positives.)</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Aug 18, 2019 at 3:13 PM David Bolvansky via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: xbolva00<br>
Date: Sun Aug 18 12:14:14 2019<br>
New Revision: 369217<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=369217&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=369217&view=rev</a><br>
Log:<br>
[Diagnostics] Diagnose misused xor as pow<br>
<br>
Summary:<br>
Motivation:<br>
<a href="https://twitter.com/jfbastien/status/1139298419988549632" rel="noreferrer" target="_blank">https://twitter.com/jfbastien/status/1139298419988549632</a><br>
<a href="https://twitter.com/mikemx7f/status/1139335901790625793" rel="noreferrer" target="_blank">https://twitter.com/mikemx7f/status/1139335901790625793</a><br>
<a href="https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=10+%5E&search=Search" rel="noreferrer" target="_blank">https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=10+%5E&search=Search</a><br>
<br>
Reviewers: jfb, rsmith, regehr, aaron.ballman<br>
<br>
Reviewed By: aaron.ballman<br>
<br>
Subscribers: lebedev.ri, Quuxplusone, erik.pilkington, riccibruno, dexonsmith, cfe-commits<br>
<br>
Tags: #clang<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D63423" rel="noreferrer" target="_blank">https://reviews.llvm.org/D63423</a><br>
<br>
Added:<br>
cfe/trunk/test/SemaCXX/warn-xor-as-pow.cpp<br>
Modified:<br>
cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
cfe/trunk/lib/Sema/SemaExpr.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=369217&r1=369216&r2=369217&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=369217&r1=369216&r2=369217&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Sun Aug 18 12:14:14 2019<br>
@@ -512,6 +512,7 @@ def CompareDistinctPointerType : DiagGro<br>
def GNUUnionCast : DiagGroup<"gnu-union-cast">;<br>
def GNUVariableSizedTypeNotAtEnd : DiagGroup<"gnu-variable-sized-type-not-at-end">;<br>
def Varargs : DiagGroup<"varargs">;<br>
+def XorUsedAsPow : DiagGroup<"xor-used-as-pow">;<br>
<br>
def Unsequenced : DiagGroup<"unsequenced">;<br>
// GCC name for -Wunsequenced<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=369217&r1=369216&r2=369217&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=369217&r1=369216&r2=369217&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Sun Aug 18 12:14:14 2019<br>
@@ -3326,6 +3326,15 @@ def warn_address_of_reference_bool_conve<br>
"code; pointer may be assumed to always convert to true">,<br>
InGroup<UndefinedBoolConversion>;<br>
<br>
+def warn_xor_used_as_pow_base_extra : Warning<<br>
+ "result of '%0' is %1; did you mean '%2' (%3)?">,<br>
+ InGroup<XorUsedAsPow>;<br>
+def warn_xor_used_as_pow_base : Warning<<br>
+ "result of '%0' is %1; did you mean '%2'?">,<br>
+ InGroup<XorUsedAsPow>;<br>
+def note_xor_used_as_pow_silence : Note<<br>
+ "replace expression with '%0' to silence this warning">;<br>
+<br>
def warn_null_pointer_compare : Warning<<br>
"comparison of %select{address of|function|array}0 '%1' %select{not |}2"<br>
"equal to a null pointer is always %select{true|false}2">,<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=369217&r1=369216&r2=369217&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=369217&r1=369216&r2=369217&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Sun Aug 18 12:14:14 2019<br>
@@ -11011,6 +11011,107 @@ QualType Sema::CheckVectorCompareOperand<br>
return GetSignedVectorType(vType);<br>
}<br>
<br>
+static void diagnoseXorMisusedAsPow(Sema &S, ExprResult &LHS, ExprResult &RHS,<br>
+ SourceLocation Loc) {<br>
+ // Do not diagnose macros.<br>
+ if (Loc.isMacroID())<br>
+ return;<br>
+<br>
+ bool Negative = false;<br>
+ const auto *LHSInt = dyn_cast<IntegerLiteral>(LHS.get());<br>
+ const auto *RHSInt = dyn_cast<IntegerLiteral>(RHS.get());<br>
+<br>
+ if (!LHSInt)<br>
+ return;<br>
+ if (!RHSInt) {<br>
+ // Check negative literals.<br>
+ if (const auto *UO = dyn_cast<UnaryOperator>(RHS.get())) {<br>
+ if (UO->getOpcode() != UO_Minus)<br>
+ return;<br>
+ RHSInt = dyn_cast<IntegerLiteral>(UO->getSubExpr());<br>
+ if (!RHSInt)<br>
+ return;<br>
+ Negative = true;<br>
+ } else {<br>
+ return;<br>
+ }<br>
+ }<br>
+<br>
+ if (LHSInt->getValue().getBitWidth() != RHSInt->getValue().getBitWidth())<br>
+ return;<br>
+<br>
+ CharSourceRange ExprRange = CharSourceRange::getCharRange(<br>
+ LHSInt->getBeginLoc(), S.getLocForEndOfToken(RHSInt->getLocation()));<br>
+ llvm::StringRef ExprStr =<br>
+ Lexer::getSourceText(ExprRange, S.getSourceManager(), S.getLangOpts());<br>
+<br>
+ CharSourceRange XorRange =<br>
+ CharSourceRange::getCharRange(Loc, S.getLocForEndOfToken(Loc));<br>
+ llvm::StringRef XorStr =<br>
+ Lexer::getSourceText(XorRange, S.getSourceManager(), S.getLangOpts());<br>
+ // Do not diagnose if xor keyword/macro is used.<br>
+ if (XorStr == "xor")<br>
+ return;<br>
+<br>
+ const llvm::APInt &LeftSideValue = LHSInt->getValue();<br>
+ const llvm::APInt &RightSideValue = RHSInt->getValue();<br>
+ const llvm::APInt XorValue = LeftSideValue ^ RightSideValue;<br>
+<br>
+ std::string LHSStr = Lexer::getSourceText(<br>
+ CharSourceRange::getTokenRange(LHSInt->getSourceRange()),<br>
+ S.getSourceManager(), S.getLangOpts());<br>
+ std::string RHSStr = Lexer::getSourceText(<br>
+ CharSourceRange::getTokenRange(RHSInt->getSourceRange()),<br>
+ S.getSourceManager(), S.getLangOpts());<br>
+<br>
+ int64_t RightSideIntValue = RightSideValue.getSExtValue();<br>
+ if (Negative) {<br>
+ RightSideIntValue = -RightSideIntValue;<br>
+ RHSStr = "-" + RHSStr;<br>
+ }<br>
+<br>
+ StringRef LHSStrRef = LHSStr;<br>
+ StringRef RHSStrRef = RHSStr;<br>
+ // Do not diagnose binary, hexadecimal, octal literals.<br>
+ if (LHSStrRef.startswith("0b") || LHSStrRef.startswith("0B") ||<br>
+ RHSStrRef.startswith("0b") || RHSStrRef.startswith("0B") ||<br>
+ LHSStrRef.startswith("0x") || LHSStrRef.startswith("0X") ||<br>
+ RHSStrRef.startswith("0x") || RHSStrRef.startswith("0X") ||<br>
+ (LHSStrRef.size() > 1 && LHSStrRef.startswith("0")) ||<br>
+ (RHSStrRef.size() > 1 && RHSStrRef.startswith("0")))<br>
+ return;<br>
+<br>
+ if (LeftSideValue == 2 && RightSideIntValue >= 0) {<br>
+ std::string SuggestedExpr = "1 << " + RHSStr;<br>
+ bool Overflow = false;<br>
+ llvm::APInt One = (LeftSideValue - 1);<br>
+ llvm::APInt PowValue = One.sshl_ov(RightSideValue, Overflow);<br>
+ if (Overflow) {<br>
+ if (RightSideIntValue < 64)<br>
+ S.Diag(Loc, diag::warn_xor_used_as_pow_base)<br>
+ << ExprStr << XorValue.toString(10, true) << ("1LL << " + RHSStr)<br>
+ << FixItHint::CreateReplacement(ExprRange, "1LL << " + RHSStr);<br>
+ else<br>
+ // TODO: 2 ^ 64 - 1<br>
+ return;<br>
+ } else {<br>
+ S.Diag(Loc, diag::warn_xor_used_as_pow_base_extra)<br>
+ << ExprStr << XorValue.toString(10, true) << SuggestedExpr<br>
+ << PowValue.toString(10, true)<br>
+ << FixItHint::CreateReplacement(<br>
+ ExprRange, (RightSideIntValue == 0) ? "1" : SuggestedExpr);<br>
+ }<br>
+<br>
+ S.Diag(Loc, diag::note_xor_used_as_pow_silence) << ("0x2 ^ " + RHSStr);<br>
+ } else if (LeftSideValue == 10) {<br>
+ std::string SuggestedValue = "1e" + std::to_string(RightSideIntValue);<br>
+ S.Diag(Loc, diag::warn_xor_used_as_pow_base)<br>
+ << ExprStr << XorValue.toString(10, true) << SuggestedValue<br>
+ << FixItHint::CreateReplacement(ExprRange, SuggestedValue);<br>
+ S.Diag(Loc, diag::note_xor_used_as_pow_silence) << ("0xA ^ " + RHSStr);<br>
+ }<br>
+}<br>
+<br>
QualType Sema::CheckVectorLogicalOperands(ExprResult &LHS, ExprResult &RHS,<br>
SourceLocation Loc) {<br>
// Ensure that either both operands are of the same vector type, or<br>
@@ -11054,6 +11155,9 @@ inline QualType Sema::CheckBitwiseOperan<br>
if (Opc == BO_And)<br>
diagnoseLogicalNotOnLHSofCheck(*this, LHS, RHS, Loc, Opc);<br>
<br>
+ if (Opc == BO_Xor)<br>
+ diagnoseXorMisusedAsPow(*this, LHS, RHS, Loc);<br>
+<br>
ExprResult LHSResult = LHS, RHSResult = RHS;<br>
QualType compType = UsualArithmeticConversions(LHSResult, RHSResult,<br>
IsCompAssign);<br>
@@ -17640,4 +17744,4 @@ ExprResult Sema::ActOnObjCAvailabilityCh<br>
<br>
return new (Context)<br>
ObjCAvailabilityCheckExpr(Version, AtLoc, RParen, Context.BoolTy);<br>
-}<br>
+}<br>
\ No newline at end of file<br>
<br>
Added: cfe/trunk/test/SemaCXX/warn-xor-as-pow.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-xor-as-pow.cpp?rev=369217&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-xor-as-pow.cpp?rev=369217&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/warn-xor-as-pow.cpp (added)<br>
+++ cfe/trunk/test/SemaCXX/warn-xor-as-pow.cpp Sun Aug 18 12:14:14 2019<br>
@@ -0,0 +1,105 @@<br>
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wxor-used-as-pow %s<br>
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s<br>
+// RUN: %clang_cc1 -x c -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s<br>
+<br>
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wxor-used-as-pow %s<br>
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s<br>
+// RUN: %clang_cc1 -x c++ -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s<br>
+<br>
+#define FOOBAR(x, y) (x * y)<br>
+#define XOR(x, y) (x ^ y)<br>
+#define TWO 2<br>
+#define TEN 10<br>
+#define TWO_ULL 2ULL<br>
+#define EPSILON 10 ^ -300<br>
+<br>
+#define flexor 7<br>
+<br>
+#ifdef __cplusplus<br>
+constexpr long long operator"" _xor(unsigned long long v) { return v; }<br>
+<br>
+constexpr long long operator"" _0b(unsigned long long v) { return v; }<br>
+constexpr long long operator"" _0X(unsigned long long v) { return v; }<br>
+#else<br>
+#define xor ^ // iso646.h<br>
+#endif<br>
+<br>
+void test(unsigned a, unsigned b) {<br>
+ unsigned res;<br>
+ res = a ^ 5;<br>
+ res = 2 ^ b;<br>
+ res = a ^ b;<br>
+ res = 2 ^ -1;<br>
+ res = 2 ^ 0; // expected-warning {{result of '2 ^ 0' is 2; did you mean '1 << 0' (1)?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:14}:"1"<br>
+ // expected-note@-2 {{replace expression with '0x2 ^ 0' to silence this warning}}<br>
+ res = 2 ^ 1; // expected-warning {{result of '2 ^ 1' is 3; did you mean '1 << 1' (2)?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:14}:"1 << 1"<br>
+ // expected-note@-2 {{replace expression with '0x2 ^ 1' to silence this warning}}<br>
+ res = 2 ^ 2; // expected-warning {{result of '2 ^ 2' is 0; did you mean '1 << 2' (4)?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:14}:"1 << 2"<br>
+ // expected-note@-2 {{replace expression with '0x2 ^ 2' to silence this warning}}<br>
+ res = 2 ^ 8; // expected-warning {{result of '2 ^ 8' is 10; did you mean '1 << 8' (256)?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:14}:"1 << 8"<br>
+ // expected-note@-2 {{replace expression with '0x2 ^ 8' to silence this warning}}<br>
+ res = TWO ^ 8; // expected-warning {{result of 'TWO ^ 8' is 10; did you mean '1 << 8' (256)?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:16}:"1 << 8"<br>
+ // expected-note@-2 {{replace expression with '0x2 ^ 8' to silence this warning}}<br>
+ res = 2 ^ 16; // expected-warning {{result of '2 ^ 16' is 18; did you mean '1 << 16' (65536)?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:15}:"1 << 16"<br>
+ // expected-note@-2 {{replace expression with '0x2 ^ 16' to silence this warning}}<br>
+ res = 2 ^ TEN; // expected-warning {{result of '2 ^ TEN' is 8; did you mean '1 << TEN' (1024)?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:16}:"1 << TEN"<br>
+ // expected-note@-2 {{replace expression with '0x2 ^ TEN' to silence this warning}}<br>
+ res = 0x2 ^ 16;<br>
+ res = 2 xor 16;<br>
+<br>
+ res = 2 ^ 0x4;<br>
+ res = 2 ^ 04;<br>
+ res = 0x2 ^ 10;<br>
+ res = 0X2 ^ 10;<br>
+ res = 02 ^ 10;<br>
+ res = FOOBAR(2, 16);<br>
+ res = 0b10 ^ 16;<br>
+ res = 0B10 ^ 16;<br>
+ res = 2 ^ 0b100;<br>
+ res = XOR(2, 16);<br>
+ unsigned char two = 2;<br>
+ res = two ^ 16;<br>
+ res = TWO_ULL ^ 16;<br>
+ res = 2 ^ 32; // expected-warning {{result of '2 ^ 32' is 34; did you mean '1LL << 32'?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:15}:"1LL << 32"<br>
+ // expected-note@-2 {{replace expression with '0x2 ^ 32' to silence this warning}}<br>
+ res = 2 ^ 64;<br>
+<br>
+ res = EPSILON;<br>
+ res = 10 ^ 0; // expected-warning {{result of '10 ^ 0' is 10; did you mean '1e0'?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:15}:"1e0"<br>
+ // expected-note@-2 {{replace expression with '0xA ^ 0' to silence this warning}}<br>
+ res = 10 ^ 1; // expected-warning {{result of '10 ^ 1' is 11; did you mean '1e1'?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:15}:"1e1"<br>
+ // expected-note@-2 {{replace expression with '0xA ^ 1' to silence this warning}}<br>
+ res = 10 ^ 2; // expected-warning {{result of '10 ^ 2' is 8; did you mean '1e2'?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:15}:"1e2"<br>
+ // expected-note@-2 {{replace expression with '0xA ^ 2' to silence this warning}}<br>
+ res = 10 ^ 4; // expected-warning {{result of '10 ^ 4' is 14; did you mean '1e4'?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:15}:"1e4"<br>
+ // expected-note@-2 {{replace expression with '0xA ^ 4' to silence this warning}}<br>
+ res = 10 ^ 10; // expected-warning {{result of '10 ^ 10' is 0; did you mean '1e10'?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:16}:"1e10"<br>
+ // expected-note@-2 {{replace expression with '0xA ^ 10' to silence this warning}}<br>
+ res = TEN ^ 10; // expected-warning {{result of 'TEN ^ 10' is 0; did you mean '1e10'?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:17}:"1e10"<br>
+ // expected-note@-2 {{replace expression with '0xA ^ 10' to silence this warning}}<br>
+ res = 10 ^ 100; // expected-warning {{result of '10 ^ 100' is 110; did you mean '1e100'?}}<br>
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:17}:"1e100"<br>
+ // expected-note@-2 {{replace expression with '0xA ^ 100' to silence this warning}}<br>
+ res = 0xA ^ 10;<br>
+ res = 10 xor 10;<br>
+#ifdef __cplusplus<br>
+ res = 10 ^ 5_xor;<br>
+ res = 10_xor ^ 5;<br>
+ res = 10 ^ 5_0b;<br>
+ res = 10_0X ^ 5;<br>
+#endif<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>