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