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.
Locked
clang-ownership.patch (31K)