Author: Tamás Zolnai Date: 2020-05-02T14:05:05+02:00 New Revision: 030ff901f43258d18b68a77b0085d0fae2a0fbc6
URL: https://github.com/llvm/llvm-project/commit/030ff901f43258d18b68a77b0085d0fae2a0fbc6 DIFF: https://github.com/llvm/llvm-project/commit/030ff901f43258d18b68a77b0085d0fae2a0fbc6.diff LOG: [clang-tidy] extend bugprone-signed-char-misuse check with array subscript case. Summary: To cover STR34-C rule's second use case, where ``signed char`` is used for array subscript after an integer conversion. In the case of non-ASCII character this conversion will result in a value in excess of UCHAR_MAX. There is another clang-tidy check which catches these cases. cppcoreguidelines-pro-bounds-constant-array-index catches any indexing which is not integer constant. I think this check is very strict about the index (e.g. constant), so it's still useful to cover the ``signed char`` use case in this check, so we can provide a way to catch the SEI cert rule's use cases on a codebase, where this CPP guideline is not used. Reviewers: aaron.ballman, njames93 Reviewed By: aaron.ballman Subscribers: xazax.hun, cfe-commits Tags: #clang, #clang-tools-extra Differential Revision: https://reviews.llvm.org/D78904 Added: Modified: clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp index 732ccbc9dd2a..66f00e35c7e7 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp @@ -102,11 +102,31 @@ void SignedCharMisuseCheck::registerMatchers(MatchFinder *Finder) { .bind("comparison"); Finder->addMatcher(CompareOperator, this); + + // Catch array subscripts with signed char -> integer conversion. + // Matcher for C arrays. + const auto CArraySubscript = + arraySubscriptExpr(hasIndex(SignedCharCastExpr)).bind("arraySubscript"); + + Finder->addMatcher(CArraySubscript, this); + + // Matcher for std arrays. + const auto STDArraySubscript = + cxxOperatorCallExpr( + hasOverloadedOperatorName("[]"), + hasArgument(0, hasType(cxxRecordDecl(hasName("::std::array")))), + hasArgument(1, SignedCharCastExpr)) + .bind("arraySubscript"); + + Finder->addMatcher(STDArraySubscript, this); } void SignedCharMisuseCheck::check(const MatchFinder::MatchResult &Result) { const auto *SignedCastExpression = Result.Nodes.getNodeAs<ImplicitCastExpr>("signedCastExpression"); + const auto *IntegerType = Result.Nodes.getNodeAs<QualType>("integerType"); + assert(SignedCastExpression); + assert(IntegerType); // Ignore the match if we know that the signed char's value is not negative. // The potential misinterpretation happens for negative values only. @@ -135,14 +155,17 @@ void SignedCharMisuseCheck::check(const MatchFinder::MatchResult &Result) { diag(Comparison->getBeginLoc(), "comparison between 'signed char' and 'unsigned char'"); - } else if (const auto *IntegerType = - Result.Nodes.getNodeAs<QualType>("integerType")) { + } else if (Result.Nodes.getNodeAs<Expr>("arraySubscript")) { + diag(SignedCastExpression->getBeginLoc(), + "'signed char' to %0 conversion in array subscript; " + "consider casting to 'unsigned char' first.") + << *IntegerType; + } else { diag(SignedCastExpression->getBeginLoc(), "'signed char' to %0 conversion; " "consider casting to 'unsigned char' first.") << *IntegerType; - } else - llvm_unreachable("Unexpected match"); + } } } // namespace bugprone diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst index e3ecf75a3a52..c2bd2df062b1 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst @@ -31,11 +31,10 @@ It depends on the actual platform whether plain ``char`` is handled as ``signed by default and so it is caught by this check or not. To change the default behavior you can use ``-funsigned-char`` and ``-fsigned-char`` compilation options. -Currently, this check is limited to assignments and variable declarations, -where a ``signed char`` is assigned to an integer variable and to -equality/inequality comparisons between ``signed char`` and ``unsigned char``. -There are other use cases where the unexpected value ranges might lead to -similar bogus behavior. +Currently, this check warns in the following cases: +- ``signed char`` is assigned to an integer variable +- ``signed char`` and ``unsigned char`` are compared with equality/inequality operator +- ``signed char`` is converted to an integer in the array subscript See also: `STR34-C. Cast characters to unsigned char before converting to larger integer sizes diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp index ef42b0c85ae6..a1cc2ae48991 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp @@ -3,6 +3,16 @@ /////////////////////////////////////////////////////////////////// /// Test cases correctly caught by the check. +typedef __SIZE_TYPE__ size_t; + +namespace std { +template <typename T, size_t N> +struct array { + T &operator[](size_t n); + T &at(size_t n); +}; +} // namespace std + int SimpleVarDeclaration() { signed char CCharacter = -5; int NCharacter = CCharacter; @@ -90,6 +100,16 @@ int CompareWithUnsignedNonAsciiConstant(signed char SCharacter) { return 0; } +int SignedCharCArraySubscript(signed char SCharacter) { + int Array[3] = {1, 2, 3}; + + return Array[static_cast<unsigned int>(SCharacter)]; // CHECK-MESSAGES: [[@LINE]]:42: warning: 'signed char' to 'unsigned int' conversion in array subscript; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse] +} + +int SignedCharSTDArraySubscript(std::array<int, 3> Array, signed char SCharacter) { + return Array[static_cast<unsigned int>(SCharacter)]; // CHECK-MESSAGES: [[@LINE]]:42: warning: 'signed char' to 'unsigned int' conversion in array subscript; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse] +} + /////////////////////////////////////////////////////////////////// /// Test cases correctly ignored by the check. @@ -207,3 +227,23 @@ int CompareWithUnsignedAsciiConstant(signed char SCharacter) { return 1; return 0; } + +int UnsignedCharCArraySubscript(unsigned char USCharacter) { + int Array[3] = {1, 2, 3}; + + return Array[static_cast<unsigned int>(USCharacter)]; +} + +int CastedCArraySubscript(signed char SCharacter) { + int Array[3] = {1, 2, 3}; + + return Array[static_cast<unsigned char>(SCharacter)]; +} + +int UnsignedCharSTDArraySubscript(std::array<int, 3> Array, unsigned char USCharacter) { + return Array[static_cast<unsigned int>(USCharacter)]; +} + +int CastedSTDArraySubscript(std::array<int, 3> Array, signed char SCharacter) { + return Array[static_cast<unsigned char>(SCharacter)]; +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
