[cfe-commits] [patch] Added a suggestion when a std::string is passed to printf()
Sam Panzer
panzer at google.com
Wed Jun 13 17:57:06 PDT 2012
Thanks for taking the time to review! Here is another update that addresses
Richard's suggestions.
On Wed, Jun 13, 2012 at 4:09 PM, Richard Smith <richard at metafoo.co.uk>wrote:
> On Wed, Jun 13, 2012 at 2:14 PM, Sam Panzer <panzer at google.com> wrote:
>
>> Ping.
>>
>> On Wed, Jun 6, 2012 at 6:35 PM, Sam Panzer <panzer at google.com> wrote:
>>
>>> Thanks again for the input.
>>>
>>> Of the two main issues,
>>> (1) the POD_Type enum
>>> I used the enum to distinguish between three cases: one requires a
>>> warning for C++98 compatibility, one requires the standard
>>> can't-pass-non-pod-to-vararg warning, and the third does nothing. Since the
>>> code that checks for non-POD arguments to vararg functions lives in two
>>> places because of the way format-string checking is implemented, I thought
>>> it made sense to separate the pod-kind-determining logic from the
>>> warning-emitting logic.
>>>
>>> Does someone have a suggestion for implementing this? I can think of two
>>> other approaches, and neither seems as clean: levelOfTriviality could
>>> either take an extra argument that tells it not to emit warnings, or it
>>> could take a series of callbacks for the three cases.
>>>
>>> (2) It is possible for SemaChecking to reuse the logic from
>>> hasFormatStringArgument(), since both of them really compute the (expected)
>>> location of the format string. I refactored a bit to share the relevant
>>> logic, though there isn't too much in common. Is this an improvement?
>>>
>>> On Wed, Jun 6, 2012 at 5:20 PM, Chandler Carruth <chandlerc at google.com>wrote:
>>>
>>>> Some minor comments, and I'll let others start chiming in here to make
>>>> sure there is general agreement about the approach:
>>>>
>>>>
>>>> +def warn_non_pod_vararg_with_format_string : Warning<
>>>> + "cannot pass non-POD object of type %0 to variadic function; "
>>>> + "expected type was %1">,
>>>>
>>>> I'd like to mention that the expectation stems from a format string
>>>> here:
>>>>
>>>> '... variadic function; the format string expects it to have type %1'
>>>>
>>>> or some such...
>>>>
>>>> + // Used for determining in which context a type is considered POD
>>>> + enum PODType {
>>>> + POD,
>>>> + CXX11_POD,
>>>> + ObjC_Lifetime_POD,
>>>> + NON_POD
>>>> + };
>>>>
>>>> The LLVM coding conventions on enums are much more precise than other
>>>> things. I would vaguely expect it to look like:
>>>>
>>>> enum PODKind {
>>>> PK_NonPOD,
>>>> PK_POD,
>>>> PK_CXX11POD,
>>>> PK_ObjCLifetimePOD
>>>> };
>>>>
>>>> +
>>>> + // Determines which PODType fits an expression.
>>>> + PODType levelOfTriviality(const Expr *E);
>>>> +
>>>>
>>>> Actually, looking at how this function is implemented, I think you're
>>>> going about it the wrong way. This isn't a generalizable concept, it is a
>>>> very specific and precise query: isValidTypeForVarargArgument or some such.
>>>> It should return true or false, no enum required.
>>>>
>>>
>>>> + bool HasformatString =
>>>> false);
>>>>
>>>> s/HasformatString/HasFormatString/
>>>>
>>>> +// 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) {
>>>>
>>>> I don't see you using this function from anywhere but new code -- is
>>>> there a reason that SemaChecking can't use this logic?
>>>>
>>>> + // Decide if we're probably inspecting a printf-like function
>>>> +
>>>> + bool HasFormatString = hasFormatStringArgument(FDecl, Args,
>>>> NumArgs);
>>>>
>>>> I think the comment is now extraneous.
>>>>
>>>>
>>>> On Wed, Jun 6, 2012 at 4:56 PM, Sam Panzer <panzer at google.com> wrote:
>>>>
>>>>> Parens removed, though operator-> is unavailable here. For future
>>>>> reference, is Clang averse to extra variable declarations in favor of extra
>>>>> method calls?
>>>>
>>>>
>>>>
>>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
> diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
> index 3dc03fa..c51302c 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; "
>
> In C++11, this should be 'non-trivial' not 'non-POD'.
>
> I added an extra %select to print "non-trivial" for C++11.
> + "expected type from format string was %1">,
> + InGroup<DiagGroup<"non-pod-varargs">>, 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,6 +5166,7 @@ def warn_scanf_scanlist_incomplete : Warning<
> "no closing ']' for '%%[' in scanf format string">,
> InGroup<Format>;
> 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">,
> @@ -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..c1d2933 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 PODKind {
> + PK_POD,
> + PK_CXX11_POD,
> + PK_ObjC_Lifetime_POD,
> + PK_NON_POD
> + };
> +
> + // Determines which PODKind fits an expression.
> + PODKind levelOfTriviality(const Expr *E);
>
> This seems to be specific to vararg arguments, so I'd prefer for it to be phrased in those terms. Possibly VarArgKind with values VAK_Invalid, VAK_Valid, and VAK_ValidInCXX11 (for the C++98 compat warning), and isValidVarArgType?
>
> I would also suggest changing the argument type from const Expr * to QualType, since only the type is relevant.
>
>
Good points! Done.
> +
> /// 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,19 @@ private:
> const ArraySubscriptExpr *ASE=0,
> bool AllowOnePastEnd=true, bool IndexNegated=false);
> void CheckArrayAccess(const Expr *E);
> + bool hasFormatStringArgument(FunctionDecl *FDecl, Expr **Args,
> + unsigned NumArgs);
> + // Used to grab the relevant information from a FormatAttr and a
> + // FunctionDeclaration.
> + struct FormatStringInfo {
> + unsigned format_idx;
> + unsigned firstDataArg;
> + bool hasVAListArg;
>
> Even though these are just being factored out from elsewhere, please fix the names to conform to the coding standard: FormatIdx, FirstDataArg, HasVAListArg.
>
> + };
> +
> + bool getFormatStringInfo(const FormatAttr *Format, Expr **Args,
> + unsigned NumArgs, bool IsCXXMember,
> + FormatStringInfo *FSI);
> 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..b075331 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,49 @@ 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)
>
> Please add a '.', and use '///' so doxygen picks this comment up.
>
> Done.
> +bool Sema::hasFormatStringArgument(FunctionDecl *FDecl, Expr **Args,
> + unsigned NumArgs) {
> + // Anonymous and lambda functions will be assumed not to be format strings
>
> The !FDecl case will mainly be calls through function pointers. For a lambda, we should actually have an FDecl. Indeed, g++ supports format attributes in such a case:
>
> Ah - I must have misunderstood the original comment. I modified this one
to say that functions without FDecls will be assumed not to be printf-like.
> auto f = [](const char *p, ...) __attribute__((format(printf, 2, 3))) { return 0; };
>
> int k = f("%d", 10.2); // warning in gcc
>
> + if (!FDecl)
> + return false;
> +
> + FormatStringInfo FSI;
> + for (specific_attr_iterator<FormatAttr>
> + I = FDecl->specific_attr_begin<FormatAttr>(),
> + E = FDecl->specific_attr_end<FormatAttr>(); I != E ; ++I) {
> + if (getFormatStringInfo(*I, Args, NumArgs, isa<CXXMethodDecl>(FDecl), &FSI))
> + return true;
> + }
> + return false;
> +}
> +
> +// If so, fills in the FormatStringInfo parameter to have the correct format_idx
> +// and firstDataArg.
>
> Please reword the 'If so' so this comment stands on its own, and explain what the return value means.
>
> Done.
> +bool Sema::getFormatStringInfo(const FormatAttr *Format, Expr **Args,
> + unsigned NumArgs, bool IsCXXMember,
> + FormatStringInfo *FSI) {
> + // The way the format attribute works in GCC, the implicit this argument
> + // of member functions is counted. However, it doesn't appear in our own
> + // lists, so decrement format_idx in that case.
>
> This comment should presumably be attached to the 'if (IsCXXMember)' below.
>
> Done.
> + FSI->hasVAListArg = Format->getFirstArg() == 0;
> + FSI->format_idx = Format->getFormatIdx() - 1;
> + FSI->firstDataArg = FSI->hasVAListArg ? 0 : Format->getFirstArg() - 1;
> +
> + // We need to adjust the index of the format parameter because
> + // the `this` parameter is not counted in the argument list
> + if (IsCXXMember) {
> + if(FSI->format_idx == 0)
> + return false;
> + --FSI->format_idx;
> + if (FSI->firstDataArg != 0)
> + --FSI->firstDataArg;
> + }
> + return true;
> +}
> +
> /// 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) {
> @@ -1702,9 +1746,6 @@ Sema::FormatStringType Sema::GetFormatStringType(const FormatAttr *Format) {
> /// functions) for correct use of format strings.
> void Sema::CheckFormatArguments(const FormatAttr *Format, CallExpr *TheCall) {
> bool IsCXXMember = false;
> - // The way the format attribute works in GCC, the implicit this argument
> - // of member functions is counted. However, it doesn't appear in our own
> - // lists, so decrement format_idx in that case.
> IsCXXMember = isa<CXXMemberCallExpr>(TheCall);
> CheckFormatArguments(Format, TheCall->getArgs(), TheCall->getNumArgs(),
> IsCXXMember, TheCall->getRParenLoc(),
> @@ -1714,18 +1755,12 @@ void Sema::CheckFormatArguments(const FormatAttr *Format, CallExpr *TheCall) {
> void Sema::CheckFormatArguments(const FormatAttr *Format, Expr **Args,
> unsigned NumArgs, bool IsCXXMember,
> SourceLocation Loc, SourceRange Range) {
> - bool HasVAListArg = Format->getFirstArg() == 0;
> - unsigned format_idx = Format->getFormatIdx() - 1;
> - unsigned firstDataArg = HasVAListArg ? 0 : Format->getFirstArg() - 1;
> - if (IsCXXMember) {
> - if (format_idx == 0)
> - return;
> - --format_idx;
> - if(firstDataArg != 0)
> - --firstDataArg;
> + FormatStringInfo FSI;
> + if (getFormatStringInfo(Format, Args, NumArgs, IsCXXMember, &FSI)) {
> + CheckFormatArguments(Args, NumArgs, FSI.hasVAListArg, FSI.format_idx,
> + FSI.firstDataArg, GetFormatStringType(Format), Loc,
> + Range);
> }
> - CheckFormatArguments(Args, NumArgs, HasVAListArg, format_idx,
> - firstDataArg, GetFormatStringType(Format), Loc, Range);
> }
>
> void Sema::CheckFormatArguments(Expr **Args, unsigned NumArgs,
> @@ -2138,6 +2173,10 @@ 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);
> @@ -2152,6 +2191,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 +2311,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<typename FieldKind>
>
> Clang uses 'Field' to refer to non-static data members; MemberKind would be clearer.
>
>
Done.
> +static llvm::SmallPtrSet<FieldKind*, 1>
> +CXXRecordMembersNamed(StringRef Name, Sema &S, QualType Ty) {
> + const RecordType *RT = Ty->getAs<RecordType>();
> + llvm::SmallPtrSet<FieldKind*, 1> Results;
> +
> + if (!RT)
> + return Results;
> + const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(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();
>
> You can just use LookupResult's begin/end/iterators here, rather than building a Filter, since you're not removing elements from the LookupResult.
>
>
Done.
> + while (filter.hasNext()) {
> + NamedDecl *decl = filter.next()->
> getUnderlyingDecl();
> + if (FieldKind *FK = dyn_cast<FieldKind>(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<CXXMethodDecl*, 1> MethodSet;
> +
> + MethodSet Results =
> + CXXRecordMembersNamed<CXXMethodDecl>("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 +2498,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<ImplicitCastExpr>(Ex)) {
> + if (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(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 +2529,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 +2537,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 +2548,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 {
> + 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::PK_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) << Ex->getType()
> - << getSpecifierRange(startSpecifier, specifierLen)
> - << Ex->getSourceRange(),
> - Ex->getLocStart(),
> - /*IsStringLocation*/false,
> - getSpecifierRange(startSpecifier, specifierLen));
> + << 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..3e923e2 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::PODKind Sema::levelOfTriviality(const Expr *E) {
> + if (E->getType()->isIncompleteType() || E->getType().isCXX98PODType(Context))
> + return PK_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 PK_CXX11_POD;
> + }
> + }
> + }
> +
> + if (getLangOpts().ObjCAutoRefCount && E->getType()->isObjCLifetimeType())
> + return PK_ObjC_Lifetime_POD;
> + return PK_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,37 +584,25 @@ ExprResult Sema::DefaultVariadicArgumentPromotion(Expr *E, VariadicCallType CT,
> << E->getType() << CT))
> return ExprError();
>
> + PODKind 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()) {
> + switch (PT) {
> + case PK_POD: // Perfectly fine.
> + case PK_ObjC_Lifetime_POD: // Nothing to diagnose. This is okay.
> + break;
> + case PK_CXX11_POD:
> DiagRuntimeBehavior(E->getLocStart(), 0,
> PDiag(diag::warn_cxx98_compat_pass_non_pod_arg_to_vararg)
> << E->getType() << CT);
>
> Indentation seems to have gone a bit weird here.
>
>
Fixed. Though the line-wrap gets a bit difficult for
diag::warn_cxx98_compat_pass_non_pod_arg_to_vararg.
> - TrivialEnough = true;
> - }
> - }
> - }
> -
> - if (!TrivialEnough &&
> - getLangOpts().ObjCAutoRefCount &&
> - E->getType()->isObjCLifetimeType())
> - TrivialEnough = true;
> -
> - if (TrivialEnough) {
> - // Nothing to diagnose. This is okay.
> + break;
> + case PK_NON_POD:
> + if (HasFormatString) {
> + // If there is a format string argument, non-POD argument checking is
> + // handled along with format string checking.
>
> Does this do the right thing for arguments which aren't format arguments, for instance sprintf(my_cstr1, "%d", 1); ? Do we reach the check in the -Wformat code if the format string is not a constant?
>
>
Yes, the sprintf() case is handled correctly - that's what
getFormatStringInfo() is responsible for. The -Wformat check wasn't reached
if the format string wasn't const-qualified, though. This is fixed in the
new patch.
I added test cases for these two situations to printf-cstr.cpp.
> + break;
> } else if (DiagRuntimeBehavior(E->getLocStart(), 0,
> PDiag(diag::warn_cannot_pass_non_pod_arg_to_vararg)
> << getLangOpts().CPlusPlus0x << E->getType()
> @@ -3532,6 +3547,7 @@ bool Sema::GatherArgumentsForCall(SourceLocation CallLoc,
>
> // If this is a variadic call, handle args passed through "...".
> if (CallType != VariadicDoesNotApply) {
> + bool HasFormatString = hasFormatStringArgument(FDecl, Args, NumArgs);
>
> // Assume that extern "C" functions with variadic arguments that
> // return __unknown_anytype aren't *really* variadic.
> @@ -3542,7 +3558,8 @@ bool Sema::GatherArgumentsForCall(SourceLocation CallLoc,
> if (isa<ExplicitCastExpr>(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 +3568,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..3d979a5
> --- /dev/null
> +++ b/test/SemaCXX/printf-cstr.cpp
> @@ -0,0 +1,30 @@
> +// RUN: %clang_cc1 -Wall -fsyntax-only -Wformat -verify %s
> +
> +#include <stdarg.h>
> +
> +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 from format string 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 from format string was 'char *'}}
> +}
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120613/6517b5a4/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: range-diagnostic.patch
Type: application/octet-stream
Size: 26781 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120613/6517b5a4/attachment.obj>
More information about the cfe-commits
mailing list