r369217 - [Diagnostics] Diagnose misused xor as pow

Nico Weber via cfe-commits cfe-commits at lists.llvm.org
Mon Aug 19 10:53:14 PDT 2019


Hi,

this results in a false positive on webrtc, on this code:

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

The code does this:

#ifdef WORDS_BIGENDIAN
#define ALPHA_OFFSET 0   // uint32_t 0xff000000 is 0xff,00,00,00 in memory
#else
#define ALPHA_OFFSET 3   // uint32_t 0xff000000 is 0x00,00,00,ff in memory
#endif

// ...

    const uint8_t* const argb = (const uint8_t*)picture->argb;
    const uint8_t* const a = argb + (0 ^ ALPHA_OFFSET);
    const uint8_t* const r = argb + (1 ^ ALPHA_OFFSET);
    const uint8_t* const g = argb + (2 ^ ALPHA_OFFSET);
    const uint8_t* const b = argb + (3 ^ ALPHA_OFFSET);

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.

Maybe this shouldn't fire if the rhs is a macro?

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

On Sun, Aug 18, 2019 at 3:13 PM David Bolvansky via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

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


More information about the cfe-commits mailing list