[cfe-dev] Ownership attribute for malloc etc. checking
Ted Kremenek
kremenek at apple.com
Mon Jun 28 17:42:47 PDT 2010
Hi Andrew,
In addition to Jordy's excellent comments, here are my comments on your patch. Comments inline:
Index: test/Analysis/malloc.c
===================================================================
--- test/Analysis/malloc.c (revision 106614)
+++ test/Analysis/malloc.c (working copy)
@@ -4,7 +4,13 @@
void free(void *);
void *realloc(void *ptr, size_t size);
void *calloc(size_t nmemb, size_t size);
+void __attribute((ownership_returns(malloc))) *my_malloc(size_t);
+void __attribute((ownership_takes(malloc, 1))) my_free(void *);
+void __attribute((ownership_returns(malloc, 1))) *my_malloc2(size_t);
+void __attribute((ownership_holds(malloc, 1))) my_hold(void *);
+void *my_malloc3(size_t);
Good test cases, but also include test cases where the index isn't '1'.
Please also include test cases when multiple indices are annotated with a single attribute.
Also, what happens if the same index is specified more than once?
Index: include/clang/Basic/Attr.td
===================================================================
--- include/clang/Basic/Attr.td (revision 106614)
+++ include/clang/Basic/Attr.td (working copy)
@@ -297,6 +297,21 @@
let Spellings = ["overloadable"];
}
+def OwnershipReturns : Attr {
+ let Spellings = ["ownership_returns"];
+ let Args = [StringArgument<"Module">, IntArgument<"SizeIdx">];
+}
+
+def OwnershipTakes : Attr {
+ let Spellings = ["ownership_takes"];
+ let Args = [StringArgument<"Module">, IntArgument<"PtrIdx">];
+}
+
+def OwnershipHolds : Attr {
+ let Spellings = ["ownership_holds"];
+ let Args = [StringArgument<"Module">, IntArgument<"PtrIdx">];
+}
+
def Packed : Attr {
let Spellings = ["packed"];
}
In addition to this syntax, Jeffrey Yaskin also made the excellent point of whether or not we want to allow the ownership_takes and ownership_holds attributes directly to be affixed directly to parameters instead of using indices. Should we allow both models, and thus make the PtrIdx optional in those cases?
I'm also not certain what the SizeIdx entry is for. Is this to explicitly model the size of the returned memory? That's fine, but please also keep in mind how we would want to model out-parameters. Would we want a separate attribute?
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 106614)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -831,6 +831,12 @@
"attribute may only be applied to an Objective-C interface">;
def err_nonnull_pointers_only : Error<
"nonnull attribute only applies to pointer arguments">;
+def err_ownership_returns_integers_only : Error<
+ "ownership_returns attribute only applies to integer arguments">;
+def err_ownership_takes_pointers_only : Error<
+ "ownership_takes attribute only applies to pointer arguments">;
+def err_ownership_holds_pointers_only : Error<
+ "ownership_holds attribute only applies to pointer arguments">;
def err_format_strftime_third_parameter : Error<
"strftime format attribute requires 3rd parameter to be 0">;
def err_format_attribute_requires_variadic : Error<
Index: include/clang/AST/Attr.h
===================================================================
--- include/clang/AST/Attr.h (revision 106614)
+++ include/clang/AST/Attr.h (working copy)
@@ -351,6 +351,128 @@
static bool classof(const NonNullAttr *A) { return true; }
};
+class OwnershipReturnsAttr: public AttrWithString {
+ unsigned* ArgNums;
+ unsigned Size;
+public:
+ OwnershipReturnsAttr(ASTContext &C, unsigned* arg_nums, unsigned size,
+ llvm::StringRef module);
+
+ virtual void Destroy(ASTContext &C);
+
+ llvm::StringRef getModule() const {
+ return getString();
+ }
+ void setModule(ASTContext &C, llvm::StringRef module);
+ typedef const unsigned *iterator;
+ iterator begin() const {
+ return ArgNums;
+ }
+ iterator end() const {
+ return ArgNums + Size;
+ }
+ unsigned size() const {
+ return Size;
+ }
+
+ bool isOwnershipReturns(unsigned arg) const {
+ return ArgNums ? std::binary_search(ArgNums, ArgNums + Size, arg) : true;
+ }
+
+ bool isModule(const char *m) const {
+ return getModule().equals(m);
+ }
+
+ virtual Attr *clone(ASTContext &C) const;
+
+ static bool classof(const Attr *A) {
+ return A->getKind() == attr::OwnershipReturns;
+ }
+ static bool classof(const OwnershipReturnsAttr *A) {
+ return true;
+ }
+};
+
+class OwnershipTakesAttr: public AttrWithString {
+ unsigned* ArgNums;
+ unsigned Size;
+public:
+ OwnershipTakesAttr(ASTContext &C, unsigned* arg_nums, unsigned size,
+ llvm::StringRef module);
+
+ virtual void Destroy(ASTContext &C);
+
+ llvm::StringRef getModule() const {
+ return getString();
+ }
+ void setModule(ASTContext &C, llvm::StringRef module);
+ typedef const unsigned *iterator;
+ iterator begin() const {
+ return ArgNums;
+ }
+ iterator end() const {
+ return ArgNums + Size;
+ }
+ unsigned size() const {
+ return Size;
+ }
+
+ bool isOwnershipTakes(unsigned arg) const {
+ return ArgNums ? std::binary_search(ArgNums, ArgNums + Size, arg) : true;
+ }
+
+ bool isModule(const char *m) const {
+ return getModule().equals(m);
+ }
+
+ virtual Attr *clone(ASTContext &C) const;
+
+ static bool classof(const Attr *A) {
+ return A->getKind() == attr::OwnershipTakes;
+ }
+ static bool classof(const OwnershipTakesAttr *A) {
+ return true;
+ }
+};
+
+class OwnershipHoldsAttr: public AttrWithString {
+ unsigned* ArgNums;
+ unsigned Size;
+public:
+ OwnershipHoldsAttr(ASTContext &C, unsigned* arg_nums, unsigned size,
+ llvm::StringRef module);
+
+ virtual void Destroy(ASTContext &C);
+
+ llvm::StringRef getModule() const {
+ return getString();
+ }
+ void setModule(ASTContext &C, llvm::StringRef module);
+ typedef const unsigned *iterator;
+ iterator begin() const {
+ return ArgNums;
+ }
+ iterator end() const {
+ return ArgNums + Size;
+ }
+ unsigned size() const {
+ return Size;
+ }
+
+ bool isOwnershipHolds(unsigned arg) const {
+ return ArgNums ? std::binary_search(ArgNums, ArgNums + Size, arg) : true;
+ }
+
+ bool isModule(const char *m) const {
+ return getModule().equals(m);
+ }
+
+ virtual Attr *clone(ASTContext &C) const;
+
+ static bool classof(const Attr *A) { return A->getKind() == attr::OwnershipHolds; }
+ static bool classof(const OwnershipHoldsAttr *A) { return true; }
+};
Consider having these three attribute classes subclass a common base class, e.g. OwnershipAttr. There's a bunch of duplication here which is unneeded. For example, setModule, getModule, etc., have identical implementations.
There is also value in having them derive from a common superclass. From a design perspective, it clearly marks them as being related.
Index: include/clang/Parse/AttributeList.h
===================================================================
--- include/clang/Parse/AttributeList.h (revision 106614)
+++ include/clang/Parse/AttributeList.h (working copy)
@@ -97,6 +97,9 @@
AT_ns_returns_retained, // Clang-specific.
AT_objc_gc,
AT_overloadable, // Clang-specific.
+ AT_ownership_returns, // Clang-specific.
+ AT_ownership_takes, // Clang-specific.
+ AT_ownership_holds, // Clang-specific.
Please keep these sorted w.r.t. each other.
AT_packed,
AT_pure,
AT_regparm,
Index: lib/Sema/SemaDeclAttr.cpp
===================================================================
--- lib/Sema/SemaDeclAttr.cpp (revision 106614)
+++ lib/Sema/SemaDeclAttr.cpp (working copy)
@@ -350,6 +350,261 @@
d->addAttr(::new (S.Context) NonNullAttr(S.Context, start, size));
}
+static void HandleOwnershipReturnsAttr(Decl *d, const AttributeList &Attr,
+ Sema &S) {
+ // This attribute must be applied to a function declaration.
+ // The first argument to the attribute must be a string,
+ // the name of the resource, for example "malloc".
+ // The next argument is optional, and represents the size of allocation.
+ // If present, it must be an integer less than the number of function arguments,
+ // this is an argument index, the argument must be of integer type.
+ // If not present, the allocation is of unknown size.
+
+ if (!Attr.getParameterName()) {
+ S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_not_string)
+ << "ownership_returns" << 1;
+ return;
+ }
+
+ if (!isFunctionOrMethodOrBlock(d) || !hasFunctionProto(d)) {
+ S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
+ << Attr.getName() << 0 /*function*/;
+ return;
+ }
These attributes only apply to functions, not blocks or Objective-C methods. We can possibly extend them to support methods (and possibly even blocks), but that's not in the current design. Your diagnostic even indicates that it only works on functions. Use 'isFunction' instead.
+
+ unsigned NumArgs = getFunctionOrMethodNumArgs(d);
+
+ llvm::StringRef Module = Attr.getParameterName()->getName();
+
+ // Normalize the argument, __foo__ becomes foo.
+ if (Module.startswith("__") && Module.endswith("__"))
+ Module = Module.substr(2, Module.size() - 4);
+
+ llvm::SmallVector<unsigned, 10> OwnershipReturnsArgs;
Note that the small vector here is 'unsigned', but that the arguments themselves can be signed. You should issue the appropriate diagnostic, and make sure the indices are non-negative.
+
+ if (Attr.getNumArgs() == 1) {
+ // Handle the index argument, if present.
+ Expr *IdxExpr = static_cast<Expr *> (Attr.getArg(0));
+ llvm::APSInt ArgNum(32);
+ if (IdxExpr->isTypeDependent() || IdxExpr->isValueDependent()
+ || !IdxExpr->isIntegerConstantExpr(ArgNum, S.Context)) {
+ S.Diag(Attr.getLoc(), diag::err_attribute_argument_not_int)
+ << "ownership_returns" << IdxExpr->getSourceRange();
+ return;
+ }
+
+ unsigned x = (unsigned) ArgNum.getZExtValue();
+
+ if (x < 1 || x > NumArgs) {
+ S.Diag(Attr.getLoc(), diag::err_attribute_argument_out_of_bounds)
+ << "ownership_returns" << 1 << IdxExpr->getSourceRange();
+ return;
+ }
+ --x;
+ // Is the function argument an integer type?
+ QualType T = getFunctionOrMethodArgType(d, x);
+ if (!T->isUnsignedIntegerType()) {
+ // FIXME: Should also highlight argument in decl.
+ S.Diag(Attr.getLoc(), diag::err_ownership_returns_integers_only)
+ << "ownership_returns" << IdxExpr->getSourceRange();
+ return;
+ }
+ OwnershipReturnsArgs.push_back(x);
+ }
Looks good.
+
+ unsigned* start = OwnershipReturnsArgs.data();
+ unsigned size = OwnershipReturnsArgs.size();
+ std::sort(start, start + size);
Please use array_pod_sort. std::sort causes a bunch of stuff to get pulled in (or instantiated) that bloats the code.
+ d->addAttr(::new (S.Context) OwnershipReturnsAttr(S.Context, start, size,
+ Module));
+}
Looks good.
+
+static void HandleOwnershipHoldsAttr(Decl *d, const AttributeList &AL,
+ Sema &S) {
+ // This attribute must be applied to a function declaration.
+ // The first argument to the attribute must be a string,
+ // the name of the resource, for example "malloc".
+ // The following arguments must be argument indexes, the arguments must be of pointer type.
+ // The difference between Holds and Takes is that a pointer may still be used
+ // after being held. free() should be __attribute((ownership_takes)), whereas a list
+ // append function may well be __attribute((ownership_holds)).
+
+ if (!AL.getParameterName()) {
+ S.Diag(AL.getLoc(), diag::err_attribute_argument_n_not_string)
+ << "ownership_holds" << 1;
+ return;
+ }
+
+ if (AL.getNumArgs() < 1) {
+ S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << 2;
+ return;
+ }
+
+ if (!isFunctionOrMethodOrBlock(d) || !hasFunctionProto(d)) {
+ S.Diag(AL.getLoc(), diag::warn_attribute_wrong_decl_type)
+ << AL.getName() << 0 /*function*/;
+ return;
+ }
Same comment as above. Use 'isFunction'.
+
+ unsigned NumArgs = getFunctionOrMethodNumArgs(d);
+
+ llvm::StringRef Module = AL.getParameterName()->getName();
+
+ // Normalize the argument, __foo__ becomes foo.
+ if (Module.startswith("__") && Module.endswith("__"))
+ Module = Module.substr(2, Module.size() - 4);
+
+ llvm::SmallVector<unsigned, 10> OwnershipHoldsArgs;
+
+ for (AttributeList::arg_iterator I = AL.arg_begin(), E = AL.arg_end(); I
+ != E; ++I) {
+
+ Expr *IdxExpr = static_cast<Expr *> (*I);
+ llvm::APSInt ArgNum(32);
+ if (IdxExpr->isTypeDependent() || IdxExpr->isValueDependent()
+ || !IdxExpr->isIntegerConstantExpr(ArgNum, S.Context)) {
+ S.Diag(AL.getLoc(), diag::err_attribute_argument_not_int)
+ << "ownership_holds" << IdxExpr->getSourceRange();
+ continue;
+ }
+
+ unsigned x = (unsigned) ArgNum.getZExtValue();
+
+ if (x < 1 || x > NumArgs) {
+ S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_bounds)
+ << "ownership_holds" << x << IdxExpr->getSourceRange();
+ continue;
+ }
+ --x;
+ // Is the function argument a pointer type?
+ QualType T = getFunctionOrMethodArgType(d, x);
+ if (!T->isAnyPointerType() && !T->isBlockPointerType()) {
+ // FIXME: Should also highlight argument in decl.
+ S.Diag(AL.getLoc(), diag::err_ownership_holds_pointers_only)
+ << IdxExpr->getSourceRange();
+ continue;
+ }
+ // Check we don't have a conflict with an ownership_takes attribute.
+ if (d->hasAttrs()) {
+ for (const Attr *attr = d->getAttrs(); attr; attr = attr->getNext()) {
+ if (const OwnershipTakesAttr* Att = dyn_cast<OwnershipTakesAttr> (attr)) {
+ for (const unsigned *I = Att->begin(), *E = Att->end(); I != E; ++I) {
+ if (x == *I) {
+ S.Diag(AL.getLoc(), diag::err_attributes_are_not_compatible)
+ << "ownership_takes" << "ownership_holds";
+ }
+ }
+ }
+ }
+ }
+ OwnershipHoldsArgs.push_back(x);
+ }
+
+ if (OwnershipHoldsArgs.empty()) {
+ S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << 2;
+ return;
+ }
+
+ unsigned* start = &OwnershipHoldsArgs[0];
+ unsigned size = OwnershipHoldsArgs.size();
+ std::sort(start, start + size);
+ d->addAttr(::new (S.Context) OwnershipHoldsAttr(S.Context, start, size,
+ Module));
+}
Most of this is copy-and-paste from HandleOwnershipReturnsAttr. This looks like it could easily be refactored, with few choice 'if' statements based on the attribute type differentiating behavior. Not only is this excessively verbose, this can lead to subtle bugs and inconsistencies. For example, see how HandleNSReturnsRetainedAttr() is used to process four different kinds of attributes. Please refactor the logic for these attributes into a common function.
+
+static void HandleOwnershipTakesAttr(Decl *d, const AttributeList &AL,
+ Sema &S) {
+ // This attribute must be applied to a function declaration.
+ // The first argument to the attribute must be a string,
+ // the name of the resource, for example "malloc".
+ // The following arguments must be argument indexes, the arguments must be of pointer type.
+ // The difference between Holds and Takes is that a pointer may still be used
+ // after being held. free() should be __attribute((ownership_takes)), whereas a list
+ // append function may well be __attribute((ownership_holds)).
+
+ if (!AL.getParameterName()) {
+ S.Diag(AL.getLoc(), diag::err_attribute_argument_n_not_string)
+ << "ownership_takes" << 1;
+ return;
+ }
+
+ if (AL.getNumArgs() < 1) {
+ S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << 2;
+ return;
+ }
+
+ if (!isFunctionOrMethodOrBlock(d) || !hasFunctionProto(d)) {
+ S.Diag(AL.getLoc(), diag::warn_attribute_wrong_decl_type)
+ << AL.getName() << 0 /*function*/;
+ return;
+ }
+
+ unsigned NumArgs = getFunctionOrMethodNumArgs(d);
+
+ llvm::StringRef Module = AL.getParameterName()->getName();
+
+ // Normalize the argument, __foo__ becomes foo.
+ if (Module.startswith("__") && Module.endswith("__"))
+ Module = Module.substr(2, Module.size() - 4);
+
+ llvm::SmallVector<unsigned, 10> OwnershipTakesArgs;
+
+ for (AttributeList::arg_iterator I = AL.arg_begin(), E = AL.arg_end(); I
+ != E; ++I) {
+
+ Expr *IdxExpr = static_cast<Expr *> (*I);
+ llvm::APSInt ArgNum(32);
+ if (IdxExpr->isTypeDependent() || IdxExpr->isValueDependent()
+ || !IdxExpr->isIntegerConstantExpr(ArgNum, S.Context)) {
+ S.Diag(AL.getLoc(), diag::err_attribute_argument_not_int)
+ << "ownership_takes" << IdxExpr->getSourceRange();
+ continue;
+ }
+
+ unsigned x = (unsigned) ArgNum.getZExtValue();
+
+ if (x < 1 || x > NumArgs) {
+ S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_bounds)
+ << "ownership_takes" << x << IdxExpr->getSourceRange();
+ continue;
+ }
+ --x;
+ // Is the function argument a pointer type?
+ QualType T = getFunctionOrMethodArgType(d, x);
+ if (!T->isAnyPointerType() && !T->isBlockPointerType()) {
+ // FIXME: Should also highlight argument in decl.
+ S.Diag(AL.getLoc(), diag::err_ownership_holds_pointers_only)
+ << IdxExpr->getSourceRange();
+ continue;
+ }
+ // Check we don't have a conflict with an ownership_holds attribute.
+ if (d->hasAttrs()) {
+ for (const Attr *attr = d->getAttrs(); attr; attr = attr->getNext()) {
+ if (const OwnershipHoldsAttr* Att = dyn_cast<OwnershipHoldsAttr> (attr)) {
+ for (const unsigned *I = Att->begin(), *E = Att->end(); I != E; ++I) {
+ if (x == *I) {
+ S.Diag(AL.getLoc(), diag::err_attributes_are_not_compatible)
+ << "ownership_takes" << "ownership_holds";
+ }
+ }
+ }
+ }
+ }
+ OwnershipTakesArgs.push_back(x);
+ }
+
+ if (OwnershipTakesArgs.empty()) {
+ S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << 2;
+ return;
+ }
+
+ unsigned* start = &OwnershipTakesArgs[0];
+ unsigned size = OwnershipTakesArgs.size();
+ std::sort(start, start + size);
+ d->addAttr(::new (S.Context) OwnershipTakesAttr(S.Context, start, size,
+ Module));
+}
+
Same comment as above. This is all copy-and-paste. We can do better.
static bool isStaticVarOrStaticFunciton(Decl *D) {
if (VarDecl *VD = dyn_cast<VarDecl>(D))
return VD->getStorageClass() == VarDecl::Static;
@@ -2002,6 +2257,12 @@
case AttributeList::AT_mode: HandleModeAttr (D, Attr, S); break;
case AttributeList::AT_malloc: HandleMallocAttr (D, Attr, S); break;
case AttributeList::AT_nonnull: HandleNonNullAttr (D, Attr, S); break;
+ case AttributeList::AT_ownership_returns:
+ HandleOwnershipReturnsAttr (D, Attr, S); break;
+ case AttributeList::AT_ownership_takes:
+ HandleOwnershipTakesAttr (D, Attr, S); break;
+ case AttributeList::AT_ownership_holds:
+ HandleOwnershipHoldsAttr (D, Attr, S); break;
Instead of calling three functions, probably calling one or two is fine (and looking at the Attr kind).
case AttributeList::AT_noreturn: HandleNoReturnAttr (D, Attr, S); break;
case AttributeList::AT_nothrow: HandleNothrowAttr (D, Attr, S); break;
case AttributeList::AT_override: HandleOverrideAttr (D, Attr, S); break;
Index: lib/AST/AttrImpl.cpp
===================================================================
--- lib/AST/AttrImpl.cpp (revision 106614)
+++ lib/AST/AttrImpl.cpp (working copy)
@@ -66,6 +66,70 @@
Attr::Destroy(C);
}
+OwnershipReturnsAttr::OwnershipReturnsAttr(ASTContext &C, unsigned* arg_nums,
+ unsigned size,
+ llvm::StringRef module) :
+ AttrWithString(attr::OwnershipReturns, C, module), ArgNums(0), Size(0) {
+ if (size == 0)
+ return;
+ assert(arg_nums);
+ ArgNums = new (C) unsigned[size];
+ Size = size;
+ memcpy(ArgNums, arg_nums, sizeof(*ArgNums) * size);
+}
Looks good.
+
+void OwnershipReturnsAttr::setModule(ASTContext &C, llvm::StringRef module) {
+ ReplaceString(C, module);
+}
+
+void OwnershipReturnsAttr::Destroy(ASTContext &C) {
+ if (ArgNums)
+ C.Deallocate(ArgNums);
+ Attr::Destroy(C);
+}
Looks good.
+
+OwnershipTakesAttr::OwnershipTakesAttr(ASTContext &C, unsigned* arg_nums,
+ unsigned size, llvm::StringRef module) :
+ AttrWithString(attr::OwnershipTakes, C, module), ArgNums(0), Size(0) {
+ if (size == 0)
+ return;
+ assert(arg_nums);
+ ArgNums = new (C) unsigned[size];
+ Size = size;
+ memcpy(ArgNums, arg_nums, sizeof(*ArgNums) * size);
+}
Notice how this is practically identical to OwnershipReturnsAttr? I think most of this can be merged into a common constructor of a base class.
+
+void OwnershipTakesAttr::setModule(ASTContext &C, llvm::StringRef module) {
+ ReplaceString(C, module);
+}
+
+void OwnershipTakesAttr::Destroy(ASTContext &C) {
+ if (ArgNums)
+ C.Deallocate(ArgNums);
+ Attr::Destroy(C);
+}
+
+OwnershipHoldsAttr::OwnershipHoldsAttr(ASTContext &C, unsigned* arg_nums,
+ unsigned size, llvm::StringRef module) :
+ AttrWithString(attr::OwnershipHolds, C, module), ArgNums(0), Size(0) {
+ if (size == 0)
+ return;
+ assert(arg_nums);
+ ArgNums = new (C) unsigned[size];
+ Size = size;
+ memcpy(ArgNums, arg_nums, sizeof(*ArgNums) * size);
+}
Same comment as with OwnershipReturnsAttr. This is basically copy-and-paste. The attributes are basically the same except one is "takes" instead of "holds". We shouldn't be duplicating so much code for that.
+
+void OwnershipHoldsAttr::setModule(ASTContext &C, llvm::StringRef module) {
+ ReplaceString(C, module);
+}
+
+void OwnershipHoldsAttr::Destroy(ASTContext &C) {
+ if (ArgNums)
+ C.Deallocate(ArgNums);
+ Attr::Destroy(C);
+}
+
#define DEF_SIMPLE_ATTR_CLONE(ATTR) \
Attr *ATTR##Attr::clone(ASTContext &C) const { \
return ::new (C) ATTR##Attr; \
@@ -165,6 +229,18 @@
return ::new (C) NonNullAttr(C, ArgNums, Size);
}
+Attr *OwnershipReturnsAttr::clone(ASTContext &C) const {
+ return ::new (C) OwnershipReturnsAttr(C, ArgNums, Size, getModule());
+}
+
+Attr *OwnershipTakesAttr::clone(ASTContext &C) const {
+ return ::new (C) OwnershipTakesAttr(C, ArgNums, Size, getModule());
+}
+
+Attr *OwnershipHoldsAttr::clone(ASTContext &C) const {
+ return ::new (C) OwnershipHoldsAttr(C, ArgNums, Size, getModule());
+}
+
Attr *FormatAttr::clone(ASTContext &C) const {
return ::new (C) FormatAttr(C, getType(), formatIdx, firstArg);
}
Index: lib/Lex/PPMacroExpansion.cpp
===================================================================
--- lib/Lex/PPMacroExpansion.cpp (revision 106614)
+++ lib/Lex/PPMacroExpansion.cpp (working copy)
@@ -505,6 +505,9 @@
.Case("cxx_static_assert", LangOpts.CPlusPlus0x)
.Case("objc_nonfragile_abi", LangOpts.ObjCNonFragileABI)
.Case("objc_weak_class", LangOpts.ObjCNonFragileABI)
+ .Case("ownership_returns", true)
+ .Case("ownership_takes", true)
+ .Case("ownership_holds", true)
Great!
//.Case("cxx_concepts", false)
//.Case("cxx_lambdas", false)
//.Case("cxx_nullptr", false)
Index: lib/Checker/MallocChecker.cpp
===================================================================
--- lib/Checker/MallocChecker.cpp (revision 106614)
+++ lib/Checker/MallocChecker.cpp (working copy)
@@ -24,15 +24,17 @@
namespace {
class RefState {
- enum Kind { AllocateUnchecked, AllocateFailed, Released, Escaped } K;
+ enum Kind { AllocateUnchecked, AllocateFailed, Released, Escaped, Relinquished } K;
const Stmt *S;
public:
RefState(Kind k, const Stmt *s) : K(k), S(s) {}
bool isAllocated() const { return K == AllocateUnchecked; }
+ bool isFailed() const { return K == AllocateFailed; }
bool isReleased() const { return K == Released; }
bool isEscaped() const { return K == Escaped; }
+ bool isRelinquished() const { return K == Relinquished; }
bool operator==(const RefState &X) const {
return K == X.K && S == X.S;
@@ -46,6 +48,7 @@
}
static RefState getReleased(const Stmt *s) { return RefState(Released, s); }
static RefState getEscaped(const Stmt *s) { return RefState(Escaped, s); }
+ static RefState getRelinquished(const Stmt *s) { return RefState(Relinquished, s); }
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddInteger(K);
@@ -59,12 +62,13 @@
BuiltinBug *BT_DoubleFree;
BuiltinBug *BT_Leak;
BuiltinBug *BT_UseFree;
+ BuiltinBug *BT_UseRelinquished;
BuiltinBug *BT_BadFree;
IdentifierInfo *II_malloc, *II_free, *II_realloc, *II_calloc;
public:
MallocChecker()
- : BT_DoubleFree(0), BT_Leak(0), BT_UseFree(0), BT_BadFree(0),
+ : BT_DoubleFree(0), BT_Leak(0), BT_UseFree(0), BT_UseRelinquished(0), BT_BadFree(0),
II_malloc(0), II_free(0), II_realloc(0), II_calloc(0) {}
static void *getTag();
bool EvalCallExpr(CheckerContext &C, const CallExpr *CE);
@@ -76,6 +80,7 @@
private:
void MallocMem(CheckerContext &C, const CallExpr *CE);
+ void MallocMemReturnsAttr(CheckerContext &C, const CallExpr *CE, const OwnershipReturnsAttr* Att);
const GRState *MallocMemAux(CheckerContext &C, const CallExpr *CE,
const Expr *SizeEx, SVal Init,
const GRState *state) {
@@ -86,8 +91,10 @@
const GRState *state);
void FreeMem(CheckerContext &C, const CallExpr *CE);
+ void FreeMemTakesAttr(CheckerContext &C, const CallExpr *CE, const OwnershipTakesAttr* Att);
+ void FreeMemHoldsAttr(CheckerContext &C, const CallExpr *CE, const OwnershipHoldsAttr* Att);
const GRState *FreeMemAux(CheckerContext &C, const CallExpr *CE,
- const GRState *state);
+ const GRState *state, unsigned Num, bool Hold);
void ReallocMem(CheckerContext &C, const CallExpr *CE);
void CallocMem(CheckerContext &C, const CallExpr *CE);
@@ -156,7 +163,26 @@
return true;
}
- return false;
+ // Check all the attributes, if there are any.
+ // There can be multiple of these attributes.
+ bool rv = false;
+ if (FD->hasAttrs()) {
+ for (const Attr *attr = FD->getAttrs(); attr; attr = attr->getNext()) {
+ if (const OwnershipReturnsAttr* Att = dyn_cast<OwnershipReturnsAttr>(attr)) {
+ MallocMemReturnsAttr(C, CE, Att);
+ rv = true;
+ }
+ if (const OwnershipTakesAttr* Att = dyn_cast<OwnershipTakesAttr> (attr)) {
+ FreeMemTakesAttr(C, CE, Att);
+ rv = true;
+ }
+ if (const OwnershipHoldsAttr* Att = dyn_cast<OwnershipHoldsAttr> (attr)) {
+ FreeMemHoldsAttr(C, CE, Att);
+ rv = true;
+ }
Use 'else if' to avoid redundant checks here.
+ }
+ }
+ return rv;
}
void MallocChecker::MallocMem(CheckerContext &C, const CallExpr *CE) {
@@ -165,6 +191,23 @@
C.addTransition(state);
}
+void MallocChecker::MallocMemReturnsAttr(CheckerContext &C, const CallExpr *CE,
+ const OwnershipReturnsAttr* Att) {
+ if (!Att->isModule("malloc"))
+ return;
+
+ const unsigned *I = Att->begin(), *E = Att->end();
+ if (I != E) {
+ const GRState *state =
+ MallocMemAux(C, CE, CE->getArg(*I), UndefinedVal(), C.getState());
+ C.addTransition(state);
+ return;
+ }
+ const GRState *state = MallocMemAux(C, CE, UndefinedVal(), UndefinedVal(),
+ C.getState());
+ C.addTransition(state);
+}
+
const GRState *MallocChecker::MallocMemAux(CheckerContext &C,
const CallExpr *CE,
SVal Size, SVal Init,
@@ -188,15 +231,41 @@
}
void MallocChecker::FreeMem(CheckerContext &C, const CallExpr *CE) {
- const GRState *state = FreeMemAux(C, CE, C.getState());
+ const GRState *state = FreeMemAux(C, CE, C.getState(), 0, false);
if (state)
C.addTransition(state);
}
+void MallocChecker::FreeMemTakesAttr(CheckerContext &C, const CallExpr *CE,
+ const OwnershipTakesAttr* Att) {
+ if (!Att->isModule("malloc"))
+ return;
+
+ for (const unsigned *I = Att->begin(), *E = Att->end(); I != E; ++I) {
+ const GRState *state =
+ FreeMemAux(C, CE, C.getState(), *I, false);
+ if (state)
+ C.addTransition(state);
+ }
+}
+
+void MallocChecker::FreeMemHoldsAttr(CheckerContext &C, const CallExpr *CE,
+ const OwnershipHoldsAttr* Att) {
+ if (!Att->isModule("malloc"))
+ return;
+
+ for (const unsigned *I = Att->begin(), *E = Att->end(); I != E; ++I) {
+ const GRState *state =
+ FreeMemAux(C, CE, C.getState(), *I, true);
+ if (state)
+ C.addTransition(state);
+ }
+}
+
const GRState *MallocChecker::FreeMemAux(CheckerContext &C, const CallExpr *CE,
- const GRState *state) {
- const Expr *ArgExpr = CE->getArg(0);
+ const GRState *state, unsigned Num, bool Hold) {
+ const Expr *ArgExpr = CE->getArg(Num);
SVal ArgVal = state->getSVal(ArgExpr);
// If ptr is NULL, no operation is preformed.
@@ -272,7 +341,24 @@
return NULL;
}
+ // Check free after relinquished ownership.
+ if (RS->isRelinquished()) {
+ ExplodedNode *N = C.GenerateSink();
+ if (N) {
+ if (!BT_UseRelinquished)
+ BT_UseRelinquished = new BuiltinBug("Double free",
+ "Try to free a memory block that has been passed elsewhere");
+ // FIXME: should find where it's freed last time.
+ BugReport *R = new BugReport(*BT_UseRelinquished,
+ BT_UseRelinquished->getDescription(), N);
+ C.EmitReport(R);
+ }
+ return NULL;
+ }
+
// Normal free.
+ if (Hold)
+ return state->set<RegionState>(Sym, RefState::getRelinquished(CE));
return state->set<RegionState>(Sym, RefState::getReleased(CE));
}
@@ -437,13 +523,13 @@
ValMgr.makeIntValWithPtrWidth(0, false));
if (const GRState *stateSizeZero = stateNotEqual->Assume(SizeZero, true)) {
- const GRState *stateFree = FreeMemAux(C, CE, stateSizeZero);
+ const GRState *stateFree = FreeMemAux(C, CE, stateSizeZero, 0, false);
if (stateFree)
C.addTransition(stateFree->BindExpr(CE, UndefinedVal(), true));
}
if (const GRState *stateSizeNotZero=stateNotEqual->Assume(SizeZero,false)) {
- const GRState *stateFree = FreeMemAux(C, CE, stateSizeNotZero);
+ const GRState *stateFree = FreeMemAux(C, CE, stateSizeNotZero, 0, false);
if (stateFree) {
// FIXME: We should copy the content of the original buffer.
const GRState *stateRealloc = MallocMemAux(C, CE, CE->getArg(1),
@@ -536,7 +622,6 @@
// FIXME: check other cases.
if (RS->isAllocated())
state = state->set<RegionState>(Sym, RefState::getEscaped(S));
-
C.addTransition(state);
}
Index: lib/Parse/AttributeList.cpp
===================================================================
--- lib/Parse/AttributeList.cpp (revision 106614)
+++ lib/Parse/AttributeList.cpp (working copy)
@@ -118,6 +118,9 @@
.Case("ns_returns_retained", AT_ns_returns_retained)
.Case("cf_returns_not_retained", AT_cf_returns_not_retained)
.Case("cf_returns_retained", AT_cf_returns_retained)
+ .Case("ownership_returns", AT_ownership_returns)
+ .Case("ownership_holds", AT_ownership_holds)
+ .Case("ownership_takes", AT_ownership_takes)
.Case("reqd_work_group_size", AT_reqd_wg_size)
.Case("init_priority", AT_init_priority)
.Case("no_instrument_function", AT_no_instrument_function)
Looks good.
On Jun 28, 2010, at 2:15 PM, Andrew McGregor wrote:
> Oops, forgot to reply-all...
>
> ---------- Forwarded message ----------
> From: Andrew McGregor <andrewmcgr at gmail.com>
> Date: Mon, Jun 28, 2010 at 12:04 PM
> Subject: Re: Ownership attribute for malloc etc. checking
> To: Jordy Rose <jediknil at belkadan.com>
>
>
> Hi,
>
> Thanks for the review. Comments inline, new patch attached.
>
> Andrew
>
> On Sun, Jun 27, 2010 at 9:33 AM, Jordy Rose <jediknil at belkadan.com> wrote:
>
> Looking good! Some remaining comments below.
>
> I'm also a reasonably new committer, so someone else should probably look
> over this as well (particularly the Attr hunks, which I'm not very familiar
> with).
>
> Jordy
>
>
> General:
> - It would be nice to make sure one argument isn't both ownership_takes
> and ownership_holds.
>
> True, will do.
>
> - LLVM's convention is to use spaces rather than tabs.
> - There's a hunk where the only thing that changed is indentation; it'd be
> nice to take that out.
>
> Oops, neither of these were intentional.
>
> - Two more useful test would be free(p); my_free(p) and free(p);
> my_hold(p). Both of which should warn, of course.
>
> Instead of crashing, yes. Well spotted.
>
> (MallocChecker::EvalCallExpr)
> + for (const Attr *attr = FD->getAttrs(); attr; attr = attr->getNext())
> {
> + if (const OwnershipReturnsAttr* Att =
> dyn_cast<OwnershipReturnsAttr>(attr)) {
> + MallocMemReturnsAttr(C, CE, Att);
> + rv = true;
> + }
> + if (const OwnershipTakesAttr* Att = dyn_cast<OwnershipTakesAttr>
> (attr)) {
> + FreeMemTakesAttr(C, CE, Att);
> + rv = true;
> + }
> + if (const OwnershipHoldsAttr* Att = dyn_cast<OwnershipHoldsAttr>
> (attr)) {
> + FreeMemHoldsAttr(C, CE, Att);
> + rv = true;
> + }
> + }
>
> Here's the dispatch from EvalCallExpr(). I think even though it's
> technically a little less efficient, you should just simplify this and use
> FD->getAttr<OwnershipTakesAttr>(), etc. It's a little simpler, and it lets
> you order the attributes.
>
> However, I did it this way because I wanted to allow two or more attributes of the same kind (for the benefit of macros).
>
> In other words, __attribute((ownership_holds, 1)) __attribute((ownership_holds, 2)) should be the same as __attribute((ownership_holds, 1, 2)).
>
> Why does that matter? Because an ownership_takes() or ownership_holds()
> function might have a return value! If it's also ownership_returns(), then
> you're fine, but otherwise you need to set a symbolic return value. (You
> can see how this is done in MallocMemAux(), or in StreamChecker.cpp with
> fopen.) There should probably be a test for this as well.
>
> It isn't necessarily the case that an ownership_takes() function that returns a pointer generated that pointer from an ownership_takes argument, or allocated anything, so what would you set the region to, in the absence of other information? They default to Unknown, yes?
>
> Actually, because these attributes don't affect the return value, they
> should probably go in a PreVisitCallExpr() callback, rather than
> EvalCallExpr(). But for now it's probably okay, as long as you handle that
> return value.
>
>
> (MallocChecker::MallocMemReturnsAttr)
> + for (const unsigned *I = Att->begin(), *E = Att->end(); I != E; ++I,
> ++count) {
> + const GRState *state =
> + MallocMemAux(C, CE, CE->getArg(*I), UndefinedVal(),
> C.getState());
> + C.addTransition(state);
> + }
> + if (count == 0) {
> + const GRState *state = MallocMemAux(C, CE, UndefinedVal(),
> UndefinedVal(),
> + C.getState());
> + C.addTransition(state);
> + }
>
> What does it mean to have multiple size fields? There can still only be
> one return value. Maybe you should only allow zero or one positions for
> ownership_returns (which would make this section simpler).
>
> The intention here was to implement something for calloc-like functions and matrix allocators; one argument is a special case, more than one the size of the region is the product of the arguments times sizeof(pointee of the return value), taking sizeof(void) as 1. If it isn't worth doing that, fair enough, but that's what I was thinking this could lead to.
>
> However, the parser only allows the one argument at the moment, so this is redundant.
>
> (MallocChecker::PreVisitReturnStmt)
> + if (RS->isReleased()) {
> + ExplodedNode *N = C.GenerateSink();
> + if (!BT_UseFree)
> + BT_UseFree = new BuiltinBug("Use dynamically allocated memory
> after"
> + " it is freed.");
>
> + BugReport *R = new BugReport(*BT_UseFree,
> BT_UseFree->getDescription(),
> + N);
> + C.EmitReport(R);
> + }
>
> This section checks whether the return value is a released address.
> Unfortunately this isn't a 100% unambiguous case; consider the following
> (contrived) example:
>
> struct Node *freeSmallest () {
> struct Node *min;
> // find the minimum node
> free(min);
> return min; // so the caller knows the ID of the deleted node
> }
>
> As long as the caller doesn't dereference the returned pointer, it could
> still be a useful value. 95% of the time, this check would be useful, but
> since a false positive is worse than a false negative for the analyzer, we
> should probably leave it out.
>
> Ok, I see. Out it is, then.
>
> Andrew
>
> <clang-ownership-withtests-r1.patch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100628/b91654af/attachment.html>
More information about the cfe-dev
mailing list