[PATCH] D153001: [clang][ThreadSafety] Add __builtin_instance_member (WIP)

2023-10-11 Thread Timm Bäder via Phabricator via cfe-commits
tbaeder abandoned this revision.
tbaeder added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153001/new/

https://reviews.llvm.org/D153001

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D153001: [clang][ThreadSafety] Add __builtin_instance_member (WIP)

2023-07-05 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153001/new/

https://reviews.llvm.org/D153001

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D153001: [clang][ThreadSafety] Add __builtin_instance_member (WIP)

2023-07-05 Thread Timm Bäder via Phabricator via cfe-commits
tbaeder added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153001/new/

https://reviews.llvm.org/D153001

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D153001: [clang][ThreadSafety] Add __builtin_instance_member (WIP)

2023-06-28 Thread Timm Bäder via Phabricator via cfe-commits
tbaeder added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153001/new/

https://reviews.llvm.org/D153001

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D153001: [clang][ThreadSafety] Add __builtin_instance_member (WIP)

2023-06-24 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment.

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()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153001/new/

https://reviews.llvm.org/D153001

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D153001: [clang][ThreadSafety] Add __builtin_instance_member (WIP)

2023-06-23 Thread Timm Bäder via Phabricator via cfe-commits
tbaeder added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153001/new/

https://reviews.llvm.org/D153001

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D153001: [clang][ThreadSafety] Add __builtin_instance_member (WIP)

2023-06-15 Thread Timm Bäder via Phabricator via cfe-commits
tbaeder created this revision.
tbaeder added reviewers: aaronpuchert, NoQ, aaron.ballman.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153001

Files:
  clang/include/clang/AST/EvaluatedExprVisitor.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/Analysis/ThreadSafetyCommon.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/Sema/warn-thread-safety-analysis.c

Index: clang/test/Sema/warn-thread-safety-analysis.c
===
--- clang/test/Sema/warn-thread-safety-analysis.c
+++ clang/test/Sema/warn-thread-safety-analysis.c
@@ -145,6 +145,24 @@
   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.counter++;
+  unlock_holder();
+
+  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}}
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -747,6 +747,9 @@
   Code = serialization::EXPR_UNARY_OPERATOR;
 }
 
+void ASTStmtWriter::VisitBuiltinInstanceMemberExpr(
+BuiltinInstanceMemberExpr *E) {}
+
 void ASTStmtWriter::VisitOffsetOfExpr(OffsetOfExpr *E) {
   VisitExpr(E);
   Record.push_back(E->getNumComponents());
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -714,6 +714,9 @@
 FPOptionsOverride::getFromOpaqueInt(Record.readInt()));
 }
 
+void ASTStmtReader::VisitBuiltinInstanceMemberExpr(
+BuiltinInstanceMemberExpr *E) {}
+
 void ASTStmtReader::VisitOffsetOfExpr(OffsetOfExpr *E) {
   VisitExpr(E);
   assert(E->getNumComponents() == Record.peekInt());
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -10957,6 +10957,12 @@
SubExpr.get());
 }
 
+template 
+ExprResult TreeTransform::TransformBuiltinInstanceMemberExpr(
+BuiltinInstanceMemberExpr *E) {
+  return ExprError();
+}
+
 template
 ExprResult
 TreeTransform::TransformOffsetOfExpr(OffsetOfExpr *E) {
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16700,6 +16700,19 @@
   Comps, Exprs, RParenLoc);
 }
 
+ExprResult Sema::ActOnBuiltinInstanceMember(const IdentifierInfo *MemberII) {
+  const auto *RD = dyn_cast(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,
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@