[clang][ThreadSafety] Add __builtin_instance_member (WIP)

[tbaeder]

Summary

As discussed in https://github.com/llvm/llvm-project/issues/20777, this adds __builtin_instance_member(membername), which acts like this->membername, but in C.

This is obviously very much WIP and the patch contains several placeholders, but I wonder if this is the right approach to take here and if I should continue on this path, so I'm opening this for review.

As you can see from the test case, this works for the (one) use case it was created for.

[tbaeder]

Ping

[aaronpuchert]

I'm wondering why you chose this over a direct equivalent to this, say __builtin_instance(), such that instead of __builtin_instance_member(M) one would write __builtin_instance().M or __builtin_instance()->M? While allowing to reference the same fields, it would additionally allow referencing the object itself. That's not so interesting for Thread Safety Analysis, because many attributes refer to this if no argument is given, but it might be helpful for other analyses.

It also seems cleaner C/C++ to me, as you can read the expression in the usual way, whereas in __builtin_instance_member(M), M is not a DeclRefExpr. And you could do #define THIS __builtin_instance().

[tbaeder]

Didn't remember and re-checked, using it that way makes the implementation harder I think:

tsa2.c:7:54: error: incomplete definition of type 'struct Mutex'
    7 |   int counter GUARDED_BY(__builtin_instance_member(M)->M);
      |                                                      ^
tsa2.c:6:10: note: forward declaration of 'struct Mutex'
    6 |   struct Mutex *M;
      |          ^
1 error generated.

and I didn't immediately know how to fix that.

[tbaeder]

Ping

[aaronpuchert]

We probably parse the attributes delayed in C++ but not in C. This probably also means you can't refer to later members in the attribute, so the mutex always needs to come first, right? Not sure if such a limitation is expected for C developers.

[tbaeder]

As discussed via other channels, this is going nowhere and we will take another approach, if any.

clang/include/clang/AST/EvaluatedExprVisitor.h

 #define PTR(CLASS) typename Ptr<CLASS>::type
 
   explicit EvaluatedExprVisitorBase(const ASTContext &Context) : Context(Context) { }
 
   // Expressions that have no potentially-evaluated subexpressions (but may have
   // other sub-expressions).
   void VisitDeclRefExpr(PTR(DeclRefExpr) E) { }
   void VisitOffsetOfExpr(PTR(OffsetOfExpr) E) { }
+  void VisitBuiltinInstanceMemberExpr(PTR(BuiltinInstanceMemberExpr) E) {}
   void VisitUnaryExprOrTypeTraitExpr(PTR(UnaryExprOrTypeTraitExpr) E) { }
   void VisitExpressionTraitExpr(PTR(ExpressionTraitExpr) E) { }
   void VisitBlockExpr(PTR(BlockExpr) E) { }
   void VisitCXXUuidofExpr(PTR(CXXUuidofExpr) E) { }
   void VisitCXXNoexceptExpr(PTR(CXXNoexceptExpr) E) { }
 
   void VisitMemberExpr(PTR(MemberExpr) E) {
     // Only the base matters.

clang/include/clang/AST/Expr.h

public:
   const_child_range children() const {
     Stmt *const *begin =
         reinterpret_cast<Stmt *const *>(getTrailingObjects<Expr *>());
     return const_child_range(begin, begin + NumExprs);
   }
   friend TrailingObjects;
 };
 
+class BuiltinInstanceMemberExpr final : public Expr {
+public:
+  const FieldDecl *F;
+  BuiltinInstanceMemberExpr(const ASTContext &C, const FieldDecl *F);
+
+  static BuiltinInstanceMemberExpr *Create(const ASTContext &C,
+                                           const FieldDecl *F);
+
+  SourceLocation getBeginLoc() const LLVM_READONLY { return SourceLocation(); }
+  SourceLocation getEndLoc() const LLVM_READONLY { return SourceLocation(); }
+
+  static bool classof(const Stmt *T) {
+    return T->getStmtClass() == BuiltinInstanceMemberExprClass;
+  }
+
+  // Iterators
+  child_range children() {
+    return child_range(child_iterator(), child_iterator());
+  }
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
+
+  const FieldDecl *getFieldDecl() const { return F; }
+};
+
 /// UnaryExprOrTypeTraitExpr - expression with either a type or (unevaluated)
 /// expression operand.  Used for sizeof/alignof (C99 6.5.3.4) and
 /// vec_step (OpenCL 1.1 6.11.12).
 class UnaryExprOrTypeTraitExpr : public Expr {
   union {
     TypeSourceInfo *Ty;
     Stmt *Ex;
   } Argument;

clang/include/clang/AST/RecursiveASTVisitor.h

 DEF_TRAVERSE_STMT(OffsetOfExpr, {
   // The child-iterator will pick up the expression representing
   // the field.
   // FIMXE: for code like offsetof(Foo, a.b.c), should we get
   // making a MemberExpr callbacks for Foo.a, Foo.a.b, and Foo.a.b.c?
   TRY_TO(TraverseTypeLoc(S->getTypeSourceInfo()->getTypeLoc()));
 })
 
+DEF_TRAVERSE_STMT(BuiltinInstanceMemberExpr, {})
+
 DEF_TRAVERSE_STMT(UnaryExprOrTypeTraitExpr, {
   // The child-iterator will pick up the arg if it's an expression,
   // but not if it's a type.
   if (S->isArgumentType())
     TRY_TO(TraverseTypeLoc(S->getArgumentTypeInfo()->getTypeLoc()));
 })
 
 DEF_TRAVERSE_STMT(CXXTypeidExpr, {

clang/include/clang/Basic/StmtNodes.td

 def NoInitExpr : StmtNode<Expr>;
 def ArrayInitLoopExpr : StmtNode<Expr>;
 def ArrayInitIndexExpr : StmtNode<Expr>;
 def ParenListExpr : StmtNode<Expr>;
 def VAArgExpr : StmtNode<Expr>;
 def GenericSelectionExpr : StmtNode<Expr>;
 def PseudoObjectExpr : StmtNode<Expr>;
 def SourceLocExpr : StmtNode<Expr>;
+def BuiltinInstanceMemberExpr : StmtNode<Expr>;
 
 // Wrapper expressions
 def FullExpr : StmtNode<Expr, 1>;
 def ConstantExpr : StmtNode<FullExpr>;
 
 // Atomic expressions
 def AtomicExpr : StmtNode<Expr>;
 

clang/include/clang/Basic/TokenKinds.def

 KEYWORD(_Decimal128                 , KEYALL)
 KEYWORD(__null                      , KEYCXX)
 // __alignof returns the preferred alignment of a type, the alignment
 // clang will attempt to give an object of the type if allowed by ABI.
 UNARY_EXPR_OR_TYPE_TRAIT(__alignof, PreferredAlignOf, KEYALL)
 KEYWORD(__attribute                 , KEYALL)
 KEYWORD(__builtin_choose_expr       , KEYALL)
 KEYWORD(__builtin_offsetof          , KEYALL)
+KEYWORD(__builtin_instance_member   , KEYALL)
 KEYWORD(__builtin_FILE              , KEYALL)
 KEYWORD(__builtin_FILE_NAME         , KEYALL)
 KEYWORD(__builtin_FUNCTION          , KEYALL)
 KEYWORD(__builtin_FUNCSIG           , KEYMS)
 KEYWORD(__builtin_LINE              , KEYALL)
 KEYWORD(__builtin_COLUMN            , KEYALL)
 KEYWORD(__builtin_source_location   , KEYCXX)
 

clang/include/clang/Sema/Sema.h

ExprResult BuildBuiltinOffsetOf(SourceLocation BuiltinLoc,
                                   ArrayRef<OffsetOfComponent> Components,
                                   SourceLocation RParenLoc);
   ExprResult ActOnBuiltinOffsetOf(Scope *S,
                                   SourceLocation BuiltinLoc,
                                   SourceLocation TypeLoc,
                                   ParsedType ParsedArgTy,
                                   ArrayRef<OffsetOfComponent> Components,
                                   SourceLocation RParenLoc);
+  ExprResult ActOnBuiltinInstanceMember(const IdentifierInfo *II);
 
   // __builtin_choose_expr(constExpr, expr1, expr2)
   ExprResult ActOnChooseExpr(SourceLocation BuiltinLoc,
                              Expr *CondExpr, Expr *LHSExpr,
                              Expr *RHSExpr, SourceLocation RPLoc);
 
   // __builtin_va_arg(expr, type)
   ExprResult ActOnVAArg(SourceLocation BuiltinLoc, Expr *E, ParsedType Ty,

clang/lib/AST/Expr.cpp

if (const auto *OCE = dyn_cast<CXXOperatorCallExpr>(this))
     return OCE->getEndLoc();
 
   SourceLocation end = getRParenLoc();
   if (end.isInvalid() && getNumArgs() > 0 && getArg(getNumArgs() - 1))
     end = getArg(getNumArgs() - 1)->getEndLoc();
   return end;
 }
 
+BuiltinInstanceMemberExpr *
+BuiltinInstanceMemberExpr::Create(const ASTContext &C, const FieldDecl *F) {
+  void *Mem = C.Allocate(sizeof(BuiltinInstanceMemberExpr));
+  return new (Mem) BuiltinInstanceMemberExpr(C, F);
+}
+
+BuiltinInstanceMemberExpr::BuiltinInstanceMemberExpr(const ASTContext &C,
+                                                     const FieldDecl *F)
+    : Expr(BuiltinInstanceMemberExprClass, F->getType(), VK_PRValue,
+           OK_Ordinary) {
+  this->F = F;
+}
+
 OffsetOfExpr *OffsetOfExpr::Create(const ASTContext &C, QualType type,
                                    SourceLocation OperatorLoc,
                                    TypeSourceInfo *tsi,
                                    ArrayRef<OffsetOfNode> comps,
                                    ArrayRef<Expr*> exprs,
                                    SourceLocation RParenLoc) {
   void *Mem = C.Allocate(
       totalSizeToAlloc<OffsetOfNode, Expr *>(comps.size(), exprs.size()));

clang/lib/AST/StmtPrinter.cpp

if (!Node->isPostfix()) {
     }
   }
   PrintExpr(Node->getSubExpr());
 
   if (Node->isPostfix())
     OS << UnaryOperator::getOpcodeStr(Node->getOpcode());
 }
 
+void StmtPrinter::VisitBuiltinInstanceMemberExpr(
+    BuiltinInstanceMemberExpr *Node) {}
+
 void StmtPrinter::VisitOffsetOfExpr(OffsetOfExpr *Node) {
   OS << "__builtin_offsetof(";
   Node->getTypeSourceInfo()->getType().print(OS, Policy);
   OS << ", ";
   bool PrintedSomething = false;
   for (unsigned i = 0, n = Node->getNumComponents(); i < n; ++i) {
     OffsetOfNode ON = Node->getComponent(i);
     if (ON.getKind() == OffsetOfNode::Array) {

clang/lib/AST/StmtProfile.cpp

void StmtProfiler::VisitParenListExpr(const ParenListExpr *S) {
   VisitExpr(S);
 }
 
 void StmtProfiler::VisitUnaryOperator(const UnaryOperator *S) {
   VisitExpr(S);
   ID.AddInteger(S->getOpcode());
 }
 
+void StmtProfiler::VisitBuiltinInstanceMemberExpr(
+    const BuiltinInstanceMemberExpr *S) {}
+
 void StmtProfiler::VisitOffsetOfExpr(const OffsetOfExpr *S) {
   VisitType(S->getTypeSourceInfo()->getType());
   unsigned n = S->getNumComponents();
   for (unsigned i = 0; i < n; ++i) {
     const OffsetOfNode &ON = S->getComponent(i);
     ID.AddInteger(ON.getKind());
     switch (ON.getKind()) {
     case OffsetOfNode::Array:

clang/lib/Analysis/ThreadSafetyCommon.cpp

 }
 
 std::pair<til::LiteralPtr *, StringRef>
 SExprBuilder::createThisPlaceholder(const Expr *Exp) {
   return {new (Arena) til::LiteralPtr(nullptr),
           ClassifyDiagnostic(Exp->getType())};
 }
 
+static const ValueDecl *getValueDeclFromSExpr(const til::SExpr *E) {
+  if (const auto *V = dyn_cast<til::Variable>(E))
+    return V->clangDecl();
+  if (const auto *Ph = dyn_cast<til::Phi>(E))
+    return Ph->clangDecl();
+  if (const auto *P = dyn_cast<til::Project>(E))
+    return P->clangDecl();
+  if (const auto *L = dyn_cast<til::LiteralPtr>(E))
+    return L->clangDecl();
+  return nullptr;
+}
+
+static bool hasAnyPointerType(const til::SExpr *E) {
+  auto *VD = getValueDeclFromSExpr(E);
+  if (VD && VD->getType()->isAnyPointerType())
+    return true;
+  if (const auto *C = dyn_cast<til::Cast>(E))
+    return C->castOpcode() == til::CAST_objToPtr;
+
+  return false;
+}
+
 // Translate a clang statement or expression to a TIL expression.
 // Also performs substitution of variables; Ctx provides the context.
 // Dispatches on the type of S.
 til::SExpr *SExprBuilder::translate(const Stmt *S, CallingContext *Ctx) {
   if (!S)
     return nullptr;
 
   // Check if S has already been translated and cached.
   // This handles the lookup of SSA names for DeclRefExprs here.
   if (til::SExpr *E = lookupStmt(S))
     return E;
 
   switch (S->getStmtClass()) {
   case Stmt::DeclRefExprClass:
     return translateDeclRefExpr(cast<DeclRefExpr>(S), Ctx);
   case Stmt::CXXThisExprClass:
     return translateCXXThisExpr(cast<CXXThisExpr>(S), Ctx);
+
+  case Stmt::BuiltinInstanceMemberExprClass: {
+    const BuiltinInstanceMemberExpr *BSME = cast<BuiltinInstanceMemberExpr>(S);
+    til::SExpr *SelfExpr = translate(cast<const Expr *>(Ctx->SelfArg), Ctx);
+    til::SExpr *BE = SelfExpr;
+    til::SExpr *E = new (Arena) til::SApply(BE);
+
+    til::Project *P = new (Arena) til::Project(E, BSME->getFieldDecl());
+    if (hasAnyPointerType(BE))
+      P->setArrow(true);
+    return P;
+  }
   case Stmt::MemberExprClass:
     return translateMemberExpr(cast<MemberExpr>(S), Ctx);
   case Stmt::ObjCIvarRefExprClass:
     return translateObjCIVarRefExpr(cast<ObjCIvarRefExpr>(S), Ctx);
   case Stmt::CallExprClass:
     return translateCallExpr(cast<CallExpr>(S), Ctx);
   case Stmt::CXXMemberCallExprClass:
     return translateCXXMemberCallExpr(cast<CXXMemberCallExpr>(S), Ctx);
if (const auto *SelfArg = dyn_cast<const Expr *>(Ctx->SelfArg))
       return translate(SelfArg, Ctx->Prev);
     else
       return cast<til::SExpr *>(Ctx->SelfArg);
   }
   assert(SelfVar && "We have no variable for 'this'!");
   return SelfVar;
 }
 
-static const ValueDecl *getValueDeclFromSExpr(const til::SExpr *E) {
-  if (const auto *V = dyn_cast<til::Variable>(E))
-    return V->clangDecl();
-  if (const auto *Ph = dyn_cast<til::Phi>(E))
-    return Ph->clangDecl();
-  if (const auto *P = dyn_cast<til::Project>(E))
-    return P->clangDecl();
-  if (const auto *L = dyn_cast<til::LiteralPtr>(E))
-    return L->clangDecl();
-  return nullptr;
-}
-
-static bool hasAnyPointerType(const til::SExpr *E) {
-  auto *VD = getValueDeclFromSExpr(E);
-  if (VD && VD->getType()->isAnyPointerType())
-    return true;
-  if (const auto *C = dyn_cast<til::Cast>(E))
-    return C->castOpcode() == til::CAST_objToPtr;
-
-  return false;
-}
-
 // Grab the very first declaration of virtual method D
 static const CXXMethodDecl *getFirstVirtualDecl(const CXXMethodDecl *D) {
   while (true) {
     D = D->getCanonicalDecl();
     auto OverriddenMethods = D->overridden_methods();
     if (OverriddenMethods.begin() == OverriddenMethods.end())
       return D;  // Method does not override anything
     // FIXME: this does not work with multiple inheritance.

clang/lib/Parse/ParseExpr.cpp

#undef RTT_JOIN
   case tok::kw__Generic:   // primary-expression: generic-selection [C11 6.5.1]
     Res = ParseGenericSelectionExpression();
     break;
   case tok::kw___builtin_available:
     Res = ParseAvailabilityCheckExpr(Tok.getLocation());
     break;
   case tok::kw___builtin_va_arg:
   case tok::kw___builtin_offsetof:
+  case tok::kw___builtin_instance_member:
   case tok::kw___builtin_choose_expr:
   case tok::kw___builtin_astype: // primary-expression: [OCL] as_type()
   case tok::kw___builtin_convertvector:
   case tok::kw___builtin_COLUMN:
   case tok::kw___builtin_FILE:
   case tok::kw___builtin_FILE_NAME:
   case tok::kw___builtin_FUNCTION:
   case tok::kw___builtin_FUNCSIG:
case tok::kw___builtin_va_arg: {
     }
 
     if (Expr.isInvalid() || Ty.isInvalid())
       Res = ExprError();
     else
       Res = Actions.ActOnVAArg(StartLoc, Expr.get(), Ty.get(), ConsumeParen());
     break;
   }
+  case tok::kw___builtin_instance_member: {
+    auto *II = Tok.getIdentifierInfo();
+    ConsumeToken();
+    if (Tok.isNot(tok::r_paren)) {
+      Diag(Tok, diag::err_expected) << tok::r_paren;
+      return ExprError();
+    }
+    PT.consumeClose();
+
+    Res = Actions.ActOnBuiltinInstanceMember(II);
+    break;
+  }
   case tok::kw___builtin_offsetof: {
     SourceLocation TypeLoc = Tok.getLocation();
     auto OOK = Sema::OffsetOfKind::OOK_Builtin;
     if (Tok.getLocation().isMacroID()) {
       StringRef MacroName = Lexer::getImmediateMacroNameForDiagnostics(
           Tok.getLocation(), PP.getSourceManager(), getLangOpts());
       if (MacroName == "offsetof")
         OOK = Sema::OffsetOfKind::OOK_Macro;

clang/lib/Sema/SemaExpr.cpp

for (const OffsetOfComponent &OC : Components) {
 
     CurrentType = MemberDecl->getType().getNonReferenceType();
   }
 
   return OffsetOfExpr::Create(Context, Context.getSizeType(), BuiltinLoc, TInfo,
                               Comps, Exprs, RParenLoc);
 }
 
+ExprResult Sema::ActOnBuiltinInstanceMember(const IdentifierInfo *MemberII) {
+  const auto *RD = dyn_cast<RecordDecl>(getFunctionLevelDeclContext());
+  assert(RD); // TODO: Diagnostic.
+
+  auto It = llvm::find_if(RD->fields(), [MemberII](const FieldDecl *F) {
+    return F->getIdentifier() == MemberII;
+  });
+
+  assert(It != RD->fields().end()); // TODO: Diagnostic.
+
+  return BuiltinInstanceMemberExpr::Create(Context, *It);
+}
+
 ExprResult Sema::ActOnBuiltinOffsetOf(Scope *S,
                                       SourceLocation BuiltinLoc,
                                       SourceLocation TypeLoc,
                                       ParsedType ParsedArgTy,
                                       ArrayRef<OffsetOfComponent> Components,
                                       SourceLocation RParenLoc) {
 
   TypeSourceInfo *ArgTInfo;

clang/lib/Sema/TreeTransform.h

TreeTransform<Derived>::TransformUnaryOperator(UnaryOperator *E) {
   if (!getDerived().AlwaysRebuild() && SubExpr.get() == E->getSubExpr())
     return E;
 
   return getDerived().RebuildUnaryOperator(E->getOperatorLoc(),
                                            E->getOpcode(),
                                            SubExpr.get());
 }
 
-template<typename Derived>
+template <typename Derived>
+ExprResult TreeTransform<Derived>::TransformBuiltinInstanceMemberExpr(
+    BuiltinInstanceMemberExpr *E) {
+  return ExprError();
+}
+
+template<typename Derived>
 ExprResult
 TreeTransform<Derived>::TransformOffsetOfExpr(OffsetOfExpr *E) {
   // Transform the type.
   TypeSourceInfo *Type = getDerived().TransformType(E->getTypeSourceInfo());
   if (!Type)
     return ExprError();
 
   // Transform all of the components into components similar to what the

clang/lib/Serialization/ASTReaderStmt.cpp

void ASTStmtReader::VisitUnaryOperator(UnaryOperator *E) {
   E->setOpcode((UnaryOperator::Opcode)Record.readInt());
   E->setOperatorLoc(readSourceLocation());
   E->setCanOverflow(Record.readInt());
   if (hasFP_Features)
     E->setStoredFPFeatures(
         FPOptionsOverride::getFromOpaqueInt(Record.readInt()));
 }
 
+void ASTStmtReader::VisitBuiltinInstanceMemberExpr(
+    BuiltinInstanceMemberExpr *E) {}
+
 void ASTStmtReader::VisitOffsetOfExpr(OffsetOfExpr *E) {
   VisitExpr(E);
   assert(E->getNumComponents() == Record.peekInt());
   Record.skipInts(1);
   assert(E->getNumExpressions() == Record.peekInt());
   Record.skipInts(1);
   E->setOperatorLoc(readSourceLocation());
   E->setRParenLoc(readSourceLocation());

clang/lib/Serialization/ASTWriterStmt.cpp

void ASTStmtWriter::VisitUnaryOperator(UnaryOperator *E) {
   Record.push_back(E->getOpcode()); // FIXME: stable encoding
   Record.AddSourceLocation(E->getOperatorLoc());
   Record.push_back(E->canOverflow());
   if (HasFPFeatures)
     Record.push_back(E->getStoredFPFeatures().getAsOpaqueInt());
   Code = serialization::EXPR_UNARY_OPERATOR;
 }
 
+void ASTStmtWriter::VisitBuiltinInstanceMemberExpr(
+    BuiltinInstanceMemberExpr *E) {}
+
 void ASTStmtWriter::VisitOffsetOfExpr(OffsetOfExpr *E) {
   VisitExpr(E);
   Record.push_back(E->getNumComponents());
   Record.push_back(E->getNumExpressions());
   Record.AddSourceLocation(E->getOperatorLoc());
   Record.AddSourceLocation(E->getRParenLoc());
   Record.AddTypeSourceInfo(E->getTypeSourceInfo());
   for (unsigned I = 0, N = E->getNumComponents(); I != N; ++I) {

clang/test/Sema/warn-thread-safety-analysis.c

mutex_shared_unlock(&mu1);    // expected-warning {{releasing mutex 'mu1' that was not held}}
 
     Bar_fun1(3);
   }
   Bar_fun1(4); // expected-warning {{calling function 'Bar_fun1' requires holding mutex 'mu1' exclusively}}
 
   return 0;
 }
 
+struct Holder {
+  struct Mutex *M;
+  int counter GUARDED_BY(__builtin_instance_member(M));
+};
+
+static void lock_holder(struct Holder *H) __attribute__((acquire_capability(H->M))) NO_THREAD_SAFETY_ANALYSIS {}
+static void unlock_holder(struct Holder *H) __attribute__((release_capability(H->M))) NO_THREAD_SAFETY_ANALYSIS {}
+
+static void test_holder(void) {
+  struct Holder H = {(void*)0, 0};
+
+  lock_holder(&H);
+  H.counter++;
+  unlock_holder(&H);
+
+  H.counter--; // expected-warning {{requires holding mutex 'H.M'}}
+}
+
 // We had a problem where we'd skip all attributes that follow a late-parsed
 // attribute in a single __attribute__.
 void run(void) __attribute__((guarded_by(mu1), guarded_by(mu1))); // expected-warning 2{{only applies to non-static data members and global variables}}