Thanks for double-checking! On Thu, Mar 5, 2015 at 2:09 AM, Seth Cantrell <[email protected]> wrote:
> 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]> > 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]> 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]> >> 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 >>> 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 >>> >>> ============================================================================== >>> --- 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 >>> >>> ============================================================================== >>> --- 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 >>> >>> ============================================================================== >>> --- 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 >>> >>> ============================================================================== >>> --- 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 >>> >>> ============================================================================== >>> --- 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 >>> >>> ============================================================================== >>> --- 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] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >> >> >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
