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