[cfe-commits] [cfe-dev] Proposal for thread safety attributes for Clang
Douglas Gregor
dgregor at apple.com
Thu Aug 4 12:58:12 PDT 2011
On Aug 1, 2011, at 4:17 PM, Caitlin Sadowski wrote:
> Here it is.
>
> Cheers,
I don't see the reason for this change:
--- a/lib/Sema/SemaDeclAttr.cpp
+++ b/lib/Sema/SemaDeclAttr.cpp
@@ -27,7 +27,7 @@ using namespace sema;
/// These constants match the enumerated choices of
/// warn_attribute_wrong_decl_type and err_attribute_wrong_decl_type.
-enum AttributeDeclType {
+enum {
ExpectedFunction,
ExpectedUnion,
ExpectedVariableOrFunction,
A few code style nits here:
+
+/// \brief Thread Safety Analysis: Check if passed in RecordType was annotated
+/// with the 'lockable' attribute. Argument should be non-null.
+static bool isLockable(const RecordType * RT) {
+ RecordDecl * RD = RT->getDecl();
LLVM style generally puts the '*' next to the declarator-id, e.g.,
RecordDecl *RD
+
+ if(!RD->hasAttrs())
+ return false;
+
+ AttrVec & ArgAttrs = RD->getAttrs();
+ for(unsigned int i = 0;i < ArgAttrs.size();++i) {
+ if(isa < LockableAttr > (ArgAttrs[i]))
Usually written as isa<LockableAttr>(ArgAttrs[i])
+/// \brief Thread Safety Analysis: Checks that all attribute arguments, starting
+/// from Sidx, resolve to a lockable object. May flag an error.
+static bool checkAttrArgsAreLockableObjs(Sema & S, Decl *D,
+ const AttributeList & Attr,
+ int Sidx = 0,
+ bool ParamIdxOk = false) {
+ for(unsigned int Idx = Sidx; Idx < Attr.getNumArgs(); ++Idx) {
+ Expr * ArgExp = Attr.getArg(Idx);
+ QualType Arg_QT = ArgExp->getType();
You probably want to check whether ArgExpr->isTypeDependent() here, and then just "continue" if it is, because when the argument type is dependent, you'll have no way to determine whether it's a lockable type or not.
+ // Get record type.
+ // first see if we can just cast to record type
+ const RecordType * RT = Arg_QT->getAs<RecordType>();
+
+ // now check if we point to record type
+ if (!RT && Arg_QT->isPointerType()) {
+ QualType PT = Arg_QT->getAs<PointerType>()->getPointeeType();
+ RT = PT->getAs<RecordType>();
+ }
+
+ // now check if we idx into a record type function param
+ if (!RT && ParamIdxOk) {
+ FunctionDecl * FD = dyn_cast <FunctionDecl>(D);
+ IntegerLiteral * IL = dyn_cast<IntegerLiteral>(ArgExp);
You want exactly integer literals here? No extra parentheses, no "1+2", just an integer literal?
+ if(FD && IL) {
+ unsigned int NumParams = FD->getNumParams();
+ llvm::APInt ArgValue = IL->getValue();
+ uint64_t ParamIdx_from1 = ArgValue.getZExtValue();
+ uint64_t ParamIdx_from0 = ParamIdx_from1 - 1;
+ if(!ArgValue.isStrictlyPositive() || ParamIdx_from1 > NumParams) {
+ S.Diag(Attr.getLoc(), diag::err_attribute_argument_out_of_range)
+ << Attr.getName() << Idx + 1 << NumParams;
+ return false;
+ }
+ RT = FD->getParamDecl(ParamIdx_from0)->getType()->getAs<RecordType>();
Don't you want to look through parameters of reference or pointer type here?
+ }
+ }
+
+ // Flag error if could not get record type for this argument
+ if (!RT) {
+ S.Diag(Attr.getLoc(), diag::err_attribute_argument_not_class)
+ << Attr.getName();
+ return false;
+ }
+ // Flag error if the type is not lockable
+ if (!isLockable(RT)) {
+ S.Diag(Attr.getLoc(), diag::err_attribute_argument_not_lockable)
+ << Attr.getName();
+ return false;
+ }
+ }
+ return true;
+}
+/// \brief Thread Safety Analysis: Check if passed in RecordType was annotated
+/// with the 'lockable' attribute. Argument should be non-null.
+static bool isLockable(const RecordType * RT) {
+ RecordDecl * RD = RT->getDecl();
+
+ if(!RD->hasAttrs())
+ return false;
+
+ AttrVec & ArgAttrs = RD->getAttrs();
+ for(unsigned int i = 0;i < ArgAttrs.size();++i) {
+ if(isa < LockableAttr > (ArgAttrs[i]))
+ return true;
+ }
This whole function collapses down to
RT->getDecl()->getAttr<LockableAttr>();
@@ -371,6 +441,21 @@ static void handleAcquireOrderAttr(Sema &S, Decl *D, const AttributeList &Attr,
return;
}
+ // check that this attribute only applies to lockable types
+ if (ValueDecl *VD = dyn_cast<ValueDecl>(D)) {
+ if (const RecordType * RT = VD->getType()->getAs<RecordType>()) {
+ if (!isLockable(RT)) {
+ S.Diag(Attr.getLoc(), diag::err_attribute_decl_not_lockable)
+ << Attr.getName();
+ return;
+ }
+ }
+ }
The comment and code don't agree. The code is checking that the type of the value is either not a RecordType or is a lockable record type.
If you do intend this to do what the comment says, you should also allow dependent types (which occur in templates; the attribute will need to get checked again when the variable is instantiated). Also, do you want to permit variables of reference-to-lockable or pointer-to-lockable type?
- if (!isFunction(D)) {
+ if (!isa<FunctionDecl>(D)) {
This happens a number of times. You probably also want to allow FunctionTemplateDecl, and at some point we'll also want ObjCMethodDecl.
- Doug
More information about the cfe-commits
mailing list