r231211 - Add a format warning for "%p" with non-void* args
Daniel Jasper
djasper at google.com
Wed Mar 4 23:28:27 PST 2015
Thanks for double-checking!
On Thu, Mar 5, 2015 at 2:09 AM, Seth Cantrell <seth.cantrell at gmail.com>
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 <djasper at google.com> wrote:
>
> 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/20150305/bb515c73/attachment.html>
More information about the cfe-commits
mailing list