r231211 - Add a format warning for "%p" with non-void* args
Seth Cantrell
seth.cantrell at gmail.com
Wed Mar 4 08:33:24 PST 2015
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 <djasper at google.com> 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 <seth.cantrell at gmail.com <mailto:seth.cantrell at gmail.com>> 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
> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits <http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150304/b41ce4f9/attachment.html>
More information about the cfe-commits
mailing list