diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 3dc03fa..a812bb1 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -4681,6 +4681,11 @@ def err_cannot_pass_objc_interface_to_vararg : Error< "cannot pass object with interface type %0 by-value through variadic " "%select{function|block|method}1">; +def warn_non_pod_vararg_with_format_string : Warning< + "cannot pass non-POD object of type %0 to variadic function; " + "expected type was %1">, + InGroup>, DefaultError; + def warn_cannot_pass_non_pod_arg_to_vararg : Warning< "cannot pass object of %select{non-POD|non-trivial}0 type %1 through variadic" " %select{function|block|method|constructor}2; call will abort at runtime">, @@ -5161,7 +5166,8 @@ def warn_scanf_scanlist_incomplete : Warning< "no closing ']' for '%%[' in scanf format string">, InGroup; def note_format_string_defined : Note<"format string is defined here">; - +def note_printf_c_str: Note<"did you mean to call the %0 method?">; + def warn_null_arg : Warning< "null passed to a callee which requires a non-null argument">, InGroup; @@ -5617,4 +5623,3 @@ def err_module_private_definition : Error< } } // end of sema component. - diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index f0d3213..84597cb 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -6395,6 +6395,17 @@ public: VariadicDoesNotApply }; + // Used for determining in which context a type is considered POD + enum PODType { + POD, + CXX11_POD, + ObjC_Lifetime_POD, + NON_POD + }; + + // Determines which PODType fits an expression. + PODType levelOfTriviality(const Expr *E); + /// GatherArgumentsForCall - Collector argument expressions for various /// form of call prototypes. bool GatherArgumentsForCall(SourceLocation CallLoc, @@ -6409,7 +6420,8 @@ public: // DefaultVariadicArgumentPromotion - Like DefaultArgumentPromotion, but // will warn if the resulting type is not a POD type. ExprResult DefaultVariadicArgumentPromotion(Expr *E, VariadicCallType CT, - FunctionDecl *FDecl); + FunctionDecl *FDecl, + bool HasformatString = false); // UsualArithmeticConversions - performs the UsualUnaryConversions on it's // operands and then handles various conversions that are common to binary @@ -6985,6 +6997,8 @@ private: const ArraySubscriptExpr *ASE=0, bool AllowOnePastEnd=true, bool IndexNegated=false); void CheckArrayAccess(const Expr *E); + bool hasFormatStringArgument(FunctionDecl *FDecl, Expr **Args, + unsigned NumArgs); bool CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall); bool CheckObjCMethodCall(ObjCMethodDecl *Method, SourceLocation loc, Expr **Args, unsigned NumArgs); diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp index e818f5f..384b249 100644 --- a/lib/Sema/SemaChecking.cpp +++ b/lib/Sema/SemaChecking.cpp @@ -16,6 +16,7 @@ #include "clang/Sema/Sema.h" #include "clang/Sema/SemaInternal.h" #include "clang/Sema/Initialization.h" +#include "clang/Sema/Lookup.h" #include "clang/Sema/ScopeInfo.h" #include "clang/Analysis/Analyses/FormatString.h" #include "clang/AST/ASTContext.h" @@ -418,6 +419,33 @@ bool Sema::CheckARMBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) { return false; } +// Given a variadic function, decides whether it looks like we have a +// printf-style format string and arguments) +bool Sema::hasFormatStringArgument(FunctionDecl *FDecl, + Expr **Args, unsigned NumArgs) { + // Anonymous and lambda functions will be assumed not to be printf. + if (!FDecl) + return false; + + for (specific_attr_iterator + I = FDecl->specific_attr_begin(), + E = FDecl->specific_attr_end(); I != E ; ++I) { + unsigned format_idx = I->getFormatIdx() - 1; + + // We need to adjust the index of the format parameter because + // the `this` parameter is not counted in the argument list + if (isa(FDecl)) { + if(format_idx > 0) + return false; + format_idx--; + } + // If the index of the format parameter makes sense, we have found a match. + if (format_idx < NumArgs) + return true; + } + return false; +} + /// CheckFunctionCall - Check a direct function call for various correctness /// and safety properties not strictly enforced by the C type system. bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall) { @@ -2138,7 +2166,11 @@ public: bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS, const char *startSpecifier, unsigned specifierLen); - + bool checkFormatExpr(const analyze_printf::PrintfSpecifier &FS, + const char *StartSpecifier, + unsigned SpecifierLen, + const Expr *E); + bool HandleAmount(const analyze_format_string::OptionalAmount &Amt, unsigned k, const char *startSpecifier, unsigned specifierLen); void HandleInvalidAmount(const analyze_printf::PrintfSpecifier &FS, @@ -2152,6 +2184,9 @@ public: const analyze_printf::OptionalFlag &ignoredFlag, const analyze_printf::OptionalFlag &flag, const char *startSpecifier, unsigned specifierLen); + bool checkForCStrMembers(const analyze_printf::ArgTypeResult &ATR, + const Expr *E, const CharSourceRange &CSR); + }; } @@ -2269,6 +2304,66 @@ void CheckPrintfHandler::HandleIgnoredFlag( getSpecifierRange(ignoredFlag.getPosition(), 1))); } +// Determines if the specified is a C++ class or struct containing +// a member with the specified name and kind (e.g. a CXXMethodDecl named +// "c_str()" +template +static llvm::SmallPtrSet +CXXRecordMembersNamed(StringRef Name, Sema &S, QualType Ty) { + const RecordType *RT = Ty->getAs(); + llvm::SmallPtrSet Results; + + if (!RT) + return Results; + const CXXRecordDecl *RD = dyn_cast(RT->getDecl()); + if (!RD) + return Results; + + LookupResult R(S, &S.PP.getIdentifierTable().get(Name), SourceLocation(), + Sema::LookupMemberName); + + // We just need to include all members of the right kind turned up by the + // filter, at this point. + if (S.LookupQualifiedName(R, RT->getDecl())) { + LookupResult::Filter filter = R.makeFilter(); + while (filter.hasNext()) { + NamedDecl *decl = filter.next()->getUnderlyingDecl(); + if (FieldKind *FK = dyn_cast(decl)) { + Results.insert(FK); + } + } + filter.done(); + } + return Results; +} + +// Check if a (w)string was passed when a (w)char* was needed, and offer a +// better diagnostic if so. ATR is assumed to be valid. +// Returns true when a c_str() conversion method is found. +bool CheckPrintfHandler::checkForCStrMembers( + const analyze_printf::ArgTypeResult &ATR, const Expr *E, + const CharSourceRange &CSR) { + typedef llvm::SmallPtrSet MethodSet; + + MethodSet Results = + CXXRecordMembersNamed("c_str", S, E->getType()); + + for (MethodSet::iterator MI = Results.begin(), ME = Results.end(); + MI != ME; ++MI) { + const CXXMethodDecl *Method = *MI; + if (Method->getNumParams() == 0 && + ATR.matchesType(S.Context, Method->getResultType())) { + + S.Diag(E->getLocStart(), diag::note_printf_c_str) + << "c_str()" + << FixItHint::CreateInsertion(E->getLocEnd(), ".c_str()"); + return true; + } + } + + return false; +} + bool CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS, @@ -2396,20 +2491,30 @@ CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier if (!CheckNumArgs(FS, CS, startSpecifier, specifierLen, argIndex)) return false; + return checkFormatExpr(FS, startSpecifier, specifierLen, + getDataArg(argIndex)); +} + +bool +CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS, + const char *StartSpecifier, + unsigned SpecifierLen, + const Expr *E) { + using namespace analyze_format_string; + using namespace analyze_printf; // Now type check the data expression that matches the // format specifier. - const Expr *Ex = getDataArg(argIndex); const analyze_printf::ArgTypeResult &ATR = FS.getArgType(S.Context, ObjCContext); - if (ATR.isValid() && !ATR.matchesType(S.Context, Ex->getType())) { + if (ATR.isValid() && !ATR.matchesType(S.Context, E->getType())) { // Look through argument promotions for our error message's reported type. // This includes the integral and floating promotions, but excludes array // and function pointer decay; seeing that an argument intended to be a // string has type 'char [6]' is probably more confusing than 'char *'. - if (const ImplicitCastExpr *ICE = dyn_cast(Ex)) { + if (const ImplicitCastExpr *ICE = dyn_cast(E)) { if (ICE->getCastKind() == CK_IntegralCast || ICE->getCastKind() == CK_FloatingCast) { - Ex = ICE->getSubExpr(); + E = ICE->getSubExpr(); // Check if we didn't match because of an implicit cast from a 'char' // or 'short' to an 'int'. This is done because printf is a varargs @@ -2417,7 +2522,7 @@ CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier if (ICE->getType() == S.Context.IntTy || ICE->getType() == S.Context.UnsignedIntTy) { // All further checking is done on the subexpression. - if (ATR.matchesType(S.Context, Ex->getType())) + if (ATR.matchesType(S.Context, E->getType())) return true; } } @@ -2425,7 +2530,7 @@ CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier // We may be able to offer a FixItHint if it is a supported type. PrintfSpecifier fixedFS = FS; - bool success = fixedFS.fixType(Ex->getType(), S.getLangOpts(), + bool success = fixedFS.fixType(E->getType(), S.getLangOpts(), S.Context, ObjCContext); if (success) { @@ -2436,24 +2541,40 @@ CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier EmitFormatDiagnostic( S.PDiag(diag::warn_printf_conversion_argument_type_mismatch) - << ATR.getRepresentativeTypeName(S.Context) << Ex->getType() - << Ex->getSourceRange(), - Ex->getLocStart(), + << ATR.getRepresentativeTypeName(S.Context) << E->getType() + << E->getSourceRange(), + E->getLocStart(), /*IsStringLocation*/false, - getSpecifierRange(startSpecifier, specifierLen), + getSpecifierRange(StartSpecifier, SpecifierLen), FixItHint::CreateReplacement( - getSpecifierRange(startSpecifier, specifierLen), + getSpecifierRange(StartSpecifier, SpecifierLen), os.str())); } else { - EmitFormatDiagnostic( - S.PDiag(diag::warn_printf_conversion_argument_type_mismatch) - << ATR.getRepresentativeTypeName(S.Context) << Ex->getType() - << getSpecifierRange(startSpecifier, specifierLen) - << Ex->getSourceRange(), - Ex->getLocStart(), - /*IsStringLocation*/false, - getSpecifierRange(startSpecifier, specifierLen)); + const CharSourceRange &CSR = getSpecifierRange(StartSpecifier, + SpecifierLen); + // Since the warning for passing non-POD types to variadic functions + // was deferred until now, we emit a warning for non-POD + // arguments here. + if (S.levelOfTriviality(E) == Sema::NON_POD) { + EmitFormatDiagnostic( + S.PDiag(diag::warn_non_pod_vararg_with_format_string) + << E->getType() + << ATR.getRepresentativeTypeName(S.Context) + << CSR + << E->getSourceRange(), + E->getLocStart(), /*IsStringLocation*/false, CSR); + + checkForCStrMembers(ATR, E, CSR); + } + else { + EmitFormatDiagnostic( + S.PDiag(diag::warn_printf_conversion_argument_type_mismatch) + << ATR.getRepresentativeTypeName(S.Context) << E->getType() + << CSR + << E->getSourceRange(), + E->getLocStart(), /*IsStringLocation*/false, CSR); + } } } diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 9f0378a..5821105 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -524,11 +524,38 @@ ExprResult Sema::DefaultArgumentPromotion(Expr *E) { return Owned(E); } +// Determine the degree of POD-ness for an expression. +// Incomplete types are considered POD +Sema::PODType Sema::levelOfTriviality(const Expr *E) { + if (E->getType()->isIncompleteType() || E->getType().isCXX98PODType(Context)) + return POD; + // C++0x [expr.call]p7: + // Passing a potentially-evaluated argument of class type (Clause 9) + // having a non-trivial copy constructor, a non-trivial move constructor, + // or a non-trivial destructor, with no corresponding parameter, + // is conditionally-supported with implementation-defined semantics. + + if (getLangOpts().CPlusPlus0x && !E->getType()->isDependentType()) { + if (CXXRecordDecl *Record = E->getType()->getAsCXXRecordDecl()) { + if (Record->hasTrivialCopyConstructor() && + Record->hasTrivialMoveConstructor() && + Record->hasTrivialDestructor()) { + return CXX11_POD; + } + } + } + + if (getLangOpts().ObjCAutoRefCount && E->getType()->isObjCLifetimeType()) + return ObjC_Lifetime_POD; + return NON_POD; +} + /// DefaultVariadicArgumentPromotion - Like DefaultArgumentPromotion, but /// will warn if the resulting type is not a POD type, and rejects ObjC /// interfaces passed by value. ExprResult Sema::DefaultVariadicArgumentPromotion(Expr *E, VariadicCallType CT, - FunctionDecl *FDecl) { + FunctionDecl *FDecl, + bool HasFormatString) { if (const BuiltinType *PlaceholderTy = E->getType()->getAsPlaceholderType()) { // Strip the unbridged-cast placeholder expression off, if applicable. if (PlaceholderTy->getKind() == BuiltinType::ARCUnbridgedCast && @@ -557,41 +584,29 @@ ExprResult Sema::DefaultVariadicArgumentPromotion(Expr *E, VariadicCallType CT, << E->getType() << CT)) return ExprError(); + PODType PT = levelOfTriviality(E); + // Complain about passing non-POD types through varargs. However, don't // perform this check for incomplete types, which we can get here when we're // in an unevaluated context. - if (!E->getType()->isIncompleteType() && - !E->getType().isCXX98PODType(Context)) { - // C++0x [expr.call]p7: - // Passing a potentially-evaluated argument of class type (Clause 9) - // having a non-trivial copy constructor, a non-trivial move constructor, - // or a non-trivial destructor, with no corresponding parameter, - // is conditionally-supported with implementation-defined semantics. - bool TrivialEnough = false; - if (getLangOpts().CPlusPlus0x && !E->getType()->isDependentType()) { - if (CXXRecordDecl *Record = E->getType()->getAsCXXRecordDecl()) { - if (Record->hasTrivialCopyConstructor() && - Record->hasTrivialMoveConstructor() && - Record->hasTrivialDestructor()) { - DiagRuntimeBehavior(E->getLocStart(), 0, - PDiag(diag::warn_cxx98_compat_pass_non_pod_arg_to_vararg) - << E->getType() << CT); - TrivialEnough = true; - } - } - } - - if (!TrivialEnough && - getLangOpts().ObjCAutoRefCount && - E->getType()->isObjCLifetimeType()) - TrivialEnough = true; - - if (TrivialEnough) { - // Nothing to diagnose. This is okay. + switch (PT) { + case POD: // Perfectly fine. + case ObjC_Lifetime_POD: // Nothing to diagnose. This is okay. + break; + case CXX11_POD: + DiagRuntimeBehavior(E->getLocStart(), 0, + PDiag(diag::warn_cxx98_compat_pass_non_pod_arg_to_vararg) + << E->getType() << CT); + break; + case NON_POD: + if (HasFormatString) { + // If there is a format string argument, non-POD argument checking is + // handled along with format string checking. + break; } else if (DiagRuntimeBehavior(E->getLocStart(), 0, - PDiag(diag::warn_cannot_pass_non_pod_arg_to_vararg) - << getLangOpts().CPlusPlus0x << E->getType() - << CT)) { + PDiag(diag::warn_cannot_pass_non_pod_arg_to_vararg) + << getLangOpts().CPlusPlus0x << E->getType() + << CT)) { // Turn this into a trap. CXXScopeSpec SS; SourceLocation TemplateKWLoc; @@ -3532,6 +3547,9 @@ bool Sema::GatherArgumentsForCall(SourceLocation CallLoc, // If this is a variadic call, handle args passed through "...". if (CallType != VariadicDoesNotApply) { + // Decide if we're probably inspecting a printf-like function + + bool HasFormatString = hasFormatStringArgument(FDecl, Args, NumArgs); // Assume that extern "C" functions with variadic arguments that // return __unknown_anytype aren't *really* variadic. @@ -3542,7 +3560,8 @@ bool Sema::GatherArgumentsForCall(SourceLocation CallLoc, if (isa(Args[i]->IgnoreParens())) arg = DefaultFunctionArrayLvalueConversion(Args[i]); else - arg = DefaultVariadicArgumentPromotion(Args[i], CallType, FDecl); + arg = DefaultVariadicArgumentPromotion(Args[i], CallType, FDecl, + HasFormatString); Invalid |= arg.isInvalid(); AllArgs.push_back(arg.take()); } @@ -3551,7 +3570,8 @@ bool Sema::GatherArgumentsForCall(SourceLocation CallLoc, } else { for (unsigned i = ArgIx; i != NumArgs; ++i) { ExprResult Arg = DefaultVariadicArgumentPromotion(Args[i], CallType, - FDecl); + FDecl, + HasFormatString); Invalid |= Arg.isInvalid(); AllArgs.push_back(Arg.take()); } diff --git a/test/SemaCXX/printf-cstr.cpp b/test/SemaCXX/printf-cstr.cpp new file mode 100644 index 0000000..68d999d --- /dev/null +++ b/test/SemaCXX/printf-cstr.cpp @@ -0,0 +1,30 @@ +// RUN: %clang_cc1 -Wall -fsyntax-only -Wformat -verify %s + +#include + +extern "C" { +extern int printf(const char *restrict, ...); +} + +class HasCStr { + const char *str; + public: + HasCStr(const char *s): str(s) { } + const char *c_str() {return str;} +}; + +class HasNoCStr { + const char *str; + public: + HasNoCStr(const char *s): str(s) { } + const char *not_c_str() {return str;} +}; + +void f() { + char str[] = "test"; + HasCStr hcs(str); + HasNoCStr hncs(str); + printf("%d: %s\n", 10, hcs.c_str()); + printf("%d: %s\n", 10, hcs); // expected-error{{cannot pass non-POD object of type 'HasCStr' to variadic function; expected type was 'char *'}} expected-note{{did you mean to call the c_str() method?}} + printf("%d: %s\n", 10, hncs); // expected-error{{cannot pass non-POD object of type 'HasNoCStr' to variadic function; expected type was 'char *'}} +}