[clang] Update -Wformat warnings for fixed-point format specifiers #82855
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PiJoules
requested review from
petrhosek,
MaskRay,
bevin-hansson and
michaelrj-google
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (PiJoules) ChangesISO/IEC TR 18037 defines %r, %R, %k, and %K for fixed point format specifiers. -Wformat should not warn on these when they are provided. Full diff: https://github.com/llvm/llvm-project/pull/82855.diff 6 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 12ce9af1e53f63..ff6b64c7f72d57 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -2981,6 +2981,10 @@ class ASTContext : public RefCountedBase<ASTContext> {
// corresponding saturated type for a given fixed point type.
QualType getCorrespondingSaturatedType(QualType Ty) const;
+ // Per ISO N1169, this method accepts fixed point types and returns the
+ // corresponding non-saturated type for a given fixed point type.
+ QualType getCorrespondingUnsaturatedType(QualType Ty) const;
+
// This method accepts fixed point types and returns the corresponding signed
// type. Unlike getCorrespondingUnsignedType(), this only accepts unsigned
// fixed point types because there are unsigned integer types like bool and
diff --git a/clang/include/clang/AST/FormatString.h b/clang/include/clang/AST/FormatString.h
index 5c4ad9baaef608..e2232fb4a47153 100644
--- a/clang/include/clang/AST/FormatString.h
+++ b/clang/include/clang/AST/FormatString.h
@@ -171,6 +171,14 @@ class ConversionSpecifier {
ZArg, // MS extension
+ // ISO/IEC TR 18037 (fixed-point) specific specifiers.
+ kArg, // %k for signed accum types
+ KArg, // %K for unsigned accum types
+ rArg, // %r for signed fract types
+ RArg, // %R for unsigned fract types
+ FixedPointArgBeg = kArg,
+ FixedPointArgEnd = RArg,
+
// Objective-C specific specifiers.
ObjCObjArg, // '@'
ObjCBeg = ObjCObjArg,
@@ -237,6 +245,9 @@ class ConversionSpecifier {
bool isDoubleArg() const {
return kind >= DoubleArgBeg && kind <= DoubleArgEnd;
}
+ bool isFixedPointArg() const {
+ return kind >= FixedPointArgBeg && kind <= FixedPointArgEnd;
+ }
const char *toString() const;
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index c475c841233c59..5a8fae76a43a4d 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -13314,6 +13314,42 @@ QualType ASTContext::getCommonSugaredType(QualType X, QualType Y,
return R;
}
+QualType ASTContext::getCorrespondingUnsaturatedType(QualType Ty) const {
+ assert(Ty->isFixedPointType());
+
+ if (Ty->isUnsaturatedFixedPointType())
+ return Ty;
+
+ switch (Ty->castAs<BuiltinType>()->getKind()) {
+ default:
+ llvm_unreachable("Not a saturated fixed point type!");
+ case BuiltinType::SatShortAccum:
+ return ShortAccumTy;
+ case BuiltinType::SatAccum:
+ return AccumTy;
+ case BuiltinType::SatLongAccum:
+ return LongAccumTy;
+ case BuiltinType::SatUShortAccum:
+ return UnsignedShortAccumTy;
+ case BuiltinType::SatUAccum:
+ return UnsignedAccumTy;
+ case BuiltinType::SatULongAccum:
+ return UnsignedLongAccumTy;
+ case BuiltinType::SatShortFract:
+ return ShortFractTy;
+ case BuiltinType::SatFract:
+ return FractTy;
+ case BuiltinType::SatLongFract:
+ return LongFractTy;
+ case BuiltinType::SatUShortFract:
+ return UnsignedShortFractTy;
+ case BuiltinType::SatUFract:
+ return UnsignedFractTy;
+ case BuiltinType::SatULongFract:
+ return UnsignedLongFractTy;
+ }
+}
+
QualType ASTContext::getCorrespondingSaturatedType(QualType Ty) const {
assert(Ty->isFixedPointType());
diff --git a/clang/lib/AST/FormatString.cpp b/clang/lib/AST/FormatString.cpp
index c5d14b4af7ff15..0c80ad109ccbb2 100644
--- a/clang/lib/AST/FormatString.cpp
+++ b/clang/lib/AST/FormatString.cpp
@@ -403,6 +403,10 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
else if (ETy->isUnscopedEnumerationType())
argTy = ETy->getDecl()->getIntegerType();
}
+
+ if (argTy->isSaturatedFixedPointType())
+ argTy = C.getCorrespondingUnsaturatedType(argTy);
+
argTy = C.getCanonicalType(argTy).getUnqualifiedType();
if (T == argTy)
@@ -761,6 +765,16 @@ const char *ConversionSpecifier::toString() const {
// MS specific specifiers.
case ZArg: return "Z";
+
+ // ISO/IEC TR 18037 (fixed-point) specific specifiers.
+ case rArg:
+ return "r";
+ case RArg:
+ return "R";
+ case kArg:
+ return "k";
+ case KArg:
+ return "K";
}
return nullptr;
}
@@ -825,6 +839,9 @@ bool FormatSpecifier::hasValidLengthModifier(const TargetInfo &Target,
if (LO.OpenCL && CS.isDoubleArg())
return !VectorNumElts.isInvalid();
+ if (CS.isFixedPointArg())
+ return true;
+
if (Target.getTriple().isOSMSVCRT()) {
switch (CS.getKind()) {
case ConversionSpecifier::cArg:
@@ -877,6 +894,9 @@ bool FormatSpecifier::hasValidLengthModifier(const TargetInfo &Target,
return true;
}
+ if (CS.isFixedPointArg())
+ return true;
+
switch (CS.getKind()) {
case ConversionSpecifier::bArg:
case ConversionSpecifier::BArg:
@@ -1043,6 +1063,11 @@ bool FormatSpecifier::hasStandardConversionSpecifier(
case ConversionSpecifier::UArg:
case ConversionSpecifier::ZArg:
return false;
+ case ConversionSpecifier::rArg:
+ case ConversionSpecifier::RArg:
+ case ConversionSpecifier::kArg:
+ case ConversionSpecifier::KArg:
+ return LangOpt.FixedPoint;
}
llvm_unreachable("Invalid ConversionSpecifier Kind!");
}
diff --git a/clang/lib/AST/PrintfFormatString.cpp b/clang/lib/AST/PrintfFormatString.cpp
index 3b09ca40bd2a53..3eb497767f2ec1 100644
--- a/clang/lib/AST/PrintfFormatString.cpp
+++ b/clang/lib/AST/PrintfFormatString.cpp
@@ -348,6 +348,8 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
case 'r':
if (isFreeBSDKPrintf)
k = ConversionSpecifier::FreeBSDrArg; // int
+ else if (LO.FixedPoint)
+ k = ConversionSpecifier::rArg;
break;
case 'y':
if (isFreeBSDKPrintf)
@@ -373,6 +375,20 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
if (Target.getTriple().isOSMSVCRT())
k = ConversionSpecifier::ZArg;
break;
+ // ISO/IEC TR 18037 (fixed-point) specific.
+ // NOTE: 'r' is handled up above since FreeBSD also supports %r.
+ case 'k':
+ if (LO.FixedPoint)
+ k = ConversionSpecifier::kArg;
+ break;
+ case 'K':
+ if (LO.FixedPoint)
+ k = ConversionSpecifier::KArg;
+ break;
+ case 'R':
+ if (LO.FixedPoint)
+ k = ConversionSpecifier::RArg;
+ break;
}
// Check to see if we used the Objective-C modifier flags with
@@ -627,6 +643,9 @@ ArgType PrintfSpecifier::getScalarArgType(ASTContext &Ctx,
}
}
+ if (CS.isFixedPointArg() && !Ctx.getLangOpts().FixedPoint)
+ return ArgType::Invalid();
+
switch (CS.getKind()) {
case ConversionSpecifier::sArg:
if (LM.getKind() == LengthModifier::AsWideChar) {
@@ -658,6 +677,50 @@ ArgType PrintfSpecifier::getScalarArgType(ASTContext &Ctx,
return ArgType::CPointerTy;
case ConversionSpecifier::ObjCObjArg:
return ArgType::ObjCPointerTy;
+ case ConversionSpecifier::kArg:
+ switch (LM.getKind()) {
+ case LengthModifier::None:
+ return Ctx.AccumTy;
+ case LengthModifier::AsShort:
+ return Ctx.ShortAccumTy;
+ case LengthModifier::AsLong:
+ return Ctx.LongAccumTy;
+ default:
+ return ArgType::Invalid();
+ }
+ case ConversionSpecifier::KArg:
+ switch (LM.getKind()) {
+ case LengthModifier::None:
+ return Ctx.UnsignedAccumTy;
+ case LengthModifier::AsShort:
+ return Ctx.UnsignedShortAccumTy;
+ case LengthModifier::AsLong:
+ return Ctx.UnsignedLongAccumTy;
+ default:
+ return ArgType::Invalid();
+ }
+ case ConversionSpecifier::rArg:
+ switch (LM.getKind()) {
+ case LengthModifier::None:
+ return Ctx.FractTy;
+ case LengthModifier::AsShort:
+ return Ctx.ShortFractTy;
+ case LengthModifier::AsLong:
+ return Ctx.LongFractTy;
+ default:
+ return ArgType::Invalid();
+ }
+ case ConversionSpecifier::RArg:
+ switch (LM.getKind()) {
+ case LengthModifier::None:
+ return Ctx.UnsignedFractTy;
+ case LengthModifier::AsShort:
+ return Ctx.UnsignedShortFractTy;
+ case LengthModifier::AsLong:
+ return Ctx.UnsignedLongFractTy;
+ default:
+ return ArgType::Invalid();
+ }
default:
break;
}
@@ -955,6 +1018,10 @@ bool PrintfSpecifier::hasValidPlusPrefix() const {
case ConversionSpecifier::AArg:
case ConversionSpecifier::FreeBSDrArg:
case ConversionSpecifier::FreeBSDyArg:
+ case ConversionSpecifier::rArg:
+ case ConversionSpecifier::RArg:
+ case ConversionSpecifier::kArg:
+ case ConversionSpecifier::KArg:
return true;
default:
@@ -966,7 +1033,7 @@ bool PrintfSpecifier::hasValidAlternativeForm() const {
if (!HasAlternativeForm)
return true;
- // Alternate form flag only valid with the bBoxXaAeEfFgG conversions
+ // Alternate form flag only valid with the bBoxXaAeEfFgGrRkK conversions
switch (CS.getKind()) {
case ConversionSpecifier::bArg:
case ConversionSpecifier::BArg:
@@ -984,6 +1051,10 @@ bool PrintfSpecifier::hasValidAlternativeForm() const {
case ConversionSpecifier::GArg:
case ConversionSpecifier::FreeBSDrArg:
case ConversionSpecifier::FreeBSDyArg:
+ case ConversionSpecifier::rArg:
+ case ConversionSpecifier::RArg:
+ case ConversionSpecifier::kArg:
+ case ConversionSpecifier::KArg:
return true;
default:
@@ -995,7 +1066,7 @@ bool PrintfSpecifier::hasValidLeadingZeros() const {
if (!HasLeadingZeroes)
return true;
- // Leading zeroes flag only valid with the bBdiouxXaAeEfFgG conversions
+ // Leading zeroes flag only valid with the bBdiouxXaAeEfFgGrRkK conversions
switch (CS.getKind()) {
case ConversionSpecifier::bArg:
case ConversionSpecifier::BArg:
@@ -1018,6 +1089,10 @@ bool PrintfSpecifier::hasValidLeadingZeros() const {
case ConversionSpecifier::GArg:
case ConversionSpecifier::FreeBSDrArg:
case ConversionSpecifier::FreeBSDyArg:
+ case ConversionSpecifier::rArg:
+ case ConversionSpecifier::RArg:
+ case ConversionSpecifier::kArg:
+ case ConversionSpecifier::KArg:
return true;
default:
@@ -1044,6 +1119,10 @@ bool PrintfSpecifier::hasValidSpacePrefix() const {
case ConversionSpecifier::AArg:
case ConversionSpecifier::FreeBSDrArg:
case ConversionSpecifier::FreeBSDyArg:
+ case ConversionSpecifier::rArg:
+ case ConversionSpecifier::RArg:
+ case ConversionSpecifier::kArg:
+ case ConversionSpecifier::KArg:
return true;
default:
@@ -1089,7 +1168,7 @@ bool PrintfSpecifier::hasValidPrecision() const {
if (Precision.getHowSpecified() == OptionalAmount::NotSpecified)
return true;
- // Precision is only valid with the bBdiouxXaAeEfFgGsP conversions
+ // Precision is only valid with the bBdiouxXaAeEfFgGsPrRkK conversions
switch (CS.getKind()) {
case ConversionSpecifier::bArg:
case ConversionSpecifier::BArg:
@@ -1114,6 +1193,10 @@ bool PrintfSpecifier::hasValidPrecision() const {
case ConversionSpecifier::FreeBSDrArg:
case ConversionSpecifier::FreeBSDyArg:
case ConversionSpecifier::PArg:
+ case ConversionSpecifier::rArg:
+ case ConversionSpecifier::RArg:
+ case ConversionSpecifier::kArg:
+ case ConversionSpecifier::KArg:
return true;
default:
diff --git a/clang/test/Sema/format-fixed-point.c b/clang/test/Sema/format-fixed-point.c
new file mode 100644
index 00000000000000..a90a205ab1adb7
--- /dev/null
+++ b/clang/test/Sema/format-fixed-point.c
@@ -0,0 +1,134 @@
+// RUN: %clang_cc1 -ffixed-point -fsyntax-only -verify -Wformat -isystem %S/Inputs %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -isystem %S/Inputs %s -DWITHOUT_FIXED_POINT
+
+int printf(const char *restrict, ...);
+
+short s;
+unsigned short us;
+int i;
+unsigned int ui;
+long l;
+unsigned long ul;
+float fl;
+double d;
+char c;
+unsigned char uc;
+
+#ifndef WITHOUT_FIXED_POINT
+short _Fract sf;
+_Fract f;
+long _Fract lf;
+unsigned short _Fract usf;
+unsigned _Fract uf;
+unsigned long _Fract ulf;
+short _Accum sa;
+_Accum a;
+long _Accum la;
+unsigned short _Accum usa;
+unsigned _Accum ua;
+unsigned long _Accum ula;
+_Sat short _Fract sat_sf;
+_Sat _Fract sat_f;
+_Sat long _Fract sat_lf;
+_Sat unsigned short _Fract sat_usf;
+_Sat unsigned _Fract sat_uf;
+_Sat unsigned long _Fract sat_ulf;
+_Sat short _Accum sat_sa;
+_Sat _Accum sat_a;
+_Sat long _Accum sat_la;
+_Sat unsigned short _Accum sat_usa;
+_Sat unsigned _Accum sat_ua;
+_Sat unsigned long _Accum sat_ula;
+
+void test_invalid_args(void) {
+ /// None of these should match against a fixed point type.
+ printf("%r", s); // expected-warning{{format specifies type '_Fract' but the argument has type 'short'}}
+ printf("%r", us); // expected-warning{{format specifies type '_Fract' but the argument has type 'unsigned short'}}
+ printf("%r", i); // expected-warning{{format specifies type '_Fract' but the argument has type 'int'}}
+ printf("%r", ui); // expected-warning{{format specifies type '_Fract' but the argument has type 'unsigned int'}}
+ printf("%r", l); // expected-warning{{format specifies type '_Fract' but the argument has type 'long'}}
+ printf("%r", ul); // expected-warning{{format specifies type '_Fract' but the argument has type 'unsigned long'}}
+ printf("%r", fl); // expected-warning{{format specifies type '_Fract' but the argument has type 'float'}}
+ printf("%r", d); // expected-warning{{format specifies type '_Fract' but the argument has type 'double'}}
+ printf("%r", c); // expected-warning{{format specifies type '_Fract' but the argument has type 'char'}}
+ printf("%r", uc); // expected-warning{{format specifies type '_Fract' but the argument has type 'unsigned char'}}
+}
+
+void test_fixed_point_specifiers(void) {
+ printf("%r", f);
+ printf("%R", uf);
+ printf("%k", a);
+ printf("%K", ua);
+
+ /// Test different sizes.
+ printf("%r", sf); // expected-warning{{format specifies type '_Fract' but the argument has type 'short _Fract'}}
+ printf("%r", lf); // expected-warning{{format specifies type '_Fract' but the argument has type 'long _Fract'}}
+ printf("%R", usf); // expected-warning{{format specifies type 'unsigned _Fract' but the argument has type 'unsigned short _Fract'}}
+ printf("%R", ulf); // expected-warning{{format specifies type 'unsigned _Fract' but the argument has type 'unsigned long _Fract'}}
+ printf("%k", sa); // expected-warning{{format specifies type '_Accum' but the argument has type 'short _Accum'}}
+ printf("%k", la); // expected-warning{{format specifies type '_Accum' but the argument has type 'long _Accum'}}
+ printf("%K", usa); // expected-warning{{format specifies type 'unsigned _Accum' but the argument has type 'unsigned short _Accum'}}
+ printf("%K", ula); // expected-warning{{format specifies type 'unsigned _Accum' but the argument has type 'unsigned long _Accum'}}
+
+ /// Test signs.
+ printf("%r", uf); // expected-warning{{format specifies type '_Fract' but the argument has type 'unsigned _Fract'}}
+ printf("%R", f); // expected-warning{{format specifies type 'unsigned _Fract' but the argument has type '_Fract'}}
+ printf("%k", ua); // expected-warning{{format specifies type '_Accum' but the argument has type 'unsigned _Accum'}}
+ printf("%K", a); // expected-warning{{format specifies type 'unsigned _Accum' but the argument has type '_Accum'}}
+
+ /// Test between types.
+ printf("%r", a); // expected-warning{{format specifies type '_Fract' but the argument has type '_Accum'}}
+ printf("%R", ua); // expected-warning{{format specifies type 'unsigned _Fract' but the argument has type 'unsigned _Accum'}}
+ printf("%k", f); // expected-warning{{format specifies type '_Accum' but the argument has type '_Fract'}}
+ printf("%K", uf); // expected-warning{{format specifies type 'unsigned _Accum' but the argument has type 'unsigned _Fract'}}
+
+ /// Test saturated types.
+ printf("%r", sat_f);
+ printf("%R", sat_uf);
+ printf("%k", sat_a);
+ printf("%K", sat_ua);
+}
+
+void test_length_modifiers_and_flags(void) {
+ printf("%hr", sf);
+ printf("%lr", lf);
+ printf("%hR", usf);
+ printf("%lR", ulf);
+ printf("%hk", sa);
+ printf("%lk", la);
+ printf("%hK", usa);
+ printf("%lK", ula);
+
+ printf("%hr", sat_sf);
+ printf("%lr", sat_lf);
+ printf("%hR", sat_usf);
+ printf("%lR", sat_ulf);
+ printf("%hk", sat_sa);
+ printf("%lk", sat_la);
+ printf("%hK", sat_usa);
+ printf("%lK", sat_ula);
+
+ printf("%10r", f);
+ printf("%10.10r", f);
+ printf("%010r", f);
+ printf("%-10r", f);
+ printf("%.10r", f);
+ printf("%+r", f);
+ printf("% r", f);
+ printf("%#r", f);
+ printf("%#.r", f);
+ printf("%#.0r", f);
+
+ /// Test some invalid length modifiers.
+ printf("%zr", f); // expected-warning{{length modifier 'z' results in undefined behavior or no effect with 'r' conversion specifier}}
+ printf("%llr", f); // expected-warning{{length modifier 'll' results in undefined behavior or no effect with 'r' conversion specifier}}
+ printf("%hhr", f); // expected-warning{{length modifier 'hh' results in undefined behavior or no effect with 'r' conversion specifier}}
+}
+#else
+void test_fixed_point_specifiers_no_printf() {
+ printf("%k", i); // expected-warning{{invalid conversion specifier 'k'}}
+ printf("%K", i); // expected-warning{{invalid conversion specifier 'K'}}
+ printf("%r", i); // expected-warning{{invalid conversion specifier 'r'}}
+ printf("%R", i); // expected-warning{{invalid conversion specifier 'R'}}
+}
+#endif // WITHOUT_FIXED_POINT
|
|
Do you know a libc implementation that actually implements |
LLVM-libc will support them soon, PR should be coming next week. We have internal customers at Google that are starting to use fixed-point types in their embedded products. |
MaskRay
approved these changes
Feb 24, 2024
petrhosek
approved these changes
Feb 25, 2024
petrhosek
reviewed
Feb 25, 2024
Comment on lines
+2984
to
+2985
| // Per ISO N1169, this method accepts fixed point types and returns the | ||
| // corresponding non-saturated type for a given fixed point type. |
There was a problem hiding this comment.
Suggested change
| // Per ISO N1169, this method accepts fixed point types and returns the | |
| // corresponding non-saturated type for a given fixed point type. | |
| // Per ISO/IEC TR 18037:2008, this method accepts fixed point types and | |
| // returns the corresponding non-saturated type for a given fixed point type. |
#82707 is the libc PR |
|
This patch looks fine from my end, but I was wondering: Does clang do warnings for printf flags that don't apply to a specific conversion? As an example, in the format specifier |
Oh, that's a good point. Since |
PiJoules
force-pushed
the
fixed-point-format-specifiers
branch
from
6bf764e to
42d128e
Compare
ISO/IEC TR 18037 defines %r, %R, %k, and %K for fixed point format specifiers. -Wformat should not warn on these when they are provided.
PiJoules
force-pushed
the
fixed-point-format-specifiers
branch
from
42d128e to
466aa42
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ISO/IEC TR 18037 defines %r, %R, %k, and %K for fixed point format specifiers. -Wformat should not warn on these when they are provided.