Hi,
So I've tried to fullfill all comments. Additionally I also provide a test
for most corner-cases.
The C++ part is still not handled, but it should be much easier now.
Nuno
----- Original Message -----
From: "Ted Kremenek" <[EMAIL PROTECTED]>
To: "Nuno Lopes" <[EMAIL PROTECTED]>
Cc: "cfe-dev" <[email protected]>
Sent: Monday, March 03, 2008 5:47 PM
Subject: Re: [cfe-dev] handle __attribute__((deprecated))
On Mar 1, 2008, at 12:28 AM, Chris Lattner wrote:
Should this emit a diagnostic if it's not a function? Ted, can you
please review the rest of the 'HandleFormatAttribute' method?
Hi Nuno,
I've reviewed the version of HandleFormatAttribute that Chris
committed to the clang tree from your latest patch. Thanks again for
submitting this patch. It's a great contribution. I have a few
comments:
// FIXME: in C++ the implicit 'this' function parameter also counts.
// the index must start in 1 and the limit is numargs+1
unsigned NumArgs = Fn->getNumParams()+1; // +1 for ...
This looks incorrect for all code except non-static C++ methods, of
which we do not currently handle. The code after this frequently
checks against NumArgs, which will be incorrect (off-by-one). Also,
I'm not certain how we are actually going to handle declarations for
class members, but chances are we are not going to use FunctionDecls.
I would just remove the increment for now, and leave the FIXME. When
I first looked at this code I was also a little confused; I thought
you were trying to offset for a deficiency in the AST, rather than the
fact that this is a game that the format attribute plays in GCC (i.e.,
offsetting for the implicit "this" pointer):
"Since non-static C++ methods have an implicit this argument, the
arguments of such methods should be counted from two, not one..."
Make a comment that this is is mirroring GCC behavior when it comes to
C++ non-static methods and the format attribute. It also seems that
offsets to the base index would have to be handled as well. I would
just punt on this for now until we have Decls for C++ methods in
place. I'm a little concerned about doing this right for C++ non-
static methods; having little fudges like these to indices and ranges
is a great source for off-by-one and buffer overflow errors if not all
of the code obeys the rule. This behavior just seems like a GCC
kludge, with the ASTs being tied to closely to the implementation of
how methods are code generated (thus forcing the attribute
specification itself to be broken); in reality, the "this" pointer
would never be used as a format string, so we probably should just
pretend that it isn't there when processing the format argument
indices. This means that in the code below your original increment of
NumArgs that we should instead *decrement* the parsed indices when
handling C++ non-static methods. That way, the FormatAttr object
always stores the canonicalized indices for the *explicit* function
arguments (again, ignoring the "this" argument).
More comments:
Expr *IdxExpr = static_cast<Expr *>(rawAttr->getArg(0));
llvm::APSInt Idx(32);
if (!IdxExpr->isIntegerConstantExpr(Idx, Context)) {
Diag(rawAttr->getLoc(), diag::err_attribute_argument_n_not_int,
"format", std::string("2"), IdxExpr->getSourceRange());
return;
}
Please do not hardcode "32" or the signedness of Idx, both which could
be target specific. Please query these using
Context::getTypeSize(IdxExpr) and IdxExpr->getType()-
isUnsignedIntegerType().
For the following block, please add a big comment indicating what it
is trying to do:
Expr *IdxExpr = static_cast<Expr *>(rawAttr->getArg(0));
llvm::APSInt Idx(32);
if (!IdxExpr->isIntegerConstantExpr(Idx, Context)) {
Diag(rawAttr->getLoc(), diag::err_attribute_argument_n_not_int,
"format", std::string("2"), IdxExpr->getSourceRange());
return;
}
if (Idx.getZExtValue() < 1 || Idx.getZExtValue() > NumArgs) {
Diag(rawAttr->getLoc(), diag::err_attribute_argument_out_of_bounds,
"format", std::string("2"), IdxExpr->getSourceRange());
return;
}
Expr *FirstArgExpr = static_cast<Expr *>(rawAttr->getArg(1));
llvm::APSInt FirstArg(32);
if (!FirstArgExpr->isIntegerConstantExpr(FirstArg, Context)) {
Diag(rawAttr->getLoc(), diag::err_attribute_argument_n_not_int,
"format", std::string("3"), FirstArgExpr->getSourceRange());
return;
}
if (FormatLen == 8 && !memcmp(Format, "strftime", 8)) {
if (FirstArg.getZExtValue() != 0) {
Diag(rawAttr->getLoc(),
diag::err_format_strftime_third_parameter,
FirstArgExpr->getSourceRange());
return;
}
} else if (FirstArg.getZExtValue() > NumArgs) {
Diag(rawAttr->getLoc(), diag::err_attribute_argument_out_of_bounds,
"format", std::string("3"), FirstArgExpr->getSourceRange());
return;
}
As far as comments are concerned, it would be great to see a citation
link to the appropriate references. For example, the following link
provides GCC's documentation on format argument attributes:
http://gcc.gnu.org/onlinedocs/gcc-4.2.3/gcc/Function-Attributes.html#index-g_t_0040code_007bformat_007d-function-attribute-1880
Adding this link in a comment would be helpful so that people know
what this code is trying to implement. This mirrors documentation
through the rest of clang where we try to cite either the C standard
or the appropriate reference. Also, please add comments above
checks like "if (FirstArg.getZExtValue() != 0)" where the argument is
not suppose to be zero and *why*.
There is no harm in adding comments that document behavior; for
example, before seeing this code I did not know that the format
attribute supported the __strftime__ and __strfmon__ archetypes. I
don't think GCC always supported these archetypes, so adding some
comments saying what functionality this adds to the format attribute
would be incredibly useful to other people browsing this code (which
is one of the goals of clang).
Overall, this patch looks very good, and like your previous patches,
it's a great contribution to clang.
Ted
Index: test/Sema/format-attribute.c
===================================================================
--- test/Sema/format-attribute.c (revision 0)
+++ test/Sema/format-attribute.c (revision 0)
@@ -0,0 +1,16 @@
+//RUN: clang -fsyntax-only -verify %s
+
+#include <stdarg.h>
+
+void a(const char *a, ...) __attribute__((format(printf, 1,2))); // no-error
+void b(const char *a, ...) __attribute__((format(printf, 1,1))); //
expected-error {{'format' attribute parameter 3 is out of bounds}}
+void c(const char *a, ...) __attribute__((format(printf, 0,2))); //
expected-error {{'format' attribute parameter 2 is out of bounds}}
+void d(const char *a, int c) __attribute__((format(printf, 1,2))); //
expected-error {{format attribute requires variadic function}}
+void e(char *str, int c, ...) __attribute__((format(printf, 2,3))); //
expected-error {{format argument not a string type}}
+
+typedef const char* xpto;
+void f(xpto c, va_list list) __attribute__((format(printf, 1, 0))); // no-error
+void g(xpto c) __attribute__((format(printf, 1, 0))); // no-error
+
+void y(char *str) __attribute__((format(strftime, 1,0))); // no-error
+void z(char *str, int c, ...) __attribute__((format(strftime, 1,2))); //
expected-error {{strftime format attribute requires 3rd parameter to be 0}}
Index: include/clang/Basic/DiagnosticKinds.def
===================================================================
--- include/clang/Basic/DiagnosticKinds.def (revision 48006)
+++ include/clang/Basic/DiagnosticKinds.def (working copy)
@@ -562,6 +562,10 @@
"'%0' attribute parameter %1 is out of bounds")
DIAG(err_format_strftime_third_parameter, ERROR,
"strftime format attribute requires 3rd parameter to be 0")
+DIAG(err_format_attribute_requires_variadic, ERROR,
+ "format attribute requires variadic function")
+DIAG(err_format_attribute_not_string, ERROR,
+ "format argument not a string type")
DIAG(err_attribute_invalid_size, ERROR,
"vector size not an integral multiple of component size")
DIAG(err_attribute_zero_size, ERROR,
Index: Sema/SemaDecl.cpp
===================================================================
--- Sema/SemaDecl.cpp (revision 48006)
+++ Sema/SemaDecl.cpp (working copy)
@@ -2084,6 +2084,8 @@
d->addAttr(new NoThrowAttr());
}
+/// Handle __attribute__((format(type,idx,firstarg))) attributes
+/// based on http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
void Sema::HandleFormatAttribute(Decl *d, AttributeList *rawAttr) {
if (!rawAttr->getParameterName()) {
@@ -2105,9 +2107,16 @@
return;
}
+ const FunctionTypeProto *proto =
+ dyn_cast<FunctionTypeProto>(Fn->getType()->getAsFunctionType());
+ if (!proto)
+ return;
+
// FIXME: in C++ the implicit 'this' function parameter also counts.
+ // this is needed in order to be compatible with GCC
// the index must start in 1 and the limit is numargs+1
- unsigned NumArgs = Fn->getNumParams()+1; // +1 for ...
+ unsigned NumArgs = Fn->getNumParams();
+ unsigned FirstIdx = 1;
const char *Format = rawAttr->getParameterName()->getName();
unsigned FormatLen = rawAttr->getParameterName()->getLength();
@@ -2128,35 +2137,60 @@
return;
}
+ // checks for the 2nd argument
Expr *IdxExpr = static_cast<Expr *>(rawAttr->getArg(0));
- llvm::APSInt Idx(32);
+ llvm::APSInt Idx(Context.getTypeSize(IdxExpr->getType()));
if (!IdxExpr->isIntegerConstantExpr(Idx, Context)) {
Diag(rawAttr->getLoc(), diag::err_attribute_argument_n_not_int,
"format", std::string("2"), IdxExpr->getSourceRange());
return;
}
- if (Idx.getZExtValue() < 1 || Idx.getZExtValue() > NumArgs) {
+ if (Idx.getZExtValue() < FirstIdx || Idx.getZExtValue() > NumArgs) {
Diag(rawAttr->getLoc(), diag::err_attribute_argument_out_of_bounds,
"format", std::string("2"), IdxExpr->getSourceRange());
return;
}
+ // make sure the format string is really a string
+ QualType Ty = proto->getArgType(Idx.getZExtValue()-1);
+ if (!Ty->isPointerType() ||
+ !Ty->getAsPointerType()->getPointeeType()->isCharType()) {
+ Diag(rawAttr->getLoc(), diag::err_format_attribute_not_string,
+ IdxExpr->getSourceRange());
+ return;
+ }
+
+
+ // check the 3rd argument
Expr *FirstArgExpr = static_cast<Expr *>(rawAttr->getArg(1));
- llvm::APSInt FirstArg(32);
+ llvm::APSInt FirstArg(Context.getTypeSize(FirstArgExpr->getType()));
if (!FirstArgExpr->isIntegerConstantExpr(FirstArg, Context)) {
Diag(rawAttr->getLoc(), diag::err_attribute_argument_n_not_int,
"format", std::string("3"), FirstArgExpr->getSourceRange());
return;
}
+ // check if the function is variadic if the 3rd argument non-zero
+ if (FirstArg != 0) {
+ if (proto->isVariadic()) {
+ ++NumArgs; // +1 for ...
+ } else {
+ Diag(d->getLocation(), diag::err_format_attribute_requires_variadic);
+ return;
+ }
+ }
+
+ // strftime requires FirstArg to be 0 because it doesn't read from any
variable
+ // the input is just the current time + the format string
if (FormatLen == 8 && !memcmp(Format, "strftime", 8)) {
- if (FirstArg.getZExtValue() != 0) {
+ if (FirstArg != 0) {
Diag(rawAttr->getLoc(), diag::err_format_strftime_third_parameter,
FirstArgExpr->getSourceRange());
return;
}
- } else if (FirstArg.getZExtValue() > NumArgs) {
+ // if 0 it disables parameter checking (to use with e.g. va_list)
+ } else if (FirstArg != 0 && FirstArg != NumArgs) {
Diag(rawAttr->getLoc(), diag::err_attribute_argument_out_of_bounds,
"format", std::string("3"), FirstArgExpr->getSourceRange());
return;
_______________________________________________
cfe-dev mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
