r231211 - Add a format warning for "%p" with non-void* args

Daniel Jasper djasper at google.com
Wed Mar 4 06:21:46 PST 2015


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/0c6ef3a6/attachment.html>


More information about the cfe-commits mailing list