-
Notifications
You must be signed in to change notification settings - Fork 14.7k
Fix Clang bug that -Wformat-signedness is not reported properly. #150962
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The goal is to correctly identify diagnostics that are emitted by virtue of -Wformat-signedness. Before this change, diagnostic messages triggered by -Wformat-signedness might look like: format specifies type 'unsigned int' but the argument has type 'int' [-Wformat] signedness of format specifier 'u' is incompatible with 'c' [-Wformat] With this change: format specifies type 'unsigned int' but the argument has type 'int', which differs in signedness [-Wformat-signedness] signedness of format specifier 'u' is incompatible with 'c' [-Wformat-signedness] Fix: - handleFormatSignedness can now return NoMatchSignedness. Callers handle this. - warn_format_conversion_argument_type extends the message it used to emit by a string that mentions "signedness". - warn_format_cmp_specifier_sign_mismatch is now correctly categorized as a diagnostic controlled by -Wformat-signedness.
@llvm/pr-subscribers-clang Author: None (DeanSturtevant1) ChangesThe goal is to correctly identify diagnostics that are emitted by virtue of -Wformat-signedness. Before this change, diagnostic messages triggered by -Wformat-signedness might look like: Fix:
Full diff: https://github.com/llvm/llvm-project/pull/150962.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index da4216dbda4c9..752ce93190d37 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10398,9 +10398,11 @@ 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_signedness
+ : Warning<warn_format_conversion_argument_type_mismatch.Summary>,
+ "format specifies type %0 but the argument has %select{type|underlying "
+ "type}2 %1, which differs in signedness" >
+ , InGroup<FormatSignedness>, DefaultIgnore;
def warn_format_conversion_argument_type_mismatch_confusion : Warning<
warn_format_conversion_argument_type_mismatch.Summary>,
InGroup<FormatTypeConfusion>, DefaultIgnore;
@@ -10512,8 +10514,10 @@ def warn_format_cmp_sensitivity_mismatch : Warning<
"it should be %select{unspecified|private|public|sensitive}1">, InGroup<Format>;
def warn_format_cmp_specifier_mismatch : Warning<
"format specifier '%0' is incompatible with '%1'">, InGroup<Format>;
-def warn_format_cmp_specifier_sign_mismatch : Warning<
- "signedness of format specifier '%0' is incompatible with '%1'">, InGroup<Format>;
+def warn_format_cmp_specifier_sign_mismatch
+ : Warning<"signedness of format specifier '%0' is incompatible with '%1'">,
+ InGroup<FormatSignedness>,
+ DefaultIgnore;
def warn_format_cmp_specifier_mismatch_pedantic : Extension<
warn_format_cmp_specifier_sign_mismatch.Summary>, InGroup<FormatPedantic>;
def note_format_cmp_with : Note<
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index c74b67106ad74..a9584f8273406 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -40,6 +40,7 @@
#include "clang/AST/UnresolvedSet.h"
#include "clang/Basic/AddressSpaces.h"
#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/IdentifierTable.h"
#include "clang/Basic/LLVM.h"
#include "clang/Basic/LangOptions.h"
@@ -7645,17 +7646,13 @@ bool EquatableFormatArgument::VerifyCompatible(
break;
case MK::NoMatchSignedness:
- if (!S.getDiagnostics().isIgnored(
- diag::warn_format_conversion_argument_type_mismatch_signedness,
- ElementLoc)) {
- EmitDiagnostic(S,
- S.PDiag(diag::warn_format_cmp_specifier_sign_mismatch)
- << buildFormatSpecifier()
- << Other.buildFormatSpecifier(),
- FmtExpr, InFunctionCall);
- HadError = S.Diag(Other.ElementLoc, diag::note_format_cmp_with)
- << 0 << Other.Range;
- }
+ EmitDiagnostic(S,
+ S.PDiag(diag::warn_format_cmp_specifier_sign_mismatch)
+ << buildFormatSpecifier()
+ << Other.buildFormatSpecifier(),
+ FmtExpr, InFunctionCall);
+ HadError = S.Diag(Other.ElementLoc, diag::note_format_cmp_with)
+ << 0 << Other.Range;
break;
}
return !HadError;
@@ -8189,11 +8186,14 @@ 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 =
+ if (Diags.isIgnored(
+ diag::warn_format_conversion_argument_type_mismatch_signedness,
+ Loc) ||
Diags.isIgnored(
- diag::warn_format_conversion_argument_type_mismatch_signedness, Loc)
- ? analyze_format_string::ArgType::Match
- : analyze_format_string::ArgType::NoMatch;
+ // Arbitrary -Wformat diagnostic to detect -Wno-format:
+ diag::warn_format_conversion_argument_type_mismatch, Loc)) {
+ return analyze_format_string::ArgType::Match;
+ }
}
return Match;
}
@@ -8407,8 +8407,10 @@ 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::NoMatchSignedness:
+ Diag = diag::warn_format_conversion_argument_type_mismatch_signedness;
+ break;
case ArgType::NoMatchPedantic:
Diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
break;
@@ -8733,9 +8735,10 @@ 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;
+ bool Pedantic = Match == analyze_format_string::ArgType::NoMatchPedantic;
+ bool Signedness = Match == analyze_format_string::ArgType::NoMatchSignedness;
ScanfSpecifier fixedFS = FS;
bool Success = fixedFS.fixType(Ex->getType(), Ex->IgnoreImpCasts()->getType(),
@@ -8743,7 +8746,9 @@ bool CheckScanfHandler::HandleScanfSpecifier(
unsigned Diag =
Pedantic ? diag::warn_format_conversion_argument_type_mismatch_pedantic
- : diag::warn_format_conversion_argument_type_mismatch;
+ : Signedness
+ ? diag::warn_format_conversion_argument_type_mismatch_signedness
+ : diag::warn_format_conversion_argument_type_mismatch;
if (Success) {
// Get the fix string from the fixed format specifier.
diff --git a/clang/test/Sema/format-strings-signedness.c b/clang/test/Sema/format-strings-signedness.c
index d5a8140d9ef8a..773ff412ac177 100644
--- a/clang/test/Sema/format-strings-signedness.c
+++ b/clang/test/Sema/format-strings-signedness.c
@@ -39,13 +39,13 @@ void test_printf_unsigned_char(unsigned char x)
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'}}
+ printf("%u", x); // expected-warning{{format specifies type 'unsigned int' but the argument has type 'int', which differs in signedness}}
+ printf("%x", x); // expected-warning{{format specifies type 'unsigned int' but the argument has type 'int', which differs in signedness}}
}
void test_printf_unsigned(unsigned x)
{
- printf("%d", x); // expected-warning{{format specifies type 'int' but the argument has type 'unsigned int'}}
+ printf("%d", x); // expected-warning{{format specifies type 'int' but the argument has type 'unsigned int', which differs in signedness}}
printf("%u", x); // no-warning
printf("%x", x); // no-warning
}
@@ -53,13 +53,13 @@ void test_printf_unsigned(unsigned x)
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'}}
+ printf("%lu", x); // expected-warning{{format specifies type 'unsigned long' but the argument has type 'long', which differs in signedness}}
+ printf("%lx", x); // expected-warning{{format specifies type 'unsigned long' but the argument has type 'long', which differs in signedness}}
}
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("%ld", x); // expected-warning{{format specifies type 'long' but the argument has type 'unsigned long', which differs in signedness}}
printf("%lu", x); // no-warning
printf("%lx", x); // no-warning
}
@@ -67,13 +67,13 @@ void test_printf_unsigned_long(unsigned long x)
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'}}
+ printf("%llu", x); // expected-warning{{format specifies type 'unsigned long long' but the argument has type 'long long', which differs in signedness}}
+ printf("%llx", x); // expected-warning{{format specifies type 'unsigned long long' but the argument has type 'long long', which differs in signedness}}
}
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("%lld", x); // expected-warning{{format specifies type 'long long' but the argument has type 'unsigned long long', which differs in signedness}}
printf("%llu", x); // no-warning
printf("%llx", x); // no-warning
}
@@ -85,8 +85,8 @@ enum enum_int {
void test_printf_enum_int(enum enum_int x)
{
printf("%d", x); // no-warning
- printf("%u", x); // expected-warning{{format specifies type 'unsigned int' but the argument has underlying type 'int'}}
- printf("%x", x); // expected-warning{{format specifies type 'unsigned int' but the argument has underlying type 'int'}}
+ printf("%u", x); // expected-warning{{format specifies type 'unsigned int' but the argument has underlying type 'int', which differs in signedness}}
+ printf("%x", x); // expected-warning{{format specifies type 'unsigned int' but the argument has underlying type 'int', which differs in signedness}}
}
#ifndef _WIN32 // Disabled due to enums have different underlying type on _WIN32
@@ -96,7 +96,7 @@ enum enum_unsigned {
void test_printf_enum_unsigned(enum enum_unsigned x)
{
- printf("%d", x); // expected-warning{{format specifies type 'int' but the argument has underlying type 'unsigned int'}}
+ printf("%d", x); // expected-warning{{format specifies type 'int' but the argument has underlying type 'unsigned int', which differs in signedness}}
printf("%u", x); // no-warning
printf("%x", x); // no-warning
}
@@ -110,8 +110,8 @@ enum enum_long {
void test_printf_enum_long(enum enum_long x)
{
printf("%ld", x); // no-warning
- printf("%lu", x); // expected-warning{{format specifies type 'unsigned long' but the argument has underlying type 'long'}}
- printf("%lx", x); // expected-warning{{format specifies type 'unsigned long' but the argument has underlying type 'long'}}
+ printf("%lu", x); // expected-warning{{format specifies type 'unsigned long' but the argument has underlying type 'long', which differs in signedness}}
+ printf("%lx", x); // expected-warning{{format specifies type 'unsigned long' but the argument has underlying type 'long', which differs in signedness}}
}
enum enum_unsigned_long {
@@ -120,7 +120,7 @@ enum enum_unsigned_long {
void test_printf_enum_unsigned_long(enum enum_unsigned_long x)
{
- printf("%ld", x); // expected-warning{{format specifies type 'long' but the argument has underlying type 'unsigned long'}}
+ printf("%ld", x); // expected-warning{{format specifies type 'long' but the argument has underlying type 'unsigned long', which differs in signedness}}
printf("%lu", x); // no-warning
printf("%lx", x); // no-warning
}
@@ -136,61 +136,61 @@ void test_scanf_unsigned_char(unsigned char *y) {
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 *'}}
+ scanf("%u", x); // expected-warning{{format specifies type 'unsigned int *' but the argument has type 'int *', which differs in signedness}}
+ scanf("%x", x); // expected-warning{{format specifies type 'unsigned int *' but the argument has type 'int *', which differs in signedness}}
}
void test_scanf_unsigned(unsigned *x) {
- scanf("%d", x); // expected-warning{{format specifies type 'int *' but the argument has type 'unsigned int *'}}
+ scanf("%d", x); // expected-warning{{format specifies type 'int *' but the argument has type 'unsigned int *', which differs in signedness}}
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 *'}}
+ scanf("%lu", x); // expected-warning{{format specifies type 'unsigned long *' but the argument has type 'long *', which differs in signedness}}
+ scanf("%lx", x); // expected-warning{{format specifies type 'unsigned long *' but the argument has type 'long *', which differs in signedness}}
}
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("%ld", x); // expected-warning{{format specifies type 'long *' but the argument has type 'unsigned long *', which differs in signedness}}
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 *'}}
+ scanf("%llu", x); // expected-warning{{format specifies type 'unsigned long long *' but the argument has type 'long long *', which differs in signedness}}
+ scanf("%llx", x); // expected-warning{{format specifies type 'unsigned long long *' but the argument has type 'long long *', which differs in signedness}}
}
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("%lld", x); // expected-warning{{format specifies type 'long long *' but the argument has type 'unsigned long long *', which differs in signedness}}
scanf("%llu", x); // no-warning
scanf("%llx", x); // no-warning
}
void test_scanf_enum_int(enum enum_int *x) {
scanf("%d", x); // no-warning
- scanf("%u", x); // expected-warning{{format specifies type 'unsigned int *' but the argument has type 'enum enum_int *'}}
- scanf("%x", x); // expected-warning{{format specifies type 'unsigned int *' but the argument has type 'enum enum_int *'}}
+ scanf("%u", x); // expected-warning{{format specifies type 'unsigned int *' but the argument has type 'enum enum_int *', which differs in signedness}}
+ scanf("%x", x); // expected-warning{{format specifies type 'unsigned int *' but the argument has type 'enum enum_int *', which differs in signedness}}
}
#ifndef _WIN32 // Disabled due to enums have different underlying type on _WIN32
void test_scanf_enum_unsigned(enum enum_unsigned *x) {
- scanf("%d", x); // expected-warning{{format specifies type 'int *' but the argument has type 'enum enum_unsigned *'}}
+ scanf("%d", x); // expected-warning{{format specifies type 'int *' but the argument has type 'enum enum_unsigned *', which differs in signedness}}
scanf("%u", x); // no-warning
scanf("%x", x); // no-warning
}
void test_scanf_enum_long(enum enum_long *x) {
scanf("%ld", x); // no-warning
- scanf("%lu", x); // expected-warning{{format specifies type 'unsigned long *' but the argument has type 'enum enum_long *'}}
- scanf("%lx", x); // expected-warning{{format specifies type 'unsigned long *' but the argument has type 'enum enum_long *'}}
+ scanf("%lu", x); // expected-warning{{format specifies type 'unsigned long *' but the argument has type 'enum enum_long *', which differs in signedness}}
+ scanf("%lx", x); // expected-warning{{format specifies type 'unsigned long *' but the argument has type 'enum enum_long *', which differs in signedness}}
}
void test_scanf_enum_unsigned_long(enum enum_unsigned_long *x) {
- scanf("%ld", x); // expected-warning{{format specifies type 'long *' but the argument has type 'enum enum_unsigned_long *'}}
+ scanf("%ld", x); // expected-warning{{format specifies type 'long *' but the argument has type 'enum enum_unsigned_long *', which differs in signedness}}
scanf("%lu", x); // no-warning
scanf("%lx", x); // no-warning
}
|
Fixing build ... |
@AaronBallman,@karka228,@hazohelet -- would you care to review this pull request?
|
@AaronBallman,@karka228,@hazohelet - please - any thoughts?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This need a release note, that is in clang/docs/ReleaseNotes.txt
First a general note. The reason why -Wformat-signedness is implemented a bit weird in clang is that we wanted the warning to be more or less compatible with how it is implemented in gcc. That way a user that change from gcc to clang (or use compile with both clang and gcc) don't run into any undesirable surprises. The original discussion about gcc compatibility can be found in the original PR #74440 With that said, the clang implementation isn't entirely compatible with gcc and we can if we want introduce new incompatibilities if we find it reasonable. I have only looked briefly on your patch and I think the added ", which differs in signedness" to the warning message is a good thing. |
Note that PR 74440 contains a review comment that the following property would be good: This change should in no way change under what conditions the diagnostics are emitted. I included release notes. PTAL. |
I think we all agree that the way gcc implemented this from the beginning is strange and that implied that the implementation in clang also is strange. I think I leave this up to @AaronBallman to decide if the gcc-compatible in point 6 which is that the diagnostics are associated with -Wformat, discussed in the original PR 74440, is important or not. |
@AaronBallman Ping. Can you please take a look at how we should handle the gcc incompatible in the warning mention above. Is it fine to go ahead anyway? |
An explanation for why I wrote this pull request: |
The goal is to correctly identify diagnostics that are emitted by virtue of -Wformat-signedness.
Before this change, diagnostic messages triggered by -Wformat-signedness might look like:
format specifies type 'unsigned int' but the argument has type 'int' [-Wformat]
signedness of format specifier 'u' is incompatible with 'c' [-Wformat]
With this change:
format specifies type 'unsigned int' but the argument has type 'int', which differs in signedness [-Wformat-signedness]
signedness of format specifier 'u' is incompatible with 'c' [-Wformat-signedness]
Fix:
mentions "signedness".
diagnostic controlled by -Wformat-signedness.