Skip to content

Commit ea92b1f

Browse files
karka228karka228
authored
[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.
1 parent c0febca commit ea92b1f

File tree

8 files changed

+379
-12
lines changed

8 files changed

+379
-12
lines changed

‎clang/docs/ReleaseNotes.rst

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,9 @@ Improvements to Clang's diagnostics
310310
- Clang now no longer diagnoses type definitions in ``offsetof`` in C23 mode.
311311
Fixes #GH83658.
312312

313+
- New ``-Wformat-signedness`` diagnostic that warn if the format string requires an
314+
unsigned argument and the argument is signed and vice versa.
315+
313316
Improvements to Clang's time-trace
314317
----------------------------------
315318

‎clang/include/clang/AST/FormatString.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,8 @@ class ArgType {
284284
/// The conversion specifier and the argument type are disallowed by the C
285285
/// standard, but are in practice harmless. For instance, "%p" and int*.
286286
NoMatchPedantic,
287+
/// The conversion specifier and the argument type have different sign.
288+
NoMatchSignedness,
287289
/// The conversion specifier and the argument type are compatible, but still
288290
/// seems likely to be an error. For instance, "%hd" and _Bool.
289291
NoMatchTypeConfusion,

‎clang/include/clang/Basic/DiagnosticGroups.td

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -985,6 +985,7 @@ def FormatSecurity : DiagGroup<"format-security">;
985985
def FormatNonStandard : DiagGroup<"format-non-iso">;
986986
def FormatY2K : DiagGroup<"format-y2k">;
987987
def FormatPedantic : DiagGroup<"format-pedantic">;
988+
def FormatSignedness : DiagGroup<"format-signedness">;
988989
def FormatTypeConfusion : DiagGroup<"format-type-confusion">;
989990

990991
def FormatOverflowNonKprintf: DiagGroup<"format-overflow-non-kprintf">;

‎clang/include/clang/Basic/DiagnosticSemaKinds.td

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9821,6 +9821,9 @@ def warn_format_conversion_argument_type_mismatch : Warning<
98219821
def warn_format_conversion_argument_type_mismatch_pedantic : Extension<
98229822
warn_format_conversion_argument_type_mismatch.Summary>,
98239823
InGroup<FormatPedantic>;
9824+
def warn_format_conversion_argument_type_mismatch_signedness : Warning<
9825+
warn_format_conversion_argument_type_mismatch.Summary>,
9826+
InGroup<FormatSignedness>, DefaultIgnore;
98249827
def warn_format_conversion_argument_type_mismatch_confusion : Warning<
98259828
warn_format_conversion_argument_type_mismatch.Summary>,
98269829
InGroup<FormatTypeConfusion>, DefaultIgnore;

‎clang/lib/AST/FormatString.cpp

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -413,7 +413,7 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
413413
return Match;
414414
if (const auto *BT = argTy->getAs<BuiltinType>()) {
415415
// Check if the only difference between them is signed vs unsigned
416-
// if true, we consider they are compatible.
416+
// if true, return match signedness.
417417
switch (BT->getKind()) {
418418
default:
419419
break;
@@ -423,44 +423,53 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
423423
[[fallthrough]];
424424
case BuiltinType::Char_S:
425425
case BuiltinType::SChar:
426+
if (T == C.UnsignedShortTy || T == C.ShortTy)
427+
return NoMatchTypeConfusion;
428+
if (T == C.UnsignedCharTy)
429+
return NoMatchSignedness;
430+
if (T == C.SignedCharTy)
431+
return Match;
432+
break;
426433
case BuiltinType::Char_U:
427434
case BuiltinType::UChar:
428435
if (T == C.UnsignedShortTy || T == C.ShortTy)
429436
return NoMatchTypeConfusion;
430-
if (T == C.UnsignedCharTy || T == C.SignedCharTy)
437+
if (T == C.UnsignedCharTy)
431438
return Match;
439+
if (T == C.SignedCharTy)
440+
return NoMatchSignedness;
432441
break;
433442
case BuiltinType::Short:
434443
if (T == C.UnsignedShortTy)
435-
return Match;
444+
return NoMatchSignedness;
436445
break;
437446
case BuiltinType::UShort:
438447
if (T == C.ShortTy)
439-
return Match;
448+
return NoMatchSignedness;
440449
break;
441450
case BuiltinType::Int:
442451
if (T == C.UnsignedIntTy)
443-
return Match;
452+
return NoMatchSignedness;
444453
break;
445454
case BuiltinType::UInt:
446455
if (T == C.IntTy)
447-
return Match;
456+
return NoMatchSignedness;
448457
break;
449458
case BuiltinType::Long:
450459
if (T == C.UnsignedLongTy)
451-
return Match;
460+
return NoMatchSignedness;
452461
break;
453462
case BuiltinType::ULong:
454463
if (T == C.LongTy)
455-
return Match;
464+
return NoMatchSignedness;
456465
break;
457466
case BuiltinType::LongLong:
458467
if (T == C.UnsignedLongLongTy)
459-
return Match;
468+
return NoMatchSignedness;
460469
break;
461470
case BuiltinType::ULongLong:
462471
if (T == C.LongLongTy)
463-
return Match;
472+
return NoMatchSignedness;
464473
break;
465474
}
466475
// "Partially matched" because of promotions?

‎clang/lib/Sema/SemaChecking.cpp

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12446,6 +12446,19 @@ isArithmeticArgumentPromotion(Sema &S, const ImplicitCastExpr *ICE) {
1244612446
S.Context.getFloatingTypeOrder(From, To) < 0;
1244712447
}
1244812448

12449+
static analyze_format_string::ArgType::MatchKind
12450+
handleFormatSignedness(analyze_format_string::ArgType::MatchKind Match,
12451+
DiagnosticsEngine &Diags, SourceLocation Loc) {
12452+
if (Match == analyze_format_string::ArgType::NoMatchSignedness) {
12453+
Match =
12454+
Diags.isIgnored(
12455+
diag::warn_format_conversion_argument_type_mismatch_signedness, Loc)
12456+
? analyze_format_string::ArgType::Match
12457+
: analyze_format_string::ArgType::NoMatch;
12458+
}
12459+
return Match;
12460+
}
12461+
1244912462
bool
1245012463
CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
1245112464
const char *StartSpecifier,
@@ -12489,6 +12502,9 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
1248912502

1249012503
ArgType::MatchKind ImplicitMatch = ArgType::NoMatch;
1249112504
ArgType::MatchKind Match = AT.matchesType(S.Context, ExprTy);
12505+
ArgType::MatchKind OrigMatch = Match;
12506+
12507+
Match = handleFormatSignedness(Match, S.getDiagnostics(), E->getExprLoc());
1249212508
if (Match == ArgType::Match)
1249312509
return true;
1249412510

@@ -12512,6 +12528,14 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
1251212528
ICE->getType() == S.Context.UnsignedIntTy) {
1251312529
// All further checking is done on the subexpression
1251412530
ImplicitMatch = AT.matchesType(S.Context, ExprTy);
12531+
if (OrigMatch == ArgType::NoMatchSignedness &&
12532+
ImplicitMatch != ArgType::NoMatchSignedness)
12533+
// If the original match was a signedness match this match on the
12534+
// implicit cast type also need to be signedness match otherwise we
12535+
// might introduce new unexpected warnings from -Wformat-signedness.
12536+
return true;
12537+
ImplicitMatch = handleFormatSignedness(
12538+
ImplicitMatch, S.getDiagnostics(), E->getExprLoc());
1251512539
if (ImplicitMatch == ArgType::Match)
1251612540
return true;
1251712541
}
@@ -12633,6 +12657,7 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
1263312657
case ArgType::Match:
1263412658
case ArgType::MatchPromotion:
1263512659
case ArgType::NoMatchPromotionTypeConfusion:
12660+
case ArgType::NoMatchSignedness:
1263612661
llvm_unreachable("expected non-matching");
1263712662
case ArgType::NoMatchPedantic:
1263812663
Diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
@@ -12668,8 +12693,10 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
1266812693
CastFix << (S.LangOpts.CPlusPlus ? ">" : ")");
1266912694

1267012695
SmallVector<FixItHint,4> Hints;
12671-
if (AT.matchesType(S.Context, IntendedTy) != ArgType::Match ||
12672-
ShouldNotPrintDirectly)
12696+
ArgType::MatchKind IntendedMatch = AT.matchesType(S.Context, IntendedTy);
12697+
IntendedMatch = handleFormatSignedness(IntendedMatch, S.getDiagnostics(),
12698+
E->getExprLoc());
12699+
if ((IntendedMatch != ArgType::Match) || ShouldNotPrintDirectly)
1267312700
Hints.push_back(FixItHint::CreateReplacement(SpecRange, os.str()));
1267412701

1267512702
if (const CStyleCastExpr *CCast = dyn_cast<CStyleCastExpr>(E)) {
@@ -12738,6 +12765,7 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
1273812765
case ArgType::Match:
1273912766
case ArgType::MatchPromotion:
1274012767
case ArgType::NoMatchPromotionTypeConfusion:
12768+
case ArgType::NoMatchSignedness:
1274112769
llvm_unreachable("expected non-matching");
1274212770
case ArgType::NoMatchPedantic:
1274312771
Diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
@@ -12949,6 +12977,7 @@ bool CheckScanfHandler::HandleScanfSpecifier(
1294912977

1295012978
analyze_format_string::ArgType::MatchKind Match =
1295112979
AT.matchesType(S.Context, Ex->getType());
12980+
Match = handleFormatSignedness(Match, S.getDiagnostics(), Ex->getExprLoc());
1295212981
bool Pedantic = Match == analyze_format_string::ArgType::NoMatchPedantic;
1295312982
if (Match == analyze_format_string::ArgType::Match)
1295412983
return true;

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

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
// RUN: cp %s %t
2+
// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -Wformat -Wformat-signedness -fixit %t
3+
// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wformat -Wformat-signedness -Werror %t
4+
// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -E -o - %t | FileCheck %s
5+
6+
#include <limits.h>
7+
8+
int printf(const char *restrict format, ...);
9+
10+
void test_printf_int(int x)
11+
{
12+
printf("%u", x);
13+
}
14+
15+
void test_printf_unsigned(unsigned x)
16+
{
17+
printf("%d", x);
18+
}
19+
20+
void test_printf_long(long x)
21+
{
22+
printf("%lu", x);
23+
}
24+
25+
void test_printf_unsigned_long(unsigned long x)
26+
{
27+
printf("%ld", x);
28+
}
29+
30+
void test_printf_long_long(long long x)
31+
{
32+
printf("%llu", x);
33+
}
34+
35+
void test_printf_unsigned_long_long(unsigned long long x)
36+
{
37+
printf("%lld", x);
38+
}
39+
40+
enum enum_int {
41+
minus_1 = -1
42+
};
43+
44+
void test_printf_enum_int(enum enum_int x)
45+
{
46+
printf("%u", x);
47+
}
48+
49+
enum enum_unsigned {
50+
zero = 0
51+
};
52+
53+
void test_printf_enum_unsigned(enum enum_unsigned x)
54+
{
55+
printf("%d", x);
56+
}
57+
58+
enum enum_long {
59+
minus_one = -1,
60+
int_val = INT_MAX,
61+
unsigned_val = (unsigned)INT_MIN
62+
};
63+
64+
void test_printf_enum_long(enum enum_long x)
65+
{
66+
printf("%lu", x);
67+
}
68+
69+
enum enum_unsigned_long {
70+
uint_max_plus = (unsigned long)UINT_MAX+1,
71+
};
72+
73+
void test_printf_enum_unsigned_long(enum enum_unsigned_long x)
74+
{
75+
printf("%ld", x);
76+
}
77+
78+
// Validate the fixes.
79+
// CHECK: void test_printf_int(int x)
80+
// CHECK: printf("%d", x);
81+
// CHECK: void test_printf_unsigned(unsigned x)
82+
// CHECK: printf("%u", x);
83+
// CHECK: void test_printf_long(long x)
84+
// CHECK: printf("%ld", x);
85+
// CHECK: void test_printf_unsigned_long(unsigned long x)
86+
// CHECK: printf("%lu", x);
87+
// CHECK: void test_printf_long_long(long long x)
88+
// CHECK: printf("%lld", x);
89+
// CHECK: void test_printf_unsigned_long_long(unsigned long long x)
90+
// CHECK: printf("%llu", x);
91+
// CHECK: void test_printf_enum_int(enum enum_int x)
92+
// CHECK: printf("%d", x);
93+
// CHECK: void test_printf_enum_unsigned(enum enum_unsigned x)
94+
// CHECK: printf("%u", x);
95+
// CHECK: void test_printf_enum_long(enum enum_long x)
96+
// CHECK: printf("%ld", x);
97+
// CHECK: void test_printf_enum_unsigned_long(enum enum_unsigned_long x)
98+
// CHECK: printf("%lu", x);

0 commit comments

Comments
 (0)