https://github.com/tbaederr updated
https://github.com/llvm/llvm-project/pull/67520
>From 55efd18bb177150a1fd170cb1535e225854967a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?=
Date: Thu, 20 Jun 2024 07:39:20 +0200
Subject: [PATCH] Warn on RequiresCapability attribute mismatch
-
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -2249,7 +2249,7 @@ void fooF1(Foo *f) EXCLUSIVE_LOCKS_REQUIRED(f->mu_) {
f
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -2282,6 +2308,9 @@ void
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContex
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -2249,7 +2249,7 @@ void fooF1(Foo *f) EXCLUSIVE_LOCKS_REQUIRED(f->mu_) {
f
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -2282,6 +2308,9 @@ void
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContex
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
aaronpuchert wrote:
We don't deduplicate "requires" attributes but check them directly. The resu
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
tbaederr wrote:
Meh, adding the attribute to the definition and declaration of a member
functio
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
https://github.com/tbaederr updated
https://github.com/llvm/llvm-project/pull/67520
>From 055f7
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
https://github.com/tbaederr updated
https://github.com/llvm/llvm-project/pull/67520
>From 055f7
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
https://github.com/tbaederr updated
https://github.com/llvm/llvm-project/pull/67520
>From 055f7
Timm =?utf-8?q?B=C3=A4der?= ,
Timm =?utf-8?q?B=C3=A4der?= ,
Timm =?utf-8?q?B=C3=A4der?= ,
Timm =?utf-8?q?B=C3=A4der?= ,
Timm =?utf-8?q?B=C3=A4der?= ,
Timm =?utf-8?q?B=C3=A4der?=
Message-ID:
In-Reply-To:
aaronpuchert wrote:
> Is there any sort of facility to deduplicate capabilities?
The exist
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
https://github.com/tbaederr updated
https://github.com/llvm/llvm-project/pull/67520
>From 1c117257921fc1c246fbb9f51e3c95
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
tbaederr wrote:
Hmm, so this is causing problems:
```c++
namespace DoubleLockBug {
class Foo {
public:
Mutex mu_;
int a GUARDED_BY(m
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -2263,6 +2265,28 @@ static bool neverReturns(const CFGBlock *B) {
return false;
}
+void ThreadSafetyAnalyzer::checkMismat
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -2263,6 +2265,28 @@ static bool neverReturns(const CFGBlock *B) {
return false;
}
+void ThreadSafetyAnalyzer::checkMismat
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -26,6 +26,7 @@ namespace clang {
class AnalysisDeclContext;
class FunctionDecl;
class NamedDecl;
+class Attr;
--
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -2282,6 +2306,9 @@ void
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
const NamedDecl *D = walker.getDecl();
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
kevmw wrote:
> This example from the failing test should warn I think
> [...]
> Since the definition of foo2 specifies attributes that aren't pres
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
tbaederr wrote:
This example from the failing test should warn I think:
```c++
class Foo {
public:
void foo1();
void foo2();
void foo3(Foo
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
tbaederr wrote:
Moved it back into `ThreadAnalysis.cpp`. I'm still only comparing the number of
args, but I'd like better diagnostics for this. I
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
https://github.com/tbaederr updated
https://github.com/llvm/llvm-project/pull/67520
>From 1c117257921fc1c246fbb9f51e3c95d6dc6d295e Mon Sep 17 00:
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -5794,6 +5794,30 @@ static void handleRequiresCapabilityAttr(Sema &S, Decl
*D,
RequiresCapabilityAttr(S.Context, AL, Args.data(), Args.size());
D->addAttr(RCA);
+
+ if (const auto *FD = dy
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -5794,6 +5794,30 @@ static void handleRequiresCapabilityAttr(Sema &S, Decl
*D,
RequiresCapabilityAttr(S.Context, AL, Args.data(), Args.size());
D->addAttr(RCA);
+
+ if (const auto *FD = dy
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -5794,6 +5794,30 @@ static void handleRequiresCapabilityAttr(Sema &S, Decl
*D,
RequiresCapabilityAttr(S.Context, AL, Args.data(), Args.size());
D->addAttr(RCA);
+
+ if (const auto *FD = dy
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
tbaederr wrote:
> So the semantics here are: if there is any `requires_capability` attribute on
> a function, it needs to exactly match the set of `requires_capability`
> attributes on every previous declaration? Or in
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -5794,6 +5794,30 @@ static void handleRequiresCapabilityAttr(Sema &S, Decl
*D,
RequiresCapabilityAttr(S.Context, AL, Args.data(), Args.size());
D->addAttr(RCA);
+
+ if (const auto *FD = dy
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
https://github.com/aaronpuchert commented:
So the semantics here are: if there is any `requires_capability` attribute on a
function, it needs to exactly match the set of `requires_capability` attributes
on every previou
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
@@ -5794,6 +5794,30 @@ static void handleRequiresCapabilityAttr(Sema &S, Decl
*D,
RequiresCapabilityAttr(S.Context, AL, Args.data(), Args.size());
D->addAttr(RCA);
+
+ if (const auto *FD = dy
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
https://github.com/aaronpuchert edited
https://github.com/llvm/llvm-project/pull/67520
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mai
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 9f3728d157
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
tbaederr wrote:
Updated the branch.
This now warns:
```c++
#define LOCKABLE__attribute__ ((lockable))
#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__
((exclusive_locks_required(__VA_ARGS__)))
struct L
Timm =?utf-8?q?Bäder?= ,
Timm =?utf-8?q?Bäder?=
Message-ID:
In-Reply-To:
https://github.com/tbaederr updated
https://github.com/llvm/llvm-project/pull/67520
>From 5f411735a5366499481c09a317aa170af44796f3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?=
Date: Thu, 20 Jun 2024 07:3
aaronpuchert wrote:
> > That might not be enough. A function might not be used (or even referenced)
> > in the TU that defines it, but only in other TUs. But it would certainly
> > catch a number of issues already.
>
> Right, though catching that ends up being pretty impossible. The most you
erichkeane wrote:
> That might not be enough. A function might not be used (or even referenced)
> in the TU that defines it, but only in other TUs. But it would certainly
> catch a number of issues already.
Right, though catching that ends up being pretty impossible. The most you could
do is
aaronpuchert wrote:
That might not be enough. A function might not be used (or even referenced) in
the TU that defines it, but only in other TUs. But it would certainly catch a
number of issues already.
https://github.com/llvm/llvm-project/pull/67520
___
erichkeane wrote:
Rather than this being "not added in the header file", should we just make this
one of the attributes that is disallowed after the thing has been 'referenced'?
Or is that a dumb suggestion here?
https://github.com/llvm/llvm-project/pull/67520
@@ -3978,7 +3978,9 @@ def warn_acquired_before : Warning<
def warn_acquired_before_after_cycle : Warning<
"cycle in acquired_before/after dependencies, starting with '%0'">,
InGroup, DefaultIgnore;
-
+def warn_attribute_mismatch : Warning<
+ "attribute mismatch between fun
@@ -2260,6 +2260,37 @@ static bool neverReturns(const CFGBlock *B) {
return false;
}
+template
+static SmallVector collectAttrArgs(const FunctionDecl *FD) {
+ SmallVector Args;
+ for (const AttrT *A : FD->specific_attrs()) {
+for (const Expr *E : A->args())
+ Arg
https://github.com/aaronpuchert edited
https://github.com/llvm/llvm-project/pull/67520
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/aaronpuchert commented:
This probably fits better in `Sema/SemaDeclAttr.cpp`. Maybe
`checkLockFunAttrCommon` is a good place. Add a test to
`clang/test/Sema/attr-capabilities.c{,pp}`.
https://github.com/llvm/llvm-project/pull/67520
__
aaronpuchert wrote:
> Maybe I misunderstood the code of this PR, but I thought it only checks
> definitions.
That's my understanding as well. I was just thinking about the usual
requirement of putting the attribute on the first declaration (like the
standard mandates it for `[[noreturn]]`). U
kevmw wrote:
> But as a rule of thumb, static analysis attributes always belong on the
> publicly visible declaration. Otherwise, the caller can't see them.
Yes, the theory is obvious and easy to understand, at least once you think of
it. Applying it in practice is where I made the experience
aaronpuchert wrote:
> > Yeah, it's a tricky question. On one hand there have been previous issues
> > like #42194 (which this would basically address), and it would definitely
> > improve consistency and prevent beginner's mistakes. On the other hand it
> > seems useful to allow adding attribu
kevmw wrote:
I'm the person who asked @tbaederr last year if this could be added for the
benefit of QEMU, and as I'm looking at it again now, I thought maybe I should
leave a comment here.
In QEMU, we currently handle the problem with a coding convention (public
functions get TSA attributes _
aaronpuchert wrote:
Doesn't this fit better in `Sema/SemaDeclAttr.cpp`? That's where we're checking
the attributes themselves and whether they make sense on a declaration. This
would seem like a good place to check against previous declarations. Though to
be fair, I don't know how late the lat
tbaederr wrote:
> I tried out this WIP on our codebase after we ran into a problem where an
> annotation was added to a definition rather than a declaration, leading to a
> (silently) missed capability check.
>
> This addition seems to work fine, but it triggers warnings when annotations
> ar
theuni wrote:
I tried out this WIP on our codebase after we ran into a problem where an
annotation was added to a definition rather than a declaration, leading to a
(silently) missed capability check.
This addition seems to work fine, but it triggers warnings when annotations are
added to bot
tbaederr wrote:
I've resorted to do this in `ThreadSafety.cpp` now, since all other places
where we merge function definition and declaration happen before we have the
late-parsed attributes available.
>From a diagnostic POV, this seems quite complicated since we have N
>declarations and need
https://github.com/tbaederr updated
https://github.com/llvm/llvm-project/pull/67520
>From 800ce47e967593ec149e0187abf6d2cb3ee1b1b5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?=
Date: Thu, 20 Jun 2024 07:39:20 +0200
Subject: [PATCH] Warn on RequiresCapability attribute mismatch
-
AaronBallman wrote:
> @AaronBallman Is the prototype of the implementation here at least sound? Or
> should this go somewhere completely different? Maybe in one of the TSA source
> files?
This is a semantic concern rather than a syntactic one, so I would expect this
to happen in Sema rather t
tbaederr wrote:
@AaronBallman Is the prototype of the implementation here at least sound? Or
should this go somewhere completely different? Maybe in one of the TSA source
files?
https://github.com/llvm/llvm-project/pull/67520
___
cfe-commits mailing
aaronpuchert wrote:
Yeah, it's a tricky question. On one hand there have been previous issues like
#42194 (which this would basically address), and it would definitely improve
consistency and prevent beginner's mistakes. On the other hand it seems useful
to allow adding attributes e.g. to thir
AaronBallman wrote:
> There's a problem with this attribute where the declaration in a header file
> doesn't have the attribute, but the definition in the source file has. As a
> result, the attribute doesn't take effect when just the header file is
> included.
Errr, that's the behavior of ba
tbaederr wrote:
Ping
https://github.com/llvm/llvm-project/pull/67520
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/tbaederr converted_to_draft
https://github.com/llvm/llvm-project/pull/67520
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
llvmbot wrote:
@llvm/pr-subscribers-clang
Changes
There's a problem with this attribute where the declaration in a header file
doesn't have the attribute, but the definition in the source file has. As a
result, the attribute doesn't take effect when just the header file is included.
This
https://github.com/tbaederr created
https://github.com/llvm/llvm-project/pull/67520
There's a problem with this attribute where the declaration in a header file
doesn't have the attribute, but the definition in the source file has. As a
result, the attribute doesn't take effect when just the h
57 matches
Mail list logo
