[clang-tidy] extend bugprone-signed-char-misuse check.

[ztamas]

Summary

Cover a new use case when using a 'signed char' as an integer
might lead to issue with non-ASCII characters. Comparing
a 'signed char' with an 'unsigned char' using equality / unequality
operator produces an unexpected result for non-ASCII characters.

[ztamas]

I run the check on LLVM code and found only one catch of suspicious comparison:

clang-tidy -p=/home/zolnai/clang/build /home/zolnai/clang/llvm-project/clang/lib/Lex/Lexer.cpp
/home/zolnai/clang/llvm-project/clang/lib/Lex/Lexer.cpp:1293:39: warning: comparison between signed and unsigned char [bugprone-signed-char-misuse]
      if ((C == '\n' || C == '\r') && C != PrevC)

[ztamas]

In the LibreOffice codebase, I found 8 catches:
https://gist.github.com/tzolnai/2b22492c0cf79f5dba577f6a878657e3

All catches seem valid to me.

[njames93]

clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp

74

All node matchers are implicitly allOf matchers so you can safely drop the allOf matcher.

97

binaryOperator(hasAnyOperatorName("==", "!=") ...
Also whats the reason for not checking <, >, <=, >= or <=>?

103

Any reason for this trailing ;?

125

This should probably be a condition variable and bring the init from the start of check inside.

141–143

} else if (const auto *IntegerType = Result.Nodes.getNodeAs<QualType>("integerType")) {
  ...
} else 
  llvm_unreachable("Unexpected match");

clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp

210

New line

[ztamas]

Fix pre-merge check error

Plus remove unneeded "no warning" comments
and add a missing quote mark in docs.

[ztamas]

Changed code based on reviewer comments.

• Removed unneeded allOf()
• Remove unneeded ';'
• Added new line at the end of the file
• Moved variable declarations inside if()

[ztamas]

One more unneeded allOf and clang-format

[ztamas]

clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp

97

I think these are different cases when the programmer checks the equality of two characters or checking some kind of order of characters. In case of equality, there seems to be a clear assumption, that if the two character variables represent the same character (semantically the same), then the equality should return true. This assumption fails with mixed signed and unsigned char variables.
However, when the programmer uses a less or a greater operator it's a bit different. I guess a programmer in the case starts to wonder what is the actual order of the characters, checks the ASCII table if it's not obvious for a set of characters. Also, it's not clear what might be the false assumption here. Is an ASCII character smaller or greater than a non-ASCII character? So I don't see a probable false assumption here what we can point out. At least, in general.
Furthermore, I optimized the check for equality/inequality, with ignoring the ASCII characters for both the signed and the unsigned operand. This makes sense for equality/inequality but does not make sense for the other comparison operators.
All-in-all I'm now focusing on the equality / unequality operators. Usage of these operands clearly something we can catch. Checking other comparison operators is something that needs consideration, so I don't bother with that in this patch.

[ztamas]

Use quotes in warning message.

[njames93]

clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp

97

Fair enough, in that case can you specify in the docs that you only check for == and != comparison. Currently they only say it looks for comparison rather than maybe equality comparison.
Also point about hasAnyOperatorName still stands.

[ztamas]

Update code based on reviewer comments.

• Update docs: comparison -> equality/inequality comparison.
• Use hasAnyOperatorName
• Plus fix a typo

[njames93]

LGTM

However, how does this handle cases when the type is written as char. They can be signed/unsigned based on what platform is being targeted. But on a platform where char is signed, comparison to an unsigned char is dangerous and the complimented case is the same. Surely any char comparison to signed char or unsigned char should be warned. But I'm not sure if that's in the scope of this check.

[ztamas]

In D75749#1912338, @njames93 wrote:

LGTM

However, how does this handle cases when the type is written as char. They can be signed/unsigned based on what platform is being targeted. But on a platform where char is signed, comparison to an unsigned char is dangerous and the complimented case is the same. Surely any char comparison to signed char or unsigned char should be warned. But I'm not sure if that's in the scope of this check.

It uses the default signedness what clang specifies for char (which depends on the platform). For example, on my Linux x64 system, char is equivalent to signed char and so this clang-tidy check catches also the char <-> unsigned char comparisons. This default behavior can be overridden with -funsigned-char and -fsigned-char compilation options. So for a cross-platform C++ code, it's an option to run the clang-tidy twice, using these two options and so both char <-> unsigned char and char <-> signed char comparisons can be caught.

clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNEDCHARMISUSECHECK_H
 
 #include "../ClangTidyCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
-/// Finds ``signed char`` -> integer conversions which might indicate a programming
-/// error. The basic problem with the ``signed char``, that it might store the
-/// non-ASCII characters as negative values. The human programmer probably
-/// expects that after an integer conversion the converted value matches with the
-/// character code (a value from [0..255]), however, the actual value is in
-/// [-128..127] interval. This also applies to the plain ``char`` type on
-/// those implementations which represent ``char`` similar to ``signed char``.
+/// Finds those ``signed char`` -> integer conversions which might indicate a
+/// programming error. The basic problem with the ``signed char``, that it might
+/// store the non-ASCII characters as negative values. This behavior can cause a
+/// misunderstanding of the written code both when an explicit and when an
+/// implicit conversion happens.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-signed-char-misuse.html
 class SignedCharMisuseCheck : public ClangTidyCheck {
 public:
   SignedCharMisuseCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
+  ast_matchers::internal::BindableMatcher<clang::Stmt> charCastExpression(
+      bool isSigned,
+      const ast_matchers::internal::Matcher<clang::QualType> &IntegerType,
+      const std::string &CastBindName) const;
+
   const std::string CharTypdefsToIgnoreList;
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNEDCHARMISUSECHECK_H

clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp

 
 using namespace clang::ast_matchers;
 using namespace clang::ast_matchers::internal;
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
+static constexpr int UnsignedASCIIUpperBound = 127;
+
 static Matcher<TypedefDecl> hasAnyListedName(const std::string &Names) {
   const std::vector<std::string> NameList =
       utils::options::parseStringList(Names);
   return hasAnyName(std::vector<StringRef>(NameList.begin(), NameList.end()));
 }
 
 SignedCharMisuseCheck::SignedCharMisuseCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       CharTypdefsToIgnoreList(Options.get("CharTypdefsToIgnore", "")) {}
 
 void SignedCharMisuseCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "CharTypdefsToIgnore", CharTypdefsToIgnoreList);
 }
 
-void SignedCharMisuseCheck::registerMatchers(MatchFinder *Finder) {
+// Create a matcher for char -> integer cast.
+BindableMatcher<clang::Stmt> SignedCharMisuseCheck::charCastExpression(
+    bool IsSigned, const Matcher<clang::QualType> &IntegerType,
+    const std::string &CastBindName) const {
   // We can ignore typedefs which are some kind of integer types
   // (e.g. typedef char sal_Int8). In this case, we don't need to
   // worry about the misinterpretation of char values.
   const auto IntTypedef = qualType(
       hasDeclaration(typedefDecl(hasAnyListedName(CharTypdefsToIgnoreList))));
 
-  const auto SignedCharType = expr(hasType(qualType(
+  auto CharTypeExpr = expr();
+  if (IsSigned) {
+    CharTypeExpr = expr(hasType(qualType(
         allOf(isAnyCharacter(), isSignedInteger(), unless(IntTypedef)))));
+  } else {
+    CharTypeExpr = expr(hasType(qualType(allOf(
+        isAnyCharacter(), unless(isSignedInteger()), unless(IntTypedef)))));
+  }
 
-  const auto IntegerType = qualType(allOf(isInteger(), unless(isAnyCharacter()),
-                                          unless(booleanType())))
-                               .bind("integerType");
-
-  // We are interested in signed char -> integer conversion.
   const auto ImplicitCastExpr =
-      implicitCastExpr(hasSourceExpression(SignedCharType),
+      implicitCastExpr(hasSourceExpression(CharTypeExpr),
                        hasImplicitDestinationType(IntegerType))
-          .bind("castExpression");
+          .bind(CastBindName);
 
   const auto CStyleCastExpr = cStyleCastExpr(has(ImplicitCastExpr));
   const auto StaticCastExpr = cxxStaticCastExpr(has(ImplicitCastExpr));
   const auto FunctionalCastExpr = cxxFunctionalCastExpr(has(ImplicitCastExpr));
 
   // We catch any type of casts to an integer. We need to have these cast
   // expressions explicitly to catch only those casts which are direct children
-  // of an assignment/declaration.
-  const auto CastExpr = expr(anyOf(ImplicitCastExpr, CStyleCastExpr,
-                                   StaticCastExpr, FunctionalCastExpr));
-
-  // Catch assignments with the suspicious type conversion.
-  const auto AssignmentOperatorExpr = expr(binaryOperator(
-      hasOperatorName("="), hasLHS(hasType(IntegerType)), hasRHS(CastExpr)));
+  // of the checked expressions. (e.g. assignment, declaration).
+  return expr(anyOf(ImplicitCastExpr, CStyleCastExpr, StaticCastExpr,
+                    FunctionalCastExpr));
+}
+
+void SignedCharMisuseCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IntegerType = qualType(allOf(isInteger(), unless(isAnyCharacter()),
+                                          unless(booleanType())))
+                               .bind("integerType");
+  const auto SignedCharCastExpr =
+      charCastExpression(true, IntegerType, "signedCastExpression");
+  const auto UnSignedCharCastExpr =
+      charCastExpression(false, IntegerType, "unsignedCastExpression");
+
+  // Catch assignments with singed char -> integer conversion.
+  const auto AssignmentOperatorExpr =
+      expr(binaryOperator(hasOperatorName("="), hasLHS(hasType(IntegerType)),
+                          hasRHS(SignedCharCastExpr)));
 
   Finder->addMatcher(AssignmentOperatorExpr, this);
 
-  // Catch declarations with the suspicious type conversion.
-  const auto Declaration =
-      varDecl(isDefinition(), hasType(IntegerType), hasInitializer(CastExpr));
+  // Catch declarations with singed char -> integer conversion.
+  const auto Declaration = varDecl(isDefinition(), hasType(IntegerType),
+                                   hasInitializer(SignedCharCastExpr));
 
   Finder->addMatcher(Declaration, this);
+
+  // Catch signed char/unsigned char comparison.
+  const auto CompareOperator =
+      expr(binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+                          anyOf(allOf(hasLHS(SignedCharCastExpr),
+                                      hasRHS(UnSignedCharCastExpr)),
+                                allOf(hasLHS(UnSignedCharCastExpr),
+                                      hasRHS(SignedCharCastExpr)))))
+          .bind("comparison");
+  ;
+
+  Finder->addMatcher(CompareOperator, this);
 }
 
 void SignedCharMisuseCheck::check(const MatchFinder::MatchResult &Result) {
-  const auto *CastExpression =
-      Result.Nodes.getNodeAs<ImplicitCastExpr>("castExpression");
-  const auto *IntegerType = Result.Nodes.getNodeAs<QualType>("integerType");
-  assert(CastExpression);
-  assert(IntegerType);
+  const auto *Comparison = Result.Nodes.getNodeAs<Expr>("comparison");
+
+  const auto *SignedCastExpression =
+      Result.Nodes.getNodeAs<ImplicitCastExpr>("signedCastExpression");
 
-  // Ignore the match if we know that the value is not negative.
+  // Ignore the match if we know that the signed char's value is not negative.
   // The potential misinterpretation happens for negative values only.
   Expr::EvalResult EVResult;
-  if (!CastExpression->isValueDependent() &&
-      CastExpression->getSubExpr()->EvaluateAsInt(EVResult, *Result.Context)) {
-    llvm::APSInt Value1 = EVResult.Val.getInt();
-    if (Value1.isNonNegative())
+  if (!SignedCastExpression->isValueDependent() &&
+      SignedCastExpression->getSubExpr()->EvaluateAsInt(EVResult,
+                                                        *Result.Context)) {
+    llvm::APSInt Value = EVResult.Val.getInt();
+    if (Value.isNonNegative())
       return;
   }
 
-  diag(CastExpression->getBeginLoc(),
+  if (Comparison) {
+    const auto *UnSignedCastExpression =
+        Result.Nodes.getNodeAs<ImplicitCastExpr>("unsignedCastExpression");
+
+    // We can ignore the ASCII value range also for unsigned char.
+    Expr::EvalResult EVResult;
+    if (!UnSignedCastExpression->isValueDependent() &&
+        UnSignedCastExpression->getSubExpr()->EvaluateAsInt(EVResult,
+                                                            *Result.Context)) {
+      llvm::APSInt Value = EVResult.Val.getInt();
+      if (Value <= UnsignedASCIIUpperBound)
+        return;
+    }
+
+    diag(Comparison->getBeginLoc(),
+         "comparison between signed and unsigned char");
+  } else {
+    const auto *IntegerType = Result.Nodes.getNodeAs<QualType>("integerType");
+    assert(IntegerType);
+
+    diag(SignedCastExpression->getBeginLoc(),
          "'signed char' to %0 conversion; "
          "consider casting to 'unsigned char' first.")
         << *IntegerType;
   }
+}
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst

 .. title:: clang-tidy - bugprone-signed-char-misuse
 
 bugprone-signed-char-misuse
 ===========================
 
-Finds ``signed char`` -> integer conversions which might indicate a programming
-error. The basic problem with the ``signed char``, that it might store the
-non-ASCII characters as negative values. The human programmer probably
-expects that after an integer conversion the converted value matches with the
-character code (a value from [0..255]), however, the actual value is in
-[-128..127] interval. This also applies to the plain ``char`` type on
-those implementations which represent ``char`` similar to ``signed char``.
+Finds those ``signed char`` -> integer conversions which might indicate a
+programming error. The basic problem with the ``signed char``, that it might
+store the non-ASCII characters as negative values. This behavior can cause a
+misunderstanding of the written code both when an explicit and when an
+implicit conversion happens.
 
-To avoid this kind of misinterpretation, the desired way of converting from a
-``signed char`` to an integer value is converting to ``unsigned char`` first,
-which stores all the characters in the positive [0..255] interval which matches
-with the known character codes.
+When the code contains an explicit ``signed char`` -> integer conversion, the
+human programmer probably expects that the converted value matches with the
+character code (a value from [0..255]), however, the actual value is in
+[-128..127] interval. To avoid this kind of misinterpretation, the desired way
+of converting from a ``signed char`` to an integer value is converting to
+``unsigned char`` first, which stores all the characters in the positive [0..255]
+interval which matches the known character codes.
+
+In case of implicit conversion, the programmer might not actually be aware
+that a conversion happened and char value is used as an integer. There are
+some use cases when this unawareness might lead to a functionally imperfect code.
+For example, checking the equality of a ``signed char`` and an ``unsigned char``
+variable is something we should avoid in C++ code. During this comparison,
+the two variables are converted to integers which have different value ranges.
+For ``signed char``, the non-ASCII characters are stored as a value in [-128..-1]
+interval, while the same characters are stored in the [128..255] interval for
+an `unsigned char``.
 
-It depends on the actual platform whether ``char`` is handled as ``signed char``
+It depends on the actual platform whether plain ``char`` is handled as ``signed char``
 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. There are other
-use cases where the same misinterpretation might lead to similar bogus
-behavior.
+where a ``signed char`` is assigned to an integer variable and to ``signed char``/
+``unsigned char`` comparison. There are other use cases where the unexpected
+value ranges might lead to similar bogus behavior.
 
 See also:
 `STR34-C. Cast characters to unsigned char before converting to larger integer sizes
 <https://wiki.sei.cmu.edu/confluence/display/c/STR34-C.+Cast+characters+to+unsigned+char+before+converting+to+larger+integer+sizes>`_
 
 A good example from the CERT description when a ``char`` variable is used to
 read from a file that might contain non-ASCII characters. The problem comes
 up when the code uses the ``-1`` integer value as EOF, while the 255 character
int read(void) {
     int IChar = EOF;
 
     if (readChar(CChar)) {
       IChar = static_cast<unsigned char>(CChar);
     }
     return IChar;
   }
 
+Another use case is the comparison of two ``char`` variables with different
+signedness. Inside the non-ASCII value range checking the equality of a
+``signed char`` and an ``unsigned char`` will always return ``false``.
+
+.. code-block:: c++
+
+  bool compare(signed char SChar, unsigned char USChar) {
+    if (SChar == USChar)
+      return true;
+    return false;
+  }
+
+The easiest way to fix this kind of comparison is casting one of the arguments,
+so both arguments of the comparison have the same type.
+
+.. code-block:: c++
+
+  bool compare(signed char SChar, unsigned char USChar) {
+    if (static_cast<unsigned char>(SChar) == USChar)
+      return true;
+    return false;
+  }
+
 .. option:: CharTypdefsToIgnore
 
   A semicolon-separated list of typedef names. In this list, we can list
   typedefs for ``char`` or ``signed char``, which will be ignored by the
   check. This is useful when a typedef introduces an integer alias like
   ``sal_Int8`` or ``int8_t``. In this case, human misinterpretation is not
   an issue.

clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp

 
 int CharPointer(signed char *CCharacter) {
   int NCharacter = *CCharacter;
   // CHECK-MESSAGES: [[@LINE-1]]:20: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
 
   return NCharacter;
 }
 
+int SignedUnsignedCharEquality(signed char SCharacter) {
+  unsigned char USCharacter = 'a';
+  if (SCharacter == USCharacter) // CHECK-MESSAGES: [[@LINE]]:7: warning: comparison between signed and unsigned char [bugprone-signed-char-misuse]
+    return 1;
+  return 0;
+}
+
+int SignedUnsignedCharUneqiality(signed char SCharacter) {
+  unsigned char USCharacter = 'a';
+  if (SCharacter != USCharacter) // CHECK-MESSAGES: [[@LINE]]:7: warning: comparison between signed and unsigned char [bugprone-signed-char-misuse]
+    return 1;
+  return 0;
+}
+
+int CompareWithNonAsciiConstant(unsigned char USCharacter) {
+  const signed char SCharacter = -5;
+  if (USCharacter == SCharacter) // CHECK-MESSAGES: [[@LINE]]:7: warning: comparison between signed and unsigned char [bugprone-signed-char-misuse]
+    return 1;
+  return 0;
+}
+
+int CompareWithUnsignedNonAsciiConstant(signed char SCharacter) {
+  const unsigned char USCharacter = 128;
+  if (USCharacter == SCharacter) // CHECK-MESSAGES: [[@LINE]]:7: warning: comparison between signed and unsigned char [bugprone-signed-char-misuse]
+    return 1;
+  return 0;
+}
+
 ///////////////////////////////////////////////////////////////////
 /// Test cases correctly ignored by the check.
 
 int UnsignedCharCast() {
   unsigned char CCharacter = 'a';
   int NCharacter = CCharacter;
 
   return NCharacter;

 // char is an integer type in clang; make sure to ignore it.
 unsigned char CharToCharCast() {
   signed char SCCharacter = 'a';
   unsigned char USCharacter;
   USCharacter = SCCharacter;
 
   return USCharacter;
 }
+
+int FixComparisonWithSignedCharCast(signed char SCharacter) {
+  unsigned char USCharacter = 'a';
+  if (SCharacter == static_cast<signed char>(USCharacter)) // no warning
+    return 1;
+  return 0;
+}
+
+int FixComparisonWithUnSignedCharCast(signed char SCharacter) {
+  unsigned char USCharacter = 'a';
+  if (static_cast<unsigned char>(SCharacter) == USCharacter) // no warning
+    return 1;
+  return 0;
+}
+
+// Make sure we don't catch other type of char comparison.
+int SameCharTypeComparison(signed char SCharacter) {
+  signed char SCharacter2 = 'a';
+  if (SCharacter == SCharacter2) // no warning
+    return 1;
+  return 0;
+}
+
+// Make sure we don't catch other type of char comparison.
+int SameCharTypeComparison2(unsigned char USCharacter) {
+  unsigned char USCharacter2 = 'a';
+  if (USCharacter == USCharacter2) // no warning
+    return 1;
+  return 0;
+}
+
+// Make sure we don't catch integer - char comparison.
+int CharIntComparison(signed char SCharacter) {
+  int ICharacter = 10;
+  if (SCharacter == ICharacter) // no warning
+    return 1;
+  return 0;
+}
+
+int CompareWithAsciiLiteral(unsigned char USCharacter) {
+  if (USCharacter == 'x') // no warning
+    return 1;
+  return 0;
+}
+
+int CompareWithAsciiConstant(unsigned char USCharacter) {
+  const signed char SCharacter = 'a';
+  if (USCharacter == SCharacter) // no warning
+    return 1;
+  return 0;
+}
+
+int CompareWithUnsignedAsciiConstant(signed char SCharacter) {
+  const unsigned char USCharacter = 'a';
+  if (USCharacter == SCharacter) // no warning
+    return 1;
+  return 0;
+}
 No newline at end of file