llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Karl-Johan Karlsson (karka228) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/74440.diff 7 Files Affected: - (modified) clang/include/clang/AST/FormatString.h (+2) - (modified) clang/include/clang/Basic/DiagnosticOptions.def (+1) - (modified) clang/include/clang/Driver/Options.td (+3) - (modified) clang/lib/AST/FormatString.cpp (+19-10) - (modified) clang/lib/Driver/ToolChains/Clang.cpp (+2) - (modified) clang/lib/Sema/SemaChecking.cpp (+24-2) - (added) format-strings-signedness.c (+107) ``````````diff diff --git a/clang/include/clang/AST/FormatString.h b/clang/include/clang/AST/FormatString.h index 5c4ad9baaef60..c267a32be4d6f 100644 --- a/clang/include/clang/AST/FormatString.h +++ b/clang/include/clang/AST/FormatString.h @@ -264,6 +264,8 @@ class ArgType { /// The conversion specifier and the argument type are compatible. For /// instance, "%d" and int. Match = 1, + /// The conversion specifier and the argument type have different sign + MatchSignedness, /// The conversion specifier and the argument type are compatible because of /// default argument promotions. For instance, "%hhd" and int. MatchPromotion, diff --git a/clang/include/clang/Basic/DiagnosticOptions.def b/clang/include/clang/Basic/DiagnosticOptions.def index 6d0c1b14acc12..a9562e7d8dcb0 100644 --- a/clang/include/clang/Basic/DiagnosticOptions.def +++ b/clang/include/clang/Basic/DiagnosticOptions.def @@ -44,6 +44,7 @@ DIAGOPT(Name, Bits, Default) #endif SEMANTIC_DIAGOPT(IgnoreWarnings, 1, 0) /// -w +DIAGOPT(FormatSignedness, 1, 0) /// -Wformat-signedness DIAGOPT(NoRewriteMacros, 1, 0) /// -Wno-rewrite-macros DIAGOPT(Pedantic, 1, 0) /// -pedantic DIAGOPT(PedanticErrors, 1, 0) /// -pedantic-errors diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td index 1d04e4f6e7e6d..04fdf9a7eb92d 100644 --- a/clang/include/clang/Driver/Options.td +++ b/clang/include/clang/Driver/Options.td @@ -918,6 +918,9 @@ def Wdeprecated : Flag<["-"], "Wdeprecated">, Group<W_Group>, HelpText<"Enable warnings for deprecated constructs and define __DEPRECATED">; def Wno_deprecated : Flag<["-"], "Wno-deprecated">, Group<W_Group>, Visibility<[ClangOption, CC1Option]>; +def Wformat_signedness : Flag<["-"], "Wformat-signedness">, + Flags<[HelpHidden]>, Visibility<[ClangOption, CC1Option]>, + MarshallingInfoFlag<DiagnosticOpts<"FormatSignedness">>; def Wl_COMMA : CommaJoined<["-"], "Wl,">, Visibility<[ClangOption, FlangOption]>, Flags<[LinkerInput, RenderAsInput]>, HelpText<"Pass the comma separated arguments in <arg> to the linker">, diff --git a/clang/lib/AST/FormatString.cpp b/clang/lib/AST/FormatString.cpp index e0c9e18cfe3a2..670cde017d3ac 100644 --- a/clang/lib/AST/FormatString.cpp +++ b/clang/lib/AST/FormatString.cpp @@ -409,7 +409,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; @@ -419,44 +419,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 MatchSignedness; + 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 MatchSignedness; break; case BuiltinType::Short: if (T == C.UnsignedShortTy) - return Match; + return MatchSignedness; break; case BuiltinType::UShort: if (T == C.ShortTy) - return Match; + return MatchSignedness; break; case BuiltinType::Int: if (T == C.UnsignedIntTy) - return Match; + return MatchSignedness; break; case BuiltinType::UInt: if (T == C.IntTy) - return Match; + return MatchSignedness; break; case BuiltinType::Long: if (T == C.UnsignedLongTy) - return Match; + return MatchSignedness; break; case BuiltinType::ULong: if (T == C.LongTy) - return Match; + return MatchSignedness; break; case BuiltinType::LongLong: if (T == C.UnsignedLongLongTy) - return Match; + return MatchSignedness; break; case BuiltinType::ULongLong: if (T == C.LongLongTy) - return Match; + return MatchSignedness; break; } // "Partially matched" because of promotions? diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp index f02f7c841b91f..6a6a40293d7d3 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -6054,6 +6054,8 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA, CmdArgs.push_back("-fdeprecated-macro"); } + Args.AddLastArg(CmdArgs, options::OPT_Wformat_signedness); + // Translate GCC's misnamer '-fasm' arguments to '-fgnu-keywords'. if (Arg *Asm = Args.getLastArg(options::OPT_fasm, options::OPT_fno_asm)) { if (Asm->getOption().matches(options::OPT_fasm)) diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 07ced5ffc3407..ea8d5a5d6b913 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -11815,6 +11815,18 @@ isArithmeticArgumentPromotion(Sema &S, const ImplicitCastExpr *ICE) { S.Context.getFloatingTypeOrder(From, To) < 0; } +static clang::analyze_format_string::ArgType::MatchKind +handleFormatSignedness(clang::analyze_format_string::ArgType::MatchKind Match, + DiagnosticOptions &DiagOpts) { + if (Match == clang::analyze_format_string::ArgType::MatchSignedness) { + if (DiagOpts.FormatSignedness) + Match = clang::analyze_format_string::ArgType::NoMatch; + else + Match = clang::analyze_format_string::ArgType::Match; + } + return Match; +} + bool CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS, const char *StartSpecifier, @@ -11858,6 +11870,8 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS, ArgType::MatchKind ImplicitMatch = ArgType::NoMatch; ArgType::MatchKind Match = AT.matchesType(S.Context, ExprTy); + Match = + handleFormatSignedness(Match, S.getDiagnostics().getDiagnosticOptions()); if (Match == ArgType::Match) return true; @@ -11881,6 +11895,8 @@ 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); + ImplicitMatch = handleFormatSignedness( + ImplicitMatch, S.getDiagnostics().getDiagnosticOptions()); if (ImplicitMatch == ArgType::Match) return true; } @@ -12001,6 +12017,7 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS, switch (Match) { case ArgType::Match: case ArgType::MatchPromotion: + case ArgType::MatchSignedness: case ArgType::NoMatchPromotionTypeConfusion: llvm_unreachable("expected non-matching"); case ArgType::NoMatchPedantic: @@ -12037,8 +12054,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().getDiagnosticOptions()); + if ((IntendedMatch != ArgType::Match) || ShouldNotPrintDirectly) Hints.push_back(FixItHint::CreateReplacement(SpecRange, os.str())); if (const CStyleCastExpr *CCast = dyn_cast<CStyleCastExpr>(E)) { @@ -12106,6 +12125,7 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS, switch (Match) { case ArgType::Match: case ArgType::MatchPromotion: + case ArgType::MatchSignedness: case ArgType::NoMatchPromotionTypeConfusion: llvm_unreachable("expected non-matching"); case ArgType::NoMatchPedantic: @@ -12318,6 +12338,8 @@ bool CheckScanfHandler::HandleScanfSpecifier( analyze_format_string::ArgType::MatchKind Match = AT.matchesType(S.Context, Ex->getType()); + Match = + handleFormatSignedness(Match, S.getDiagnostics().getDiagnosticOptions()); bool Pedantic = Match == analyze_format_string::ArgType::NoMatchPedantic; if (Match == analyze_format_string::ArgType::Match) return true; diff --git a/format-strings-signedness.c b/format-strings-signedness.c new file mode 100644 index 0000000000000..02964def10eef --- /dev/null +++ b/format-strings-signedness.c @@ -0,0 +1,107 @@ +// RUN: %clang_cc1 -std=c11 -fsyntax-only -verify -Wformat -Wformat-signedness %s + +int printf(const char *restrict format, ...); +int scanf(const char * restrict, ...); + +void test_printf_bool(_Bool x) +{ + printf("%d", x); // no-warning + printf("%u", x); // expected-warning{{format specifies type 'unsigned int' but the argument has type '_Bool'}} + printf("%x", x); // expected-warning{{format specifies type 'unsigned int' but the argument has type '_Bool'}} +} + +void test_printf_char(char x) +{ + printf("%c", x); // no-warning +} + +void test_printf_unsigned_char(unsigned char x) +{ + printf("%c", x); // no-warning +} + +void test_printf_int(int x) +{ + printf("%d", x); // no-warning + printf("%u", x); // expected-warning{{format specifies type 'unsigned int' but the argument has type 'int'}} + printf("%x", x); // expected-warning{{format specifies type 'unsigned int' but the argument has type 'int'}} +} + +void test_printf_unsigned(unsigned x) +{ + printf("%d", x); // expected-warning{{format specifies type 'int' but the argument has type 'unsigned int'}} + printf("%u", x); // no-warning + printf("%x", x); // no-warning +} + +void test_printf_long(long x) +{ + printf("%ld", x); // no-warning + printf("%lu", x); // expected-warning{{format specifies type 'unsigned long' but the argument has type 'long'}} + printf("%lx", x); // expected-warning{{format specifies type 'unsigned long' but the argument has type 'long'}} +} + +void test_printf_unsigned_long(unsigned long x) +{ + printf("%ld", x); // expected-warning{{format specifies type 'long' but the argument has type 'unsigned long'}} + printf("%lu", x); // no-warning + printf("%lx", x); // no-warning +} + +void test_printf_long_long(long long x) +{ + printf("%lld", x); // no-warning + printf("%llu", x); // expected-warning{{format specifies type 'unsigned long long' but the argument has type 'long long'}} + printf("%llx", x); // expected-warning{{format specifies type 'unsigned long long' but the argument has type 'long long'}} +} + +void test_printf_unsigned_long_long(unsigned long long x) +{ + printf("%lld", x); // expected-warning{{format specifies type 'long long' but the argument has type 'unsigned long long'}} + printf("%llu", x); // no-warning + printf("%llx", x); // no-warning +} + +void test_scanf_char(char *y) { + scanf("%c", y); // no-warning +} + +void test_scanf_unsigned_char(unsigned char *y) { + scanf("%c", y); // no-warning +} + +void test_scanf_int(int *x) { + scanf("%d", x); // no-warning + scanf("%u", x); // expected-warning{{format specifies type 'unsigned int *' but the argument has type 'int *'}} + scanf("%x", x); // expected-warning{{format specifies type 'unsigned int *' but the argument has type 'int *'}} +} + +void test_scanf_unsigned(unsigned *x) { + scanf("%d", x); // expected-warning{{format specifies type 'int *' but the argument has type 'unsigned int *'}} + scanf("%u", x); // no-warning + scanf("%x", x); // no-warning +} + +void test_scanf_long(long *x) { + scanf("%ld", x); // no-warning + scanf("%lu", x); // expected-warning{{format specifies type 'unsigned long *' but the argument has type 'long *'}} + scanf("%lx", x); // expected-warning{{format specifies type 'unsigned long *' but the argument has type 'long *'}} +} + +void test_scanf_unsigned_long(unsigned long *x) { + scanf("%ld", x); // expected-warning{{format specifies type 'long *' but the argument has type 'unsigned long *'}} + scanf("%lu", x); // no-warning + scanf("%lx", x); // no-warning +} + +void test_scanf_longlong(long long *x) { + scanf("%lld", x); // no-warning + scanf("%llu", x); // expected-warning{{format specifies type 'unsigned long long *' but the argument has type 'long long *'}} + scanf("%llx", x); // expected-warning{{format specifies type 'unsigned long long *' but the argument has type 'long long *'}} +} + +void test_scanf_unsigned_longlong(unsigned long long *x) { + scanf("%lld", x); // expected-warning{{format specifies type 'long long *' but the argument has type 'unsigned long long *'}} + scanf("%llu", x); // no-warning + scanf("%llx", x); // no-warning +} `````````` </details> https://github.com/llvm/llvm-project/pull/74440 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
