r231211 - Add a format warning for "%p" with non-void* args
Daniel Jasper
djasper at google.com
Wed Mar 4 08:44:08 PST 2015
Thanks
On Wed, Mar 4, 2015 at 5:33 PM, Seth Cantrell <seth.cantrell at gmail.com>
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 <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>
> 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
>> cfe-commits at cs.uiuc.edu
>> 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/04dee701/attachment.html>
More information about the cfe-commits
mailing list