<div style="font-family: arial, helvetica, sans-serif"><font size="2">Thanks for taking the time to review! Here is another update that addresses Richard's suggestions.<br><br><div class="gmail_quote">On Wed, Jun 13, 2012 at 4:09 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><div>On Wed, Jun 13, 2012 at 2:14 PM, Sam Panzer <span dir="ltr"><<a href="mailto:panzer@google.com" target="_blank">panzer@google.com</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div>
<div style="font-family:arial,helvetica,sans-serif"><font>Ping.<br><br><div class="gmail_quote"><div>On Wed, Jun 6, 2012 at 6:35 PM, Sam Panzer <span dir="ltr"><<a href="mailto:panzer@google.com" target="_blank">panzer@google.com</a>></span> wrote:<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>Thanks again for the input.</div><div><br></div><div>Of the two main issues,</div>
<div>(1) the POD_Type enum<br>
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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>(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?</div>
</div><div><div><div class="gmail_quote"><br></div><div class="gmail_quote">On Wed, Jun 6, 2012 at 5:20 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Some minor comments, and I'll let others start chiming in here to make sure there is general agreement about the approach:<div>
<br></div><div><br></div><div><div>+def warn_non_pod_vararg_with_format_string : Warning<</div>
<div>+ "cannot pass non-POD object of type %0 to variadic function; "</div><div>+ "expected type was %1">,</div></div><div><br></div><div>I'd like to mention that the expectation stems from a format string here:</div>
<div><br></div><div>'... variadic function; the format string expects it to have type %1'</div><div><br></div><div>or some such...</div><div><br></div><div><div>+ // Used for determining in which context a type is considered POD</div>
<div>+ enum PODType {</div><div>+ POD,</div><div>+ CXX11_POD,</div><div>+ ObjC_Lifetime_POD,</div><div>+ NON_POD</div><div>+ };</div><div><br></div><div>The LLVM coding conventions on enums are much more precise than other things. I would vaguely expect it to look like:</div>
<div><br></div><div>enum PODKind {</div><div> PK_NonPOD,</div><div> PK_POD,</div><div> PK_CXX11POD,</div><div> PK_ObjCLifetimePOD</div><div>};</div><div><br></div><div>+</div><div>+ // Determines which PODType fits an expression.</div>
<div>+ PODType levelOfTriviality(const Expr *E);</div><div>+</div><div><br></div><div>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.</div>
</div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<div><br></div><div><div>+ bool HasformatString = false);</div></div><div><br></div><div>s/HasformatString/HasFormatString/</div><div><br></div><div><div><div><div>
+// Given a variadic function, decides whether it looks like we have a</div>
<div>+// printf-style format string and arguments)</div></div></div><div>+bool Sema::hasFormatStringArgument(FunctionDecl *FDecl,</div><div><div><div>+ Expr **Args, unsigned NumArgs) {</div>
</div></div></div><div><br></div>
<div>I don't see you using this function from anywhere but new code -- is there a reason that SemaChecking can't use this logic?</div><div><br></div><div><div><div><div>+ // Decide if we're probably inspecting a printf-like function</div>
</div></div><div>+</div><div>+ bool HasFormatString = hasFormatStringArgument(FDecl, Args, NumArgs);</div></div><div><br></div><div>I think the comment is now extraneous.</div><div><div><br></div><br><div class="gmail_quote">
<div>
On Wed, Jun 6, 2012 at 4:56 PM, Sam Panzer <span dir="ltr"><<a href="mailto:panzer@google.com" target="_blank">panzer@google.com</a>></span> wrote:<br>
</div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Parens removed, though operator-> is unavailable here. For future reference, is Clang averse to extra variable declarations in favor of extra method calls? </blockquote>
</div></div><br></div></div>
</blockquote></div><br>
</div></div></blockquote></div><br></font></div>
<br></div></div><div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></div></blockquote></div><br><div><br></div><div><pre style="word-wrap:break-word;white-space:pre-wrap">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; "</pre><pre style="word-wrap:break-word;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">In C++11, this should be 'non-trivial' not 'non-POD'.</font></pre>
</div></blockquote><div>I added an extra %select to print "non-trivial" for C++11.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<pre style="word-wrap:break-word;white-space:pre-wrap">+ "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);</pre><pre style="word-wrap:break-word;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">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?</font></pre>
<pre style="word-wrap:break-word;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">I would also suggest changing the argument type from const Expr * to QualType, since only the type is relevant.</font></pre>
</div></blockquote><div><br></div><div>Good points! Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<pre style="word-wrap:break-word;white-space:pre-wrap">+
/// 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;</pre><pre style="word-wrap:break-word;white-space:pre-wrap">Even though these are just being factored out from elsewhere, please fix the names to conform to the coding standard: FormatIdx, FirstDataArg, HasVAListArg.</pre>
<pre style="word-wrap:break-word;white-space:pre-wrap">+ };
+
+ 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)</pre><pre style="word-wrap:break-word;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">Please add a '.', and use '///' so doxygen picks this comment up.</font></pre>
</div></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<pre style="word-wrap:break-word;white-space:pre-wrap">+bool Sema::hasFormatStringArgument(FunctionDecl *FDecl, Expr **Args,
+ unsigned NumArgs) {
+ // Anonymous and lambda functions will be assumed not to be format strings</pre><pre style="word-wrap:break-word;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">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:</font></pre>
</div></blockquote><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<pre style="word-wrap:break-word;white-space:pre-wrap">auto f = [](const char *p, ...) __attribute__((format(printf, 2, 3))) { return 0; };</pre><pre style="word-wrap:break-word;white-space:pre-wrap">int k = f("%d", 10.2); // warning in gcc</pre>
<pre style="word-wrap:break-word;white-space:pre-wrap">+ 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.</pre><pre style="word-wrap:break-word;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">Please reword the 'If so' so this comment stands on its own, and explain what the return value means.</font></pre>
</div></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<pre style="word-wrap:break-word;white-space:pre-wrap">+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.</pre><pre style="word-wrap:break-word;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">This comment should presumably be attached to the 'if (IsCXXMember)' below.</font></pre>
</div></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<pre style="word-wrap:break-word;white-space:pre-wrap">+ 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></pre><pre style="word-wrap:break-word;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">Clang uses 'Field' to refer to non-static data members; MemberKind would be clearer.</font></pre>
</div></blockquote><div><br></div><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>
<pre style="word-wrap:break-word;white-space:pre-wrap">+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();</pre></div><pre style="word-wrap:break-word;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">You can just use LookupResult's begin/end/iterators here, rather than building a Filter, since you're not removing elements from the LookupResult.</font></pre>
</div></blockquote><div><br></div><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<pre style="word-wrap:break-word;white-space:pre-wrap"><div>+ while (filter.hasNext()) {
+ NamedDecl *decl = filter.next()-></div>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);</pre><pre style="word-wrap:break-word;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">Indentation seems to have gone a bit weird here.</font></pre></div>
</blockquote><div><br></div><div>Fixed. Though the line-wrap gets a bit difficult for diag::warn_cxx98_compat_pass_non_pod_arg_to_vararg.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><pre style="word-wrap:break-word;white-space:pre-wrap">- 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.</pre><pre style="word-wrap:break-word;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">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?</font></pre>
</div></blockquote><div><br></div><div>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.</div>
<div>I added test cases for these two situations to printf-cstr.cpp.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<pre style="word-wrap:break-word;white-space:pre-wrap">+ 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 *'}}
+}</pre></div>
</blockquote></div><br>
</font></div>