https://github.com/aaronpuchert commented:
I've thought a bit more about what inheritance means for this and I think it
makes our job easier. We can probably collect capabilities on the first
declaration and then just check that the non-inherited attributes on subsequent
declarations are alrea
@@ -2263,6 +2265,27 @@ static bool neverReturns(const CFGBlock *B) {
return false;
}
+void ThreadSafetyAnalyzer::checkMismatchedFunctionAttrs(const NamedDecl *ND) {
+ auto collectCapabilities = [&](const Decl *D) {
+CapExprSet Caps;
+for (const auto *A : D->specific
@@ -2263,6 +2265,27 @@ static bool neverReturns(const CFGBlock *B) {
return false;
}
+void ThreadSafetyAnalyzer::checkMismatchedFunctionAttrs(const NamedDecl *ND) {
+ auto collectCapabilities = [&](const Decl *D) {
+CapExprSet Caps;
+for (const auto *A : D->specific
aaronpuchert wrote:
Doesn't eaa0a21d21962280dc2c03a09152510f6162a576 already fix this? If
`CMAKE_SYSTEM_NAME` is `Linux`, it can probably not be `AIX` at the same time,
right? See also the discussion on #116556.
Do you mind if I revert this?
https://github.com/llvm/llvm-project/pull/117342
__
@@ -48,11 +48,13 @@ add_clang_library(clang-cpp
${_OBJECTS}
LINK_LIBS
${_DEPS})
+# AIX linker does not support version script
+if (NOT ${CMAKE_SYSTEM_NAME} MATCHES "AIX")
+ configure_file(simple_version_script.map.in simple
aaronpuchert wrote:
We had a discussion on https://reviews.llvm.org/D52395 that might be relevant
here. To quote @delesley:
> When you pass an object to a function, either by pointer or by reference, no
> actual load from memory has yet occurred. Thus, there is a real risk of false
> positiv
aaronpuchert wrote:
Something like `READ_ONCE` might be supported differently: suppose there is
actually a read, i.e. an lvalue-to-rvalue cast. We check those here:
```c++
void BuildLockset::VisitCastExpr(const CastExpr *CE) {
if (CE->getCastKind() != CK_LValueToRValue)
return;
checkAcce
aaronpuchert wrote:
> The equivalent of passing a `pt_guarded_by` variable by value doesn't seem to
> warn.
This inconsistency is probably my largest concern. If you have
```c
int x GUARDED_BY(mu);
int *p PT_GUARDED_BY(mu);
```
then `&x` should basically behave like `p`, and `x` like `*p`. But
@@ -2316,6 +2337,49 @@ void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
POK_ReturnPointer);
}
+
+ if (!checkRecordTypeForScopedCapability(ReturnType))
+return;
+
+ if (const
@@ -1799,11 +1799,11 @@ class ThreadSafetyReporter : public
clang::threadSafety::ThreadSafetyHandler {
: getNotes();
}
- OptionalNotes makeManagedMismatchNoteForParam(SourceLocation DeclLoc) {
+ OptionalNotes makeManagedMismatchNote(SourceLocation DeclLoc,
https://github.com/aaronpuchert commented:
Looks good to me, just some small nitpicks.
https://github.com/llvm/llvm-project/pull/131831
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -2316,6 +2337,49 @@ void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
POK_ReturnPointer);
}
+
+ if (!checkRecordTypeForScopedCapability(ReturnType))
+return;
+
+ if (const
@@ -2041,15 +2042,16 @@ void BuildLockset::handleCall(const Expr *Exp, const
NamedDecl *D,
if (!a.has_value()) {
Analyzer->Handler.handleExpectFewerUnderlyingMutexes(
Exp->getExprLoc(), D->getLocation(), Scope->toString(),
- b.value
@@ -3566,6 +3565,38 @@ void releaseMemberCall() {
ReleasableMutexLock lock(&obj.mu);
releaseMember(obj, lock);
}
+#ifdef __cpp_guaranteed_copy_elision
aaronpuchert wrote:
Add blank lines around `#ifdef` and `#endif` for readability.
https://github.com/llv
@@ -3566,6 +3565,38 @@ void releaseMemberCall() {
ReleasableMutexLock lock(&obj.mu);
releaseMember(obj, lock);
}
+#ifdef __cpp_guaranteed_copy_elision
+// expected-note@+2{{mutex acquired here}}
+// expected-note@+1{{see attribute on function here}}
+RelockableScope returnU
@@ -3566,6 +3565,38 @@ void releaseMemberCall() {
ReleasableMutexLock lock(&obj.mu);
releaseMember(obj, lock);
}
+#ifdef __cpp_guaranteed_copy_elision
+// expected-note@+2{{mutex acquired here}}
+// expected-note@+1{{see attribute on function here}}
+RelockableScope returnU
https://github.com/aaronpuchert edited
https://github.com/llvm/llvm-project/pull/131831
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaronpuchert wrote:
The tests would produce without the change:
```
error: 'expected-error' diagnostics seen but not expected:
File clang/test/SemaCXX/type-traits.cpp Line 2676: calling a private
constructor of class 'AllPrivate'
File clang/test/SemaCXX/type-traits.cpp Line 2833: 'operator='
https://github.com/aaronpuchert updated
https://github.com/llvm/llvm-project/pull/135390
>From a1eda3b0d5b54ffc3d3ad4288d1d7685f6486143 Mon Sep 17 00:00:00 2001
From: Aaron Puchert
Date: Fri, 11 Apr 2025 13:47:02 +0200
Subject: [PATCH] Suppress errors from well-formed-testing type traits in
SF
https://github.com/aaronpuchert edited
https://github.com/llvm/llvm-project/pull/135390
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -163,15 +184,15 @@ using FactID = unsigned short;
/// the analysis of a single routine.
class FactManager {
private:
- std::vector> Facts;
+ std::vector> Facts;
aaronpuchert wrote:
The FactEntries themselves should never be replaced, they are immutable. W
@@ -271,26 +271,32 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::
@@ -114,31 +112,39 @@ class FactEntry : public CapabilityExpr {
};
private:
- const FactEntryKind Kind : 8;
+ const FactEntryKind Kind : 4;
/// Exclusive or shared.
- LockKind LKind : 8;
+ const LockKind LKind : 4;
+
+ /// How it was acquired.
+ const SourceKind S
@@ -81,26 +81,25 @@ static bool isCalleeArrow(const Expr *E) {
return ME ? ME->isArrow() : false;
}
-static StringRef ClassifyDiagnostic(const CapabilityAttr *A) {
- return A->getName();
-}
-
-static StringRef ClassifyDiagnostic(QualType VDT) {
+static CapabilityExpr makeCa
https://github.com/aaronpuchert edited
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -388,7 +395,7 @@ class SExprBuilder {
til::LiteralPtr *createVariable(const VarDecl *VD);
// Create placeholder for this: we don't know the VarDecl on construction
yet.
- std::pair
+ std::pair
aaronpuchert wrote:
Nice idea, but I'm not sure if it's
https://github.com/aaronpuchert closed
https://github.com/llvm/llvm-project/pull/137477
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/aaronpuchert updated
https://github.com/llvm/llvm-project/pull/135561
>From 372bfceceec7ba618d7651559f1071baacaf2fcc Mon Sep 17 00:00:00 2001
From: Aaron Puchert
Date: Sun, 13 Apr 2025 22:28:23 +0200
Subject: [PATCH] Merge similar Clang Thread Safety attributes
Some of the o
https://github.com/aaronpuchert created
https://github.com/llvm/llvm-project/pull/135561
Some of the old lock-based and new capability-based spellings behave basically
in the same way, so merging them simplifies the code significantly.
There are two minor functional changes: we only warn (inst
aaronpuchert wrote:
This seems to have fixed #60112. Thanks!
https://github.com/llvm/llvm-project/pull/89154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaronpuchert wrote:
> (It should be noted that the standard doesn't always base this on the
> immediate context being well-formed: for `std::common_type` it's based on
> whether some expression "denotes a valid type." But I assume that's an
> editorial issue and means the same thing.)
Filed c
https://github.com/aaronpuchert updated
https://github.com/llvm/llvm-project/pull/135390
>From 20219354f6a5a59cb36554fb26c5864b5d9be74e Mon Sep 17 00:00:00 2001
From: Aaron Puchert
Date: Fri, 11 Apr 2025 13:47:02 +0200
Subject: [PATCH] Suppress errors from well-formed-testing type traits in
SF
https://github.com/aaronpuchert updated
https://github.com/llvm/llvm-project/pull/135390
>From ef1b40c0247205f8147fe6050c1303628833c247 Mon Sep 17 00:00:00 2001
From: Aaron Puchert
Date: Fri, 11 Apr 2025 13:47:02 +0200
Subject: [PATCH] Suppress errors from well-formed-testing type traits in
SF
@@ -0,0 +1,34 @@
+//===--- UseEnumClassCheck.cpp - clang-tidy
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apa
@@ -0,0 +1,34 @@
+//===--- UseEnumClassCheck.cpp - clang-tidy
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apa
@@ -0,0 +1,34 @@
+//===--- UseEnumClassCheck.cpp - clang-tidy
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apa
@@ -12240,16 +12240,16 @@ class Sema final : public SemaBase {
bool PrevLastDiagnosticIgnored;
public:
-explicit SFINAETrap(Sema &SemaRef, bool AccessCheckingSFINAE = false)
+explicit SFINAETrap(Sema &SemaRef, bool TestWellformedSFINAE = false)
a
@@ -12240,16 +12240,16 @@ class Sema final : public SemaBase {
bool PrevLastDiagnosticIgnored;
public:
-explicit SFINAETrap(Sema &SemaRef, bool AccessCheckingSFINAE = false)
+explicit SFINAETrap(Sema &SemaRef, bool TestWellformedSFINAE = false)
a
https://github.com/aaronpuchert commented:
I think the biggest issue is that removing `const` from `FactEntry` does not
work. You'll have to undo all those changes and instead create a new
`FactEntry` for every lock/unlock.
https://github.com/llvm/llvm-project/pull/137133
_
@@ -1831,15 +1852,15 @@ void BuildLockset::handleCall(const Expr *Exp, const
NamedDecl *D,
assert(!Self);
const auto *TagT = Exp->getType()->getAs();
if (D->hasAttrs() && TagT && Exp->isPRValue()) {
- std::pair Placeholder =
- Analyzer->SxBuilder.crea
@@ -434,6 +434,16 @@ class can be used as a capability. The string argument
specifies the kind of
capability in error messages, e.g. ``"mutex"``. See the ``Container`` example
given above, or the ``Mutex`` class in :ref:`mutexheader`.
+REENTRANT
aaronpucher
@@ -434,6 +434,16 @@ class can be used as a capability. The string argument
specifies the kind of
capability in error messages, e.g. ``"mutex"``. See the ``Container`` example
given above, or the ``Mutex`` class in :ref:`mutexheader`.
+REENTRANT
+-
+
+``REENTRANT``
@@ -6708,15 +6708,15 @@ int testAdoptShared() {
} // namespace ReturnScopedLockable
-#endif
+#endif // __cpp_guaranteed_copy_elision
aaronpuchert wrote:
These changes are fine, but please just commit them separately. (No review
required.)
https://github.co
@@ -271,26 +271,32 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::
@@ -103,6 +103,23 @@ static StringRef ClassifyDiagnostic(QualType VDT) {
return "mutex";
}
+static unsigned getCapabilityExprFlags(QualType VDT) {
+ unsigned Flags = 0;
+
+ if (const auto *RT = VDT->getAs()) {
+if (const auto *RD = RT->getDecl())
+ if (RD->hasAttr
@@ -163,15 +184,15 @@ using FactID = unsigned short;
/// the analysis of a single routine.
class FactManager {
private:
- std::vector> Facts;
+ std::vector> Facts;
aaronpuchert wrote:
This does not work, `FactEntry` has to remain `const`. See Delesley's comm
@@ -271,26 +271,32 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::
https://github.com/aaronpuchert edited
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaronpuchert wrote:
I think it's obscure enough to not need to be mentioned. Since I went with the
less intrusive variant of downgrading an error to warning, it should not break
anyone's code.
I'd be open to add `ErrorDiag` on all thread safety attributes, but I'd do so
in a separate change i
https://github.com/aaronpuchert updated
https://github.com/llvm/llvm-project/pull/135390
>From af21e7bb441c13714f299600966bff28befe5191 Mon Sep 17 00:00:00 2001
From: Aaron Puchert
Date: Fri, 11 Apr 2025 13:47:02 +0200
Subject: [PATCH] Suppress errors from well-formed-testing type traits in
SF
https://github.com/aaronpuchert closed
https://github.com/llvm/llvm-project/pull/135561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -168,6 +197,8 @@ class FactManager {
public:
FactID newFact(std::unique_ptr Entry) {
Facts.push_back(std::move(Entry));
+assert(Facts.size() - 1 <= std::numeric_limits::max() &&
aaronpuchert wrote:
```suggestion
assert(Facts.size() - 1 <= std:
@@ -235,6 +266,20 @@ class FactSet {
return false;
}
+ std::optional replaceLock(FactManager &FM, iterator It,
+std::unique_ptr Entry) {
+if (It == end())
+ return std::nullopt;
+FactID F = FM.newFact(std::move(Entry));
+
@@ -114,31 +112,39 @@ class FactEntry : public CapabilityExpr {
};
private:
- const FactEntryKind Kind : 8;
+ const FactEntryKind Kind : 4;
/// Exclusive or shared.
- LockKind LKind : 8;
+ const LockKind LKind : 4;
+
+ /// How it was acquired.
+ const SourceKind S
https://github.com/aaronpuchert commented:
Two more things come to mind:
* Consider adding a check to `SemaDeclAttr.cpp` that the new attribute is
always accompanied by `capability` or `lockable`. Although I wonder whether
that's too early. I'm not sure if we can see the other attributes alread
@@ -114,31 +112,39 @@ class FactEntry : public CapabilityExpr {
};
private:
- const FactEntryKind Kind : 8;
+ const FactEntryKind Kind : 4;
/// Exclusive or shared.
- LockKind LKind : 8;
+ const LockKind LKind : 4;
+
+ /// How it was acquired.
+ const SourceKind S
https://github.com/aaronpuchert created
https://github.com/llvm/llvm-project/pull/137477
Support for attribute parameter packs was added some time ago in commit
ead1690d31f815c00fdd2bc23db4766191bbeabc. But template substitution didn't
expand the packs yet. For now expansion can only happen wi
aaronpuchert wrote:
Ping. Or is this not interesting enough for a review?
https://github.com/llvm/llvm-project/pull/116266
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/aaronpuchert approved this pull request.
Looks good to me, and thanks for the contribution!
> Note, I think for now it might be safer to not enable by default yet, but
> I've made a note (and hinted at in changelog) that we're planning to default
> enable in future, so that
aaronpuchert wrote:
Good question. Which AST nodes could we visit here? Let's see some examples of
initializing function pointers/references:
```c++
void f();
void (*fp)() = f;
void (*fp2)() = &f;
void (*fp3)() = fp;
void (&fr)() = f;
```
The (simplified) AST:
```
TranslationUnitDecl 0x55edc6
aaronpuchert wrote:
> One thought --- you could consider an attribute that could be put on pointer
> arguments to functions that says "yes, I dereference this and read or write
> it".
GCC [has such an
attribute](https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-access-f
@@ -2294,6 +2309,11 @@ void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
FunctionExitFSet, RetVal,
ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
POK_ReturnByRef);
+ } else if (ReturnType->isPointerType()) {
@@ -142,9 +145,19 @@ int main(void) {
(void)(get_value(b_) == 1);
mutex_unlock(foo_.mu_);
+ a_ = 0; // expected-warning{{writing variable 'a_' requires holding mutex
'foo_.mu_'}}
+ __WRITE_ONCE(a_, 0); // expected-warning{{writing variable 'a_' requires
holding mutex '
@@ -4944,6 +4949,14 @@ class Foo {
(*datap2_)[0] = 0;// expected-warning {{reading the value pointed to
by 'datap2_' requires holding mutex 'mu_'}}
data_(); // expected-warning {{reading variable 'data_'
requires holding mutex 'mu_'}}
+
+// Calls
@@ -4955,13 +4968,18 @@ class Foo {
//showDataCell(*datap2_); // xpected-warning {{reading the value pointed
to by 'datap2_' requires holding mutex 'mu_'}}
int a = data_[0]; // expected-warning {{reading variable 'data_'
requires holding mutex 'mu_'}}
+
+(v
@@ -528,6 +529,9 @@ for a period of time, after which they are migrated into
the standard analysis.
* ``-Wthread-safety-beta``: New features. Off by default.
+ + ``-Wthread-safety-pointer``: Checks when passing or returning pointers to
+guarded variables, or pointers
@@ -1795,9 +1795,22 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet
&FSet, const Expr *Exp,
// Pass by reference warnings are under a different flag.
aaronpuchert wrote:
"reference/pointer"
https://github.com/llvm/llvm-project/pull/127396
___
https://github.com/aaronpuchert edited
https://github.com/llvm/llvm-project/pull/127396
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -1780,6 +1782,14 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet
&FSet, const Expr *Exp,
Exp = CE->getSubExpr();
continue;
}
+if (const auto *UO = dyn_cast(Exp)) {
+ if (UO->getOpcode() == UO_AddrOf) {
+// Pointer access via pointe
@@ -515,7 +515,8 @@ Warning flags
+ ``-Wthread-safety-analysis``: The core analysis.
+ ``-Wthread-safety-precise``: Requires that mutex expressions match
precisely.
This warning can be disabled for code which has a lot of aliases.
- + ``-Wthread-safety-reference``:
https://github.com/aaronpuchert commented:
I think this looks very good! I just have some minor remarks.
Thanks to @aoates for trying this out, this is always appreciated!
And sorry for the delay.
https://github.com/llvm/llvm-project/pull/127396
___
@@ -2041,15 +2042,16 @@ void BuildLockset::handleCall(const Expr *Exp, const
NamedDecl *D,
if (!a.has_value()) {
Analyzer->Handler.handleExpectFewerUnderlyingMutexes(
Exp->getExprLoc(), D->getLocation(), Scope->toString(),
- b.value
aaronpuchert wrote:
Maybe the test needs to be relaxed a bit because of stack layout differences in
other OS targets? Although I'm not sure why they're different. See
https://lab.llvm.org/buildbot/#/builders/186/builds/7896:
```
TEST 'AddressSanitizer-arm-android ::
TestCa
@@ -0,0 +1,34 @@
+//===--- UseEnumClassCheck.cpp - clang-tidy
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apa
@@ -0,0 +1,34 @@
+//===--- UseEnumClassCheck.cpp - clang-tidy
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apa
@@ -1011,6 +979,30 @@ void SExprBuilder::exitCFG(const CFGBlock *Last) {
IncompleteArgs.clear();
}
+static CapabilityExpr makeCapabilityExpr(const til::SExpr *E, QualType VDT,
+ bool Neg) {
+ // We need to look at the declaration of t
https://github.com/aaronpuchert updated
https://github.com/llvm/llvm-project/pull/135390
>From d8bc5ebd7976d25e800987b3c95057364dc1c07c Mon Sep 17 00:00:00 2001
From: Aaron Puchert
Date: Fri, 11 Apr 2025 13:47:02 +0200
Subject: [PATCH] Suppress errors from well-formed-testing type traits in
SF
aaronpuchert wrote:
Could also add this under "Bug Fixes to C++ Support". The entries about type
traits [don't seem to be consistently
categorized](https://releases.llvm.org/19.1.0/tools/clang/docs/ReleaseNotes.html#bug-fixes-in-this-version),
but I think this is all C++-only.
https://github.
https://github.com/aaronpuchert closed
https://github.com/llvm/llvm-project/pull/135390
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
301 - 379 of 379 matches
Mail list logo
