[cfe-commits] [patch] Added a suggestion when a std::string is passed to printf()
Sam Panzer
panzer at google.com
Thu Jun 14 15:09:19 PDT 2012
Attached is my latest attempt.
On Wed, Jun 13, 2012 at 6:46 PM, Richard Smith <richard at metafoo.co.uk>wrote:
> On Wed, Jun 13, 2012 at 5:57 PM, Sam Panzer <panzer at google.com> wrote:
> > Thanks for taking the time to review! Here is another update that
> addresses
> > Richard's suggestions.
> >
> >> 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.
>
> The comments around there need updating.
>
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.
>
> I think you missed this?
>
I did. For some reason Gmail didn't format it differently...
>
> >> @@ -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.
>
> You don't need to indent 'case' labels within a switch. You can also
> dedent the PDiag line to get it within 80 columns.
>
The indenting should be better now.
>
> >> - 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.
>
> I would imagine you still won't get a diagnostic about the non-pod
> argument in a case like this:
>
> extern const char str[30];
> printf(str, my_std_string);
>
> I think you need essentially the same set of checks which are
> performed by SemaCheckStringLiteral in order to get this exactly
> right. Perhaps the easiest approach would be to defer all POD checking
> of vararg types to CheckFunctionArguments (after the call to
> SemaCheckStringLiteral) if the callee has a FormatAttr.
>
I factored out the checks from DefaultVariadicArgumentPromotion so that
they are reused right after SemaCheckStringLiteral in CheckFormatArguments
as Richeard suggested. Now any format string arguments that the old warning
would have caught but the format string matching misses should now be
caught, and there's a few extra test to catch this sort of thing.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120614/e5bf6804/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: printf-diag.patch
Type: application/octet-stream
Size: 28862 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120614/e5bf6804/attachment.obj>
More information about the cfe-commits
mailing list