<div dir="ltr">There are some cases that weren't correctly put into the new FormatPedantic group, but instead reported through the normal Format group. Fixed some in r231242. Could you double-check that there aren't more incorrect classifications?</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 4, 2015 at 4:12 AM, Seth Cantrell <span dir="ltr"><<a href="mailto:seth.cantrell@gmail.com" target="_blank">seth.cantrell@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: socantre<br>
Date: Tue Mar 3 21:12:10 2015<br>
New Revision: 231211<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=231211&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=231211&view=rev</a><br>
Log:<br>
Add a format warning for "%p" with non-void* args<br>
<br>
GCC -pedantic produces a format warning when the "%p" specifier is used with<br>
arguments that are not void*. It's useful for portability to be able to<br>
catch such warnings with clang as well. The warning is off by default in<br>
both gcc and with this patch. This patch enables it either when extensions<br>
are disabled with -pedantic, or with the specific flag -Wformat-pedantic.<br>
<br>
The C99 and C11 specs do appear to require arguments corresponding to 'p'<br>
specifiers to be void*: "If any argument is not the correct type for the<br>
corresponding conversion specification, the behavior is undefined."<br>
[7.19.6.1 p9], and of the 'p' format specifier "The argument shall be a<br>
pointer to void." [7.19.6.1 p8]<br>
<br>
Both printf and scanf format checking are covered.<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Analysis/Analyses/FormatString.h<br>
cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
cfe/trunk/lib/Analysis/FormatString.cpp<br>
cfe/trunk/lib/Sema/SemaChecking.cpp<br>
cfe/trunk/test/SemaCXX/format-strings-0x.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Analysis/Analyses/FormatString.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/FormatString.h?rev=231211&r1=231210&r2=231211&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/FormatString.h?rev=231211&r1=231210&r2=231211&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Analysis/Analyses/FormatString.h (original)<br>
+++ cfe/trunk/include/clang/Analysis/Analyses/FormatString.h Tue Mar 3 21:12:10 2015<br>
@@ -231,6 +231,9 @@ class ArgType {<br>
public:<br>
enum Kind { UnknownTy, InvalidTy, SpecificTy, ObjCPointerTy, CPointerTy,<br>
AnyCharTy, CStrTy, WCStrTy, WIntTy };<br>
+<br>
+ enum MatchKind { NoMatch = 0, Match = 1, NoMatchPedantic };<br>
+<br>
private:<br>
const Kind K;<br>
QualType T;<br>
@@ -254,7 +257,7 @@ public:<br>
return Res;<br>
}<br>
<br>
- bool matchesType(ASTContext &C, QualType argTy) const;<br>
+ MatchKind matchesType(ASTContext &C, QualType argTy) const;<br>
<br>
QualType getRepresentativeType(ASTContext &C) const;<br>
<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=231211&r1=231210&r2=231211&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=231211&r1=231210&r2=231211&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Tue Mar 3 21:12:10 2015<br>
@@ -551,6 +551,7 @@ def FormatInvalidSpecifier : DiagGroup<"<br>
def FormatSecurity : DiagGroup<"format-security">;<br>
def FormatNonStandard : DiagGroup<"format-non-iso">;<br>
def FormatY2K : DiagGroup<"format-y2k">;<br>
+def FormatPedantic : DiagGroup<"format-pedantic">;<br>
def Format : DiagGroup<"format",<br>
[FormatExtraArgs, FormatZeroLength, NonNull,<br>
FormatSecurity, FormatY2K, FormatInvalidSpecifier]>,<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=231211&r1=231210&r2=231211&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=231211&r1=231210&r2=231211&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Mar 3 21:12:10 2015<br>
@@ -6644,6 +6644,10 @@ def warn_format_conversion_argument_type<br>
"format specifies type %0 but the argument has "<br>
"%select{type|underlying type}2 %1">,<br>
InGroup<Format>;<br>
+def warn_format_conversion_argument_type_mismatch_pedantic : Extension<<br>
+ "format specifies type %0 but the argument has "<br>
+ "%select{type|underlying type}2 %1">,<br>
+ InGroup<FormatPedantic>;<br>
def warn_format_argument_needs_cast : Warning<<br>
"%select{values of type|enum values with underlying type}2 '%0' should not "<br>
"be used as format arguments; add an explicit cast to %1 instead">,<br>
<br>
Modified: cfe/trunk/lib/Analysis/FormatString.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/FormatString.cpp?rev=231211&r1=231210&r2=231211&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/FormatString.cpp?rev=231211&r1=231210&r2=231211&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Analysis/FormatString.cpp (original)<br>
+++ cfe/trunk/lib/Analysis/FormatString.cpp Tue Mar 3 21:12:10 2015<br>
@@ -256,16 +256,17 @@ clang::analyze_format_string::ParseLengt<br>
// Methods on ArgType.<br>
//===----------------------------------------------------------------------===//<br>
<br>
-bool ArgType::matchesType(ASTContext &C, QualType argTy) const {<br>
+clang::analyze_format_string::ArgType::MatchKind<br>
+ArgType::matchesType(ASTContext &C, QualType argTy) const {<br>
if (Ptr) {<br>
// It has to be a pointer.<br>
const PointerType *PT = argTy->getAs<PointerType>();<br>
if (!PT)<br>
- return false;<br>
+ return NoMatch;<br>
<br>
// We cannot write through a const qualified pointer.<br>
if (PT->getPointeeType().isConstQualified())<br>
- return false;<br>
+ return NoMatch;<br>
<br>
argTy = PT->getPointeeType();<br>
}<br>
@@ -275,8 +276,8 @@ bool ArgType::matchesType(ASTContext &C,<br>
llvm_unreachable("ArgType must be valid");<br>
<br>
case UnknownTy:<br>
- return true;<br>
-<br>
+ return Match;<br>
+<br>
case AnyCharTy: {<br>
if (const EnumType *ETy = argTy->getAs<EnumType>())<br>
argTy = ETy->getDecl()->getIntegerType();<br>
@@ -289,18 +290,18 @@ bool ArgType::matchesType(ASTContext &C,<br>
case BuiltinType::SChar:<br>
case BuiltinType::UChar:<br>
case BuiltinType::Char_U:<br>
- return true;<br>
+ return Match;<br>
}<br>
- return false;<br>
+ return NoMatch;<br>
}<br>
-<br>
+<br>
case SpecificTy: {<br>
if (const EnumType *ETy = argTy->getAs<EnumType>())<br>
argTy = ETy->getDecl()->getIntegerType();<br>
argTy = C.getCanonicalType(argTy).getUnqualifiedType();<br>
<br>
if (T == argTy)<br>
- return true;<br>
+ return Match;<br>
// Check for "compatible types".<br>
if (const BuiltinType *BT = argTy->getAs<BuiltinType>())<br>
switch (BT->getKind()) {<br>
@@ -309,32 +310,33 @@ bool ArgType::matchesType(ASTContext &C,<br>
case BuiltinType::Char_S:<br>
case BuiltinType::SChar:<br>
case BuiltinType::Char_U:<br>
- case BuiltinType::UChar:<br>
- return T == C.UnsignedCharTy || T == C.SignedCharTy;<br>
+ case BuiltinType::UChar:<br>
+ return T == C.UnsignedCharTy || T == C.SignedCharTy ? Match<br>
+ : NoMatch;<br>
case BuiltinType::Short:<br>
- return T == C.UnsignedShortTy;<br>
+ return T == C.UnsignedShortTy ? Match : NoMatch;<br>
case BuiltinType::UShort:<br>
- return T == C.ShortTy;<br>
+ return T == C.ShortTy ? Match : NoMatch;<br>
case BuiltinType::Int:<br>
- return T == C.UnsignedIntTy;<br>
+ return T == C.UnsignedIntTy ? Match : NoMatch;<br>
case BuiltinType::UInt:<br>
- return T == C.IntTy;<br>
+ return T == C.IntTy ? Match : NoMatch;<br>
case BuiltinType::Long:<br>
- return T == C.UnsignedLongTy;<br>
+ return T == C.UnsignedLongTy ? Match : NoMatch;<br>
case BuiltinType::ULong:<br>
- return T == C.LongTy;<br>
+ return T == C.LongTy ? Match : NoMatch;<br>
case BuiltinType::LongLong:<br>
- return T == C.UnsignedLongLongTy;<br>
+ return T == C.UnsignedLongLongTy ? Match : NoMatch;<br>
case BuiltinType::ULongLong:<br>
- return T == C.LongLongTy;<br>
+ return T == C.LongLongTy ? Match : NoMatch;<br>
}<br>
- return false;<br>
+ return NoMatch;<br>
}<br>
<br>
case CStrTy: {<br>
const PointerType *PT = argTy->getAs<PointerType>();<br>
if (!PT)<br>
- return false;<br>
+ return NoMatch;<br>
QualType pointeeTy = PT->getPointeeType();<br>
if (const BuiltinType *BT = pointeeTy->getAs<BuiltinType>())<br>
switch (BT->getKind()) {<br>
@@ -343,50 +345,56 @@ bool ArgType::matchesType(ASTContext &C,<br>
case BuiltinType::UChar:<br>
case BuiltinType::Char_S:<br>
case BuiltinType::SChar:<br>
- return true;<br>
+ return Match;<br>
default:<br>
break;<br>
}<br>
<br>
- return false;<br>
+ return NoMatch;<br>
}<br>
<br>
case WCStrTy: {<br>
const PointerType *PT = argTy->getAs<PointerType>();<br>
if (!PT)<br>
- return false;<br>
+ return NoMatch;<br>
QualType pointeeTy =<br>
C.getCanonicalType(PT->getPointeeType()).getUnqualifiedType();<br>
- return pointeeTy == C.getWideCharType();<br>
+ return pointeeTy == C.getWideCharType() ? Match : NoMatch;<br>
}<br>
-<br>
+<br>
case WIntTy: {<br>
-<br>
+<br>
QualType PromoArg =<br>
argTy->isPromotableIntegerType()<br>
? C.getPromotedIntegerType(argTy) : argTy;<br>
-<br>
+<br>
QualType WInt = C.getCanonicalType(C.getWIntType()).getUnqualifiedType();<br>
PromoArg = C.getCanonicalType(PromoArg).getUnqualifiedType();<br>
-<br>
+<br>
// If the promoted argument is the corresponding signed type of the<br>
// wint_t type, then it should match.<br>
if (PromoArg->hasSignedIntegerRepresentation() &&<br>
C.getCorrespondingUnsignedType(PromoArg) == WInt)<br>
- return true;<br>
+ return Match;<br>
<br>
- return WInt == PromoArg;<br>
+ return WInt == PromoArg ? Match : NoMatch;<br>
}<br>
<br>
case CPointerTy:<br>
- return argTy->isPointerType() || argTy->isObjCObjectPointerType() ||<br>
- argTy->isBlockPointerType() || argTy->isNullPtrType();<br>
+ if (argTy->isVoidPointerType()) {<br>
+ return Match;<br>
+ } if (argTy->isPointerType() || argTy->isObjCObjectPointerType() ||<br>
+ argTy->isBlockPointerType() || argTy->isNullPtrType()) {<br>
+ return NoMatchPedantic;<br>
+ } else {<br>
+ return NoMatch;<br>
+ }<br>
<br>
case ObjCPointerTy: {<br>
if (argTy->getAs<ObjCObjectPointerType>() ||<br>
argTy->getAs<BlockPointerType>())<br>
- return true;<br>
-<br>
+ return Match;<br>
+<br>
// Handle implicit toll-free bridging.<br>
if (const PointerType *PT = argTy->getAs<PointerType>()) {<br>
// Things such as CFTypeRef are really just opaque pointers<br>
@@ -395,9 +403,9 @@ bool ArgType::matchesType(ASTContext &C,<br>
// structs can be toll-free bridged, we just accept them all.<br>
QualType pointee = PT->getPointeeType();<br>
if (pointee->getAsStructureType() || pointee->isVoidType())<br>
- return true;<br>
+ return Match;<br>
}<br>
- return false;<br>
+ return NoMatch;<br>
}<br>
}<br>
<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaChecking.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=231211&r1=231210&r2=231211&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=231211&r1=231210&r2=231211&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Tue Mar 3 21:12:10 2015<br>
@@ -3669,8 +3669,11 @@ CheckPrintfHandler::checkFormatExpr(cons<br>
ExprTy = TET->getUnderlyingExpr()->getType();<br>
}<br>
<br>
- if (AT.matchesType(S.Context, ExprTy))<br>
+ analyze_printf::ArgType::MatchKind match = AT.matchesType(S.Context, ExprTy);<br>
+<br>
+ if (match == analyze_printf::ArgType::Match) {<br>
return true;<br>
+ }<br>
<br>
// Look through argument promotions for our error message's reported type.<br>
// This includes the integral and floating promotions, but excludes array<br>
@@ -3848,15 +3851,18 @@ CheckPrintfHandler::checkFormatExpr(cons<br>
// arguments here.<br>
switch (S.isValidVarArgType(ExprTy)) {<br>
case Sema::VAK_Valid:<br>
- case Sema::VAK_ValidInCXX11:<br>
+ case Sema::VAK_ValidInCXX11: {<br>
+ unsigned diag = diag::warn_format_conversion_argument_type_mismatch;<br>
+ if (match == analyze_printf::ArgType::NoMatchPedantic) {<br>
+ diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;<br>
+ }<br>
+<br>
EmitFormatDiagnostic(<br>
- S.PDiag(diag::warn_format_conversion_argument_type_mismatch)<br>
- << AT.getRepresentativeTypeName(S.Context) << ExprTy << IsEnum<br>
- << CSR<br>
- << E->getSourceRange(),<br>
- E->getLocStart(), /*IsStringLocation*/false, CSR);<br>
+ S.PDiag(diag) << AT.getRepresentativeTypeName(S.Context) << ExprTy<br>
+ << IsEnum << CSR << E->getSourceRange(),<br>
+ E->getLocStart(), /*IsStringLocation*/ false, CSR);<br>
break;<br>
-<br>
+ }<br>
case Sema::VAK_Undefined:<br>
case Sema::VAK_MSVCUndefined:<br>
EmitFormatDiagnostic(<br>
@@ -3988,13 +3994,13 @@ bool CheckScanfHandler::HandleScanfSpeci<br>
FixItHint::CreateRemoval(R));<br>
}<br>
}<br>
-<br>
+<br>
if (!FS.consumesDataArgument()) {<br>
// FIXME: Technically specifying a precision or field width here<br>
// makes no sense. Worth issuing a warning at some point.<br>
return true;<br>
}<br>
-<br>
+<br>
// Consume the argument.<br>
unsigned argIndex = FS.getArgIndex();<br>
if (argIndex < NumDataArgs) {<br>
@@ -4003,7 +4009,7 @@ bool CheckScanfHandler::HandleScanfSpeci<br>
// function if we encounter some other error.<br>
CoveredArgs.set(argIndex);<br>
}<br>
-<br>
+<br>
// Check the length modifier is valid with the given conversion specifier.<br>
if (!FS.hasValidLengthModifier(S.getASTContext().getTargetInfo()))<br>
HandleInvalidLengthModifier(FS, CS, startSpecifier, specifierLen,<br>
@@ -4020,21 +4026,28 @@ bool CheckScanfHandler::HandleScanfSpeci<br>
// The remaining checks depend on the data arguments.<br>
if (HasVAListArg)<br>
return true;<br>
-<br>
+<br>
if (!CheckNumArgs(FS, CS, startSpecifier, specifierLen, argIndex))<br>
return false;<br>
-<br>
+<br>
// Check that the argument type matches the format specifier.<br>
const Expr *Ex = getDataArg(argIndex);<br>
if (!Ex)<br>
return true;<br>
<br>
const analyze_format_string::ArgType &AT = FS.getArgType(S.Context);<br>
- if (AT.isValid() && !AT.matchesType(S.Context, Ex->getType())) {<br>
+ analyze_format_string::ArgType::MatchKind match =<br>
+ AT.matchesType(S.Context, Ex->getType());<br>
+ if (AT.isValid() && match != analyze_format_string::ArgType::Match) {<br>
ScanfSpecifier fixedFS = FS;<br>
- bool success = fixedFS.fixType(Ex->getType(),<br>
- Ex->IgnoreImpCasts()->getType(),<br>
- S.getLangOpts(), S.Context);<br>
+ bool success =<br>
+ fixedFS.fixType(Ex->getType(), Ex->IgnoreImpCasts()->getType(),<br>
+ S.getLangOpts(), S.Context);<br>
+<br>
+ unsigned diag = diag::warn_format_conversion_argument_type_mismatch;<br>
+ if (match == analyze_format_string::ArgType::NoMatchPedantic) {<br>
+ diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;<br>
+ }<br>
<br>
if (success) {<br>
// Get the fix string from the fixed format specifier.<br>
@@ -4043,23 +4056,20 @@ bool CheckScanfHandler::HandleScanfSpeci<br>
fixedFS.toString(os);<br>
<br>
EmitFormatDiagnostic(<br>
- S.PDiag(diag::warn_format_conversion_argument_type_mismatch)<br>
- << AT.getRepresentativeTypeName(S.Context) << Ex->getType() << false<br>
- << Ex->getSourceRange(),<br>
- Ex->getLocStart(),<br>
- /*IsStringLocation*/false,<br>
- getSpecifierRange(startSpecifier, specifierLen),<br>
- FixItHint::CreateReplacement(<br>
+ S.PDiag(diag) << AT.getRepresentativeTypeName(S.Context)<br>
+ << Ex->getType() << false << Ex->getSourceRange(),<br>
+ Ex->getLocStart(),<br>
+ /*IsStringLocation*/ false,<br>
getSpecifierRange(startSpecifier, specifierLen),<br>
- os.str()));<br>
+ FixItHint::CreateReplacement(<br>
+ getSpecifierRange(startSpecifier, specifierLen), os.str()));<br>
} else {<br>
EmitFormatDiagnostic(<br>
- S.PDiag(diag::warn_format_conversion_argument_type_mismatch)<br>
- << AT.getRepresentativeTypeName(S.Context) << Ex->getType() << false<br>
- << Ex->getSourceRange(),<br>
- Ex->getLocStart(),<br>
- /*IsStringLocation*/false,<br>
- getSpecifierRange(startSpecifier, specifierLen));<br>
+ S.PDiag(diag) << AT.getRepresentativeTypeName(S.Context)<br>
+ << Ex->getType() << false << Ex->getSourceRange(),<br>
+ Ex->getLocStart(),<br>
+ /*IsStringLocation*/ false,<br>
+ getSpecifierRange(startSpecifier, specifierLen));<br>
}<br>
}<br>
<br>
<br>
Modified: cfe/trunk/test/SemaCXX/format-strings-0x.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/format-strings-0x.cpp?rev=231211&r1=231210&r2=231211&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/format-strings-0x.cpp?rev=231211&r1=231210&r2=231211&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/format-strings-0x.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/format-strings-0x.cpp Tue Mar 3 21:12:10 2015<br>
@@ -8,6 +8,9 @@ extern int printf(const char *restrict,<br>
void f(char **sp, float *fp) {<br>
scanf("%as", sp); // expected-warning{{format specifies type 'float *' but the argument has type 'char **'}}<br>
<br>
+ printf("%p", sp); // expected-warning{{format specifies type 'void *' but the argument has type 'char **'}}<br>
+ scanf("%p", sp); // expected-warning{{format specifies type 'void **' but the argument has type 'char **'}}<br>
+<br>
printf("%a", 1.0);<br>
scanf("%afoobar", fp);<br>
printf(nullptr);<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>