[Sema] Implement support for -Wformat-signedness (#74440)

In gcc there exist a modifier option -Wformat-signedness that turns on
additional signedness warnings in the already existing -Wformat warning.

This patch implements that support in clang. This is done by adding a dummy
warning diag::warn_format_conversion_argument_type_mismatch_signedness that
is never emitted and only used as an option to toggle the signedness warning
in -Wformat. This will ensure gcc compatibility.

Author: karka228

clang/docs/ReleaseNotes.rst

@@ -310,6 +310,9 @@ Improvements to Clang's diagnostics
 - Clang now no longer diagnoses type definitions in ``offsetof`` in C23 mode.
   Fixes #GH83658.
 
+- New ``-Wformat-signedness`` diagnostic that warn if the format string requires an
+  unsigned argument and the argument is signed and vice versa.
+
 Improvements to Clang's time-trace
 ----------------------------------
 

clang/include/clang/AST/FormatString.h

@@ -284,6 +284,8 @@ class ArgType {
     /// The conversion specifier and the argument type are disallowed by the C
     /// standard, but are in practice harmless. For instance, "%p" and int*.
     NoMatchPedantic,
+    /// The conversion specifier and the argument type have different sign.
+    NoMatchSignedness,
     /// The conversion specifier and the argument type are compatible, but still
     /// seems likely to be an error. For instance, "%hd" and _Bool.
     NoMatchTypeConfusion,

clang/include/clang/Basic/DiagnosticGroups.td

@@ -985,6 +985,7 @@ def FormatSecurity : DiagGroup<"format-security">;
 def FormatNonStandard : DiagGroup<"format-non-iso">;
 def FormatY2K : DiagGroup<"format-y2k">;
 def FormatPedantic : DiagGroup<"format-pedantic">;
+def FormatSignedness : DiagGroup<"format-signedness">;
 def FormatTypeConfusion : DiagGroup<"format-type-confusion">;
 
 def FormatOverflowNonKprintf: DiagGroup<"format-overflow-non-kprintf">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -9821,6 +9821,9 @@ def warn_format_conversion_argument_type_mismatch : Warning<
 def warn_format_conversion_argument_type_mismatch_pedantic : Extension<
   warn_format_conversion_argument_type_mismatch.Summary>,
   InGroup<FormatPedantic>;
+def warn_format_conversion_argument_type_mismatch_signedness : Warning<
+  warn_format_conversion_argument_type_mismatch.Summary>,
+  InGroup<FormatSignedness>, DefaultIgnore;
 def warn_format_conversion_argument_type_mismatch_confusion : Warning<
   warn_format_conversion_argument_type_mismatch.Summary>,
   InGroup<FormatTypeConfusion>, DefaultIgnore;

clang/lib/AST/FormatString.cpp

@@ -413,7 +413,7 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
         return Match;
       if (const auto *BT = argTy->getAs<BuiltinType>()) {
         // Check if the only difference between them is signed vs unsigned
-        // if true, we consider they are compatible.
+        // if true, return match signedness.
         switch (BT->getKind()) {
           default:
             break;
@@ -423,44 +423,53 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
             [[fallthrough]];
           case BuiltinType::Char_S:
           case BuiltinType::SChar:
+            if (T == C.UnsignedShortTy || T == C.ShortTy)
+              return NoMatchTypeConfusion;
+            if (T == C.UnsignedCharTy)
+              return NoMatchSignedness;
+            if (T == C.SignedCharTy)
+              return Match;
+            break;
           case BuiltinType::Char_U:
           case BuiltinType::UChar:
             if (T == C.UnsignedShortTy || T == C.ShortTy)
               return NoMatchTypeConfusion;
-            if (T == C.UnsignedCharTy || T == C.SignedCharTy)
+            if (T == C.UnsignedCharTy)
               return Match;
+            if (T == C.SignedCharTy)
+              return NoMatchSignedness;
             break;
           case BuiltinType::Short:
             if (T == C.UnsignedShortTy)
-              return Match;
+              return NoMatchSignedness;
             break;
           case BuiltinType::UShort:
             if (T == C.ShortTy)
-              return Match;
+              return NoMatchSignedness;
             break;
           case BuiltinType::Int:
             if (T == C.UnsignedIntTy)
-              return Match;
+              return NoMatchSignedness;
             break;
           case BuiltinType::UInt:
             if (T == C.IntTy)
-              return Match;
+              return NoMatchSignedness;
             break;
           case BuiltinType::Long:
             if (T == C.UnsignedLongTy)
-              return Match;
+              return NoMatchSignedness;
             break;
           case BuiltinType::ULong:
             if (T == C.LongTy)
-              return Match;
+              return NoMatchSignedness;
             break;
           case BuiltinType::LongLong:
             if (T == C.UnsignedLongLongTy)
-              return Match;
+              return NoMatchSignedness;
             break;
           case BuiltinType::ULongLong:
             if (T == C.LongLongTy)
-              return Match;
+              return NoMatchSignedness;
             break;
           }
           // "Partially matched" because of promotions?

clang/lib/Sema/SemaChecking.cpp

@@ -12446,6 +12446,19 @@ isArithmeticArgumentPromotion(Sema &S, const ImplicitCastExpr *ICE) {
          S.Context.getFloatingTypeOrder(From, To) < 0;
 }
 
+static analyze_format_string::ArgType::MatchKind
+handleFormatSignedness(analyze_format_string::ArgType::MatchKind Match,
+                       DiagnosticsEngine &Diags, SourceLocation Loc) {
+  if (Match == analyze_format_string::ArgType::NoMatchSignedness) {
+    Match =
+        Diags.isIgnored(
+            diag::warn_format_conversion_argument_type_mismatch_signedness, Loc)
+            ? analyze_format_string::ArgType::Match
+            : analyze_format_string::ArgType::NoMatch;
+  }
+  return Match;
+}
+
 bool
 CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
                                     const char *StartSpecifier,
@@ -12489,6 +12502,9 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
 
   ArgType::MatchKind ImplicitMatch = ArgType::NoMatch;
   ArgType::MatchKind Match = AT.matchesType(S.Context, ExprTy);
+  ArgType::MatchKind OrigMatch = Match;
+
+  Match = handleFormatSignedness(Match, S.getDiagnostics(), E->getExprLoc());
   if (Match == ArgType::Match)
     return true;
 
@@ -12512,6 +12528,14 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
           ICE->getType() == S.Context.UnsignedIntTy) {
         // All further checking is done on the subexpression
         ImplicitMatch = AT.matchesType(S.Context, ExprTy);
+        if (OrigMatch == ArgType::NoMatchSignedness &&
+            ImplicitMatch != ArgType::NoMatchSignedness)
+          // If the original match was a signedness match this match on the
+          // implicit cast type also need to be signedness match otherwise we
+          // might introduce new unexpected warnings from -Wformat-signedness.
+          return true;
+        ImplicitMatch = handleFormatSignedness(
+            ImplicitMatch, S.getDiagnostics(), E->getExprLoc());
         if (ImplicitMatch == ArgType::Match)
           return true;
       }
@@ -12633,6 +12657,7 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
       case ArgType::Match:
       case ArgType::MatchPromotion:
       case ArgType::NoMatchPromotionTypeConfusion:
+      case ArgType::NoMatchSignedness:
         llvm_unreachable("expected non-matching");
       case ArgType::NoMatchPedantic:
         Diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
@@ -12668,8 +12693,10 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
       CastFix << (S.LangOpts.CPlusPlus ? ">" : ")");
 
       SmallVector<FixItHint,4> Hints;
-      if (AT.matchesType(S.Context, IntendedTy) != ArgType::Match ||
-          ShouldNotPrintDirectly)
+      ArgType::MatchKind IntendedMatch = AT.matchesType(S.Context, IntendedTy);
+      IntendedMatch = handleFormatSignedness(IntendedMatch, S.getDiagnostics(),
+                                             E->getExprLoc());
+      if ((IntendedMatch != ArgType::Match) || ShouldNotPrintDirectly)
         Hints.push_back(FixItHint::CreateReplacement(SpecRange, os.str()));
 
       if (const CStyleCastExpr *CCast = dyn_cast<CStyleCastExpr>(E)) {
@@ -12738,6 +12765,7 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
       case ArgType::Match:
       case ArgType::MatchPromotion:
       case ArgType::NoMatchPromotionTypeConfusion:
+      case ArgType::NoMatchSignedness:
         llvm_unreachable("expected non-matching");
       case ArgType::NoMatchPedantic:
         Diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
@@ -12949,6 +12977,7 @@ bool CheckScanfHandler::HandleScanfSpecifier(
 
   analyze_format_string::ArgType::MatchKind Match =
       AT.matchesType(S.Context, Ex->getType());
+  Match = handleFormatSignedness(Match, S.getDiagnostics(), Ex->getExprLoc());
   bool Pedantic = Match == analyze_format_string::ArgType::NoMatchPedantic;
   if (Match == analyze_format_string::ArgType::Match)
     return true;

clang/test/Sema/format-strings-signedness-fixit.c

@@ -0,0 +1,98 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -Wformat -Wformat-signedness -fixit %t
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wformat -Wformat-signedness -Werror %t
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -E -o - %t | FileCheck %s
+
+#include <limits.h>
+
+int printf(const char *restrict format, ...);
+
+void test_printf_int(int x)
+{
+    printf("%u", x);
+}
+
+void test_printf_unsigned(unsigned x)
+{
+    printf("%d", x);
+}
+
+void test_printf_long(long x)
+{
+    printf("%lu", x);
+}
+
+void test_printf_unsigned_long(unsigned long x)
+{
+    printf("%ld", x);
+}
+
+void test_printf_long_long(long long x)
+{
+    printf("%llu", x);
+}
+
+void test_printf_unsigned_long_long(unsigned long long x)
+{
+    printf("%lld", x);
+}
+
+enum enum_int {
+    minus_1 = -1
+};
+
+void test_printf_enum_int(enum enum_int x)
+{
+    printf("%u", x);
+}
+
+enum enum_unsigned {
+    zero = 0
+};
+
+void test_printf_enum_unsigned(enum enum_unsigned x)
+{
+    printf("%d", x);
+}
+
+enum enum_long {
+    minus_one = -1,
+    int_val = INT_MAX,
+    unsigned_val = (unsigned)INT_MIN
+};
+
+void test_printf_enum_long(enum enum_long x)
+{
+    printf("%lu", x);
+}
+
+enum enum_unsigned_long {
+    uint_max_plus = (unsigned long)UINT_MAX+1,
+};
+
+void test_printf_enum_unsigned_long(enum enum_unsigned_long x)
+{
+    printf("%ld", x);
+}
+
+// Validate the fixes.
+// CHECK: void test_printf_int(int x)
+// CHECK: printf("%d", x);
+// CHECK: void test_printf_unsigned(unsigned x)
+// CHECK: printf("%u", x);
+// CHECK: void test_printf_long(long x)
+// CHECK: printf("%ld", x);
+// CHECK: void test_printf_unsigned_long(unsigned long x)
+// CHECK: printf("%lu", x);
+// CHECK: void test_printf_long_long(long long x)
+// CHECK: printf("%lld", x);
+// CHECK: void test_printf_unsigned_long_long(unsigned long long x)
+// CHECK: printf("%llu", x);
+// CHECK: void test_printf_enum_int(enum enum_int x)
+// CHECK: printf("%d", x);
+// CHECK: void test_printf_enum_unsigned(enum enum_unsigned x)
+// CHECK: printf("%u", x);
+// CHECK: void test_printf_enum_long(enum enum_long x)
+// CHECK: printf("%ld", x);
+// CHECK: void test_printf_enum_unsigned_long(enum enum_unsigned_long x)
+// CHECK: printf("%lu", x);