[clang] [clang][ThreadSafety] Revert stricter typing on trylock attributes (PR #97293)
https://github.com/dmcardle created
https://github.com/llvm/llvm-project/pull/97293
This PR reverts #95290 and the one-liner followup PR #96494.
I received some substantial feedback on #95290, which I plan to address in a
future PR.
I've also received feedback that because the change emits errors where they
were not emitted before, we should at least have a flag to disable the stricter
warnings.
>From 5202d53b5e32c79b4dec723f80e4276bee77a0e6 Mon Sep 17 00:00:00 2001
From: Dan McArdle
Date: Mon, 1 Jul 2024 09:32:42 -0400
Subject: [PATCH 1/2] Revert "[clang][ThreadSafety] Fix code block syntax in
ThreadSafetyAnalysis.rst (#96494)"
This reverts commit 34026207c87116bd8e7fb0a464ea8db947f8239a.
---
clang/docs/ThreadSafetyAnalysis.rst | 1 -
1 file changed, 1 deletion(-)
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index 1513719caa464..0ecbebe7a692f 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -464,7 +464,6 @@ on success and ``LockNotAcquired`` on failure, the analysis
may fail to detect
access to guarded data without holding the mutex because they are both
non-zero.
.. code-block:: c++
-
// *** Beware: this code demonstrates incorrect usage. ***
enum TrylockResult { LockAcquired = 1, LockNotAcquired = 2 };
>From 783f56be1bacdce768ebc11e8566171a9693becf Mon Sep 17 00:00:00 2001
From: Dan McArdle
Date: Mon, 1 Jul 2024 09:33:03 -0400
Subject: [PATCH 2/2] Revert "[clang][ThreadSafety] Check trylock function
success and return types (#95290)"
This reverts commit c1bde0a2cb640b3607e9568b9a57b292e1f82666.
---
clang/docs/ReleaseNotes.rst | 10 --
clang/docs/ThreadSafetyAnalysis.rst | 52 +---
.../clang/Basic/DiagnosticSemaKinds.td| 23 +---
clang/include/clang/Sema/ParsedAttr.h | 2 -
clang/lib/Analysis/ThreadSafety.cpp | 82 +---
clang/lib/Sema/SemaDeclAttr.cpp | 34 ++---
clang/test/Sema/attr-capabilities.c | 12 +-
.../SemaCXX/warn-thread-safety-analysis.cpp | 123 +-
.../SemaCXX/warn-thread-safety-parsing.cpp| 107 +++
clang/unittests/AST/ASTImporterTest.cpp | 6 +-
10 files changed, 82 insertions(+), 369 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b7a2d97f00087..c7bb25d611971 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -71,11 +71,6 @@ C++ Specific Potentially Breaking Changes
To fix this, update libstdc++ to version 14.1.1 or greater.
-- Clang now emits errors when Thread Safety Analysis trylock attributes are
- applied to functions or methods with incompatible return values, such as
- constructors, destructors, and void-returning functions. This only affects
the
- ``TRY_ACQUIRE`` and ``TRY_ACQUIRE_SHARED`` attributes (and any synonyms).
-
ABI Changes in This Version
---
- Fixed Microsoft name mangling of implicitly defined variables used for thread
@@ -751,11 +746,6 @@ Bug Fixes in This Version
- Fixed `static_cast` to array of unknown bound. Fixes (#GH62863).
-- Clang's Thread Safety Analysis now evaluates trylock success arguments of
enum
- types rather than silently defaulting to false. This fixes a class of false
- negatives where the analysis failed to detect unchecked access to guarded
- data.
-
Bug Fixes to Compiler Builtins
^^
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index 0ecbebe7a692f..dcde0c706c704 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -420,17 +420,10 @@ TRY_ACQUIRE(, ...), TRY_ACQUIRE_SHARED(, ...)
*Previously:* ``EXCLUSIVE_TRYLOCK_FUNCTION``, ``SHARED_TRYLOCK_FUNCTION``
These are attributes on a function or method that tries to acquire the given
-capability, and returns a boolean, integer, or pointer value indicating success
-or failure.
-
-The attribute's first argument defines whether a zero or non-zero return value
-indicates success. Syntactically, it accepts ``NULL`` or ``nullptr``, ``bool``
-and ``int`` literals, as well as enumerator values. *The analysis only cares
-whether this success value is zero or non-zero.* This leads to some subtle
-consequences, discussed in the next section.
-
-The remaining arguments are interpreted in the same way as ``ACQUIRE``. See
-:ref:`mutexheader`, below, for example uses.
+capability, and returns a boolean value indicating success or failure.
+The first argument must be ``true`` or ``false``, to specify which return value
+indicates success, and the remaining arguments are interpreted in the same way
+as ``ACQUIRE``. See :ref:`mutexheader`, below, for example uses.
Because the analysis doesn't support conditional locking, a capability is
treated as acquired after the first branch on the return value of a try-acquire
@@ -452,43 +4
[clang] [clang][ThreadSafety] Check trylock function success and return types (PR #95290)
@@ -1359,17 +1361,18 @@ void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet
&Mtxs, AttrType *Attr,
const Expr *Exp, const NamedDecl *D,
const CFGBlock *PredBlock,
const CFGBlock *CurrBlock,
- Expr *BrE, bool Neg) {
- // Find out which branch has the lock
- bool branch = false;
- if (const auto *BLE = dyn_cast_or_null(BrE))
-branch = BLE->getValue();
- else if (const auto *ILE = dyn_cast_or_null(BrE))
-branch = ILE->getValue().getBoolValue();
dmcardle wrote:
Thanks for the feedback, @aaronpuchert! I've reverted the change and I'll work
through these comments for a reland.
My instinct is to explicitly disallow enumerator success expressions.
Currently, the analysis can produce both false negatives and false positives
when enumerator success expressions are used:
. To my knowledge, enumerators were never
actually supported in success expressions, so in some sense this will only
break code that was already broken. However, I do want to be sensitive to
downstream breakage based on my experience with this PR!
One suggestion was to add a flag that disables any new errors, the goal being
to enabling large code bases to incrementally adapt to the new behavior. I'm
just not sure how this flag would actually work.
* If we suppress any new type errors, but otherwise apply the attribute, the
analysis is now off the rails, operating on unexpected inputs.
* If we simply ignore any attributes that would cause an error to be emitted,
now the analysis can produce different kinds of incorrect results, e.g. it
might complain that you're unlocking a lock that was never acquired.
* I suppose we could mimic the current/incorrect behavior when the flag is
present. Is it normally expected to keep incorrect behavior around for
compatibility?
@asmok-g, do you have any thoughts? How are breaking changes such as this
typically handled?
https://github.com/llvm/llvm-project/pull/95290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Warn when constructor is trylock (PR #95290)
https://github.com/dmcardle created
https://github.com/llvm/llvm-project/pull/95290
With this change, Clang will generate a warning when a constructor is annotated
as a trylock function.
Issue #92408
>From f717082e700645f1fa6ca738a540aef20c2ba015 Mon Sep 17 00:00:00 2001
From: Dan McArdle
Date: Wed, 12 Jun 2024 14:57:49 -0400
Subject: [PATCH] [clang][ThreadSafety] Warn when constructor is trylock
With this change, Clang will generate a warning when a constructor is
annotated as a trylock function.
Issue #92408
---
clang/lib/Sema/SemaDeclAttr.cpp | 6 ++
clang/test/SemaCXX/warn-thread-safety-parsing.cpp | 7 +++
2 files changed, 13 insertions(+)
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index ce6b5b1ff6f93..373f6a591fd09 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -605,6 +605,12 @@ static void handleAllocSizeAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
static bool checkTryLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
SmallVectorImpl &Args) {
+ if (isa(D)) {
+S.Diag(AL.getLoc(), diag::warn_attribute_wrong_decl_type)
+<< AL << AL.isRegularKeywordAttribute() << ExpectedFunction;
+return false;
+ }
+
if (!AL.checkAtLeastNumArgs(S, 1))
return false;
diff --git a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
index 0c5b0cc85897b..e3e0bd31e8067 100644
--- a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -747,6 +747,13 @@ class EtfFoo {
// expected-warning {{'exclusive_trylock_function' attribute without
capability arguments refers to 'this', but 'EtfFoo' isn't annotated with
'capability' or 'scoped_lockable' attribute}}
};
+// It does not make sense to annotate a constructor as an exclusive trylock
+// function. See .
+class EtfConstructor {
+ EtfConstructor() EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
+ // expected-warning {{'exclusive_trylock_function' attribute only applies to
functions}}
+};
+
class EXCLUSIVE_TRYLOCK_FUNCTION(1) EtfTestClass { // \
// expected-warning {{'exclusive_trylock_function' attribute only applies to
functions}}
};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Warn when constructor is trylock (PR #95290)
https://github.com/dmcardle updated
https://github.com/llvm/llvm-project/pull/95290
>From 9a9bf80b0fc51dd4ef660e2a2fe625af26e85c2a Mon Sep 17 00:00:00 2001
From: Dan McArdle
Date: Wed, 12 Jun 2024 14:57:49 -0400
Subject: [PATCH] [clang][ThreadSafety] Warn when constructor is trylock
With this change, Clang will generate a warning when a constructor is
annotated as a trylock function.
Issue #92408
---
clang/lib/Sema/SemaDeclAttr.cpp | 6 ++
clang/test/SemaCXX/warn-thread-safety-parsing.cpp | 7 +++
2 files changed, 13 insertions(+)
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index ce6b5b1ff6f93..373f6a591fd09 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -605,6 +605,12 @@ static void handleAllocSizeAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
static bool checkTryLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
SmallVectorImpl &Args) {
+ if (isa(D)) {
+S.Diag(AL.getLoc(), diag::warn_attribute_wrong_decl_type)
+<< AL << AL.isRegularKeywordAttribute() << ExpectedFunction;
+return false;
+ }
+
if (!AL.checkAtLeastNumArgs(S, 1))
return false;
diff --git a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
index 1626e8375892a..c9a2ad498ff24 100644
--- a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -747,6 +747,13 @@ class EtfFoo {
// expected-warning {{'exclusive_trylock_function' attribute without
capability arguments refers to 'this', but 'EtfFoo' isn't annotated with
'capability' or 'scoped_lockable' attribute}}
};
+// It does not make sense to annotate a constructor as an exclusive trylock
+// function. See .
+class EtfConstructor {
+ EtfConstructor() EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
+ // expected-warning {{'exclusive_trylock_function' attribute only applies to
functions}}
+};
+
class EXCLUSIVE_TRYLOCK_FUNCTION(1) EtfTestClass { // \
// expected-warning {{'exclusive_trylock_function' attribute only applies to
functions}}
};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Warn when constructor is trylock (PR #95290)
https://github.com/dmcardle ready_for_review https://github.com/llvm/llvm-project/pull/95290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Warn when constructor is trylock (PR #95290)
dmcardle wrote: @AaronBallman, please take a look! What do you think of this approach? I suppose it might be even better to enforce that the function's return type is `bool`, which would align with the [ThreadSafetyAnalysis documentation](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#try-acquire-bool-try-acquire-shared-bool). https://github.com/llvm/llvm-project/pull/95290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Warn when constructor is trylock (PR #95290)
dmcardle wrote: Thanks, @AaronBallman! I tried strictly enforcing a `bool` return type, but I ran into some interesting test failures. * A handful of functions in the parsing tests return `void` and yet are annotated as trylock functions. I don't believe this is a valid use case. What does `EXCLUSIVE_TRYLOCK_FUNCTION` mean when it's impossible to return a value? I think the solution is to just change `void` to `bool` for trylock functions in tests. https://github.com/llvm/llvm-project/blob/06aa078d68380bc775f0a903204fe330d50f4f1f/clang/test/SemaCXX/warn-thread-safety-parsing.cpp#L725 * This next failure is tricker... I was really surprised to see a trylock method that returns a pointer! Confusingly, the "success" parameter to the attribute is `1`. I think the semantics work out so that returning a non-`nullptr` value indicates successful mutex acquisition. I'm not quite sure what to do here. Maybe we should be lenient and only enforce that the return type is boolean, integer, or pointer? https://github.com/llvm/llvm-project/blob/06aa078d68380bc775f0a903204fe330d50f4f1f/clang/test/SemaCXX/warn-thread-safety-analysis.cpp#L3661 https://github.com/llvm/llvm-project/pull/95290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Warn when constructor is trylock (PR #95290)
https://github.com/dmcardle updated
https://github.com/llvm/llvm-project/pull/95290
>From e53ddc9e9ce0ddd8b5dfd5dfd4c8654afe643ce2 Mon Sep 17 00:00:00 2001
From: Dan McArdle
Date: Mon, 17 Jun 2024 17:53:30 -0400
Subject: [PATCH] [clang][ThreadSafety] Enforce trylock function return type
With this change, Clang will generate an error when functions or methods
with disallowed return types are annotated as trylock functions. The
only allowed return types are boolean, integer, and pointer types.
This effect is that annotating a constructor, destructor, void-returning
function, etc. will fail to compile, where it silently passed before.
Issue #92408
---
clang/docs/ThreadSafetyAnalysis.rst | 12 --
.../clang/Basic/DiagnosticSemaKinds.td| 3 +-
clang/include/clang/Sema/ParsedAttr.h | 1 +
clang/lib/Analysis/ThreadSafety.cpp | 3 ++
clang/lib/Sema/SemaDeclAttr.cpp | 21 +-
clang/test/Sema/attr-capabilities.c | 12 +++---
.../SemaCXX/warn-thread-safety-analysis.cpp | 4 +-
.../SemaCXX/warn-thread-safety-parsing.cpp| 40 ++-
clang/unittests/AST/ASTImporterTest.cpp | 6 +--
9 files changed, 76 insertions(+), 26 deletions(-)
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index dcde0c706c704..999271320793b 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -420,10 +420,14 @@ TRY_ACQUIRE(, ...), TRY_ACQUIRE_SHARED(, ...)
*Previously:* ``EXCLUSIVE_TRYLOCK_FUNCTION``, ``SHARED_TRYLOCK_FUNCTION``
These are attributes on a function or method that tries to acquire the given
-capability, and returns a boolean value indicating success or failure.
-The first argument must be ``true`` or ``false``, to specify which return value
-indicates success, and the remaining arguments are interpreted in the same way
-as ``ACQUIRE``. See :ref:`mutexheader`, below, for example uses.
+capability, and returns a boolean, integer, or pointer value indicating success
+or failure.
+
+The attribute's first argument must be ``true`` or ``false``, to specify which
+return value indicates success. Integer literals are also accepted, but the
+analysis effectively converts them to boolean before use. The remaining
+arguments are interpreted in the same way as ``ACQUIRE``. See
+:ref:`mutexheader`, below, for example uses.
Because the analysis doesn't support conditional locking, a capability is
treated as acquired after the first branch on the return value of a try-acquire
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 9b8f5b7e80e7e..072d7e8f91b36 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3721,7 +3721,8 @@ def warn_attribute_wrong_decl_type : Warning<
"|types and namespaces"
"|variables, functions and classes"
"|kernel functions"
- "|non-K&R-style functions}2">,
+ "|non-K&R-style functions"
+ "|functions that return bool, integer, or a pointer type}2">,
InGroup;
def err_attribute_wrong_decl_type :
Error;
def warn_type_attribute_wrong_type : Warning<
diff --git a/clang/include/clang/Sema/ParsedAttr.h
b/clang/include/clang/Sema/ParsedAttr.h
index 22cbd0d90ee43..7a78056b6b7b1 100644
--- a/clang/include/clang/Sema/ParsedAttr.h
+++ b/clang/include/clang/Sema/ParsedAttr.h
@@ -1101,6 +1101,7 @@ enum AttributeDeclKind {
ExpectedFunctionVariableOrClass,
ExpectedKernelFunction,
ExpectedFunctionWithProtoType,
+ ExpectedFunctionReturningBoolIntegerOrPointer,
};
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
diff --git a/clang/lib/Analysis/ThreadSafety.cpp
b/clang/lib/Analysis/ThreadSafety.cpp
index e25b843c9bf83..582181f79063c 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -32,6 +32,7 @@
#include "clang/Analysis/Analyses/ThreadSafetyUtil.h"
#include "clang/Analysis/AnalysisDeclContext.h"
#include "clang/Analysis/CFG.h"
+#include "clang/Basic/AttrKinds.h"
#include "clang/Basic/Builtins.h"
#include "clang/Basic/LLVM.h"
#include "clang/Basic/OperatorKinds.h"
@@ -1366,6 +1367,8 @@ void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs,
AttrType *Attr,
branch = BLE->getValue();
else if (const auto *ILE = dyn_cast_or_null(BrE))
branch = ILE->getValue().getBoolValue();
+ else
+llvm_unreachable("Trylock success values must be bool or integer.");
int branchnum = branch ? 0 : 1;
if (Neg)
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index ce6b5b1ff6f93..d07f9d6c5568b 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -25,6 +25,7 @@
#include "clang/Basic/CharInfo.h"
#include "clang/Basic/Cuda.h"
#include "clang/Basic/DarwinSDKInfo.h"
+#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/HLSLRuntime.h"
[clang] [clang][ThreadSafety] Warn when constructor is trylock (PR #95290)
dmcardle wrote: Just force-pushed what I described in the last comment. 1. Changed void-returning trylock functions to return bool in tests. (Probably needs another pass to minimize the diff and undo some unnecessary changes.) 2. Now enforcing bool/int/pointer returns from trylock-annotated functions. 3. Also updated the Thread Safety Analysis doc a bit. PTAL. Thanks! https://github.com/llvm/llvm-project/pull/95290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Warn when constructor is trylock (PR #95290)
@@ -3657,8 +3657,8 @@ class Foo {
int a GUARDED_BY(mu_);
bool c;
- inttryLockMutexI() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu_);
- Mutex* tryLockMutexP() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu_);
+ bool tryLockMutexI() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu_);
dmcardle wrote:
This bool can stay int if we go with the permissive bool/int/pointer return
values.
https://github.com/llvm/llvm-project/pull/95290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Warn when constructor is trylock (PR #95290)
@@ -614,9 +619,23 @@ static bool checkTryLockFunAttrCommon(Sema &S, Decl *D,
const ParsedAttr &AL,
return false;
}
- // check that all arguments are lockable objects
+ // Check that all remaining arguments are lockable objects.
checkAttrArgsAreCapabilityObjs(S, D, AL, Args, 1);
+ // Check that the attribute is applied to a function.
+ if (!D->isFunctionOrFunctionTemplate()) {
+return false;
+ }
+ // Check that the function returns a boolean, integer, or pointer.
+ QualType ReturnType = D->getAsFunction()->getReturnType();
+ if (!ReturnType->isBooleanType() && !ReturnType->isIntegerType() &&
+ !ReturnType->isPointerType()) {
+S.Diag(AL.getLoc(), diag::err_attribute_wrong_decl_type)
+<< AL << AL.isRegularKeywordAttribute()
+<< ExpectedFunctionReturningBoolIntegerOrPointer;
+return false;
+ }
+
return true;
}
dmcardle wrote:
I'm adding a some new logic for checking the success argument type. If I leave
this as is, they will have the same style. If you feel strongly, I'm happy to
apply DeMorgan's law here :)
https://github.com/llvm/llvm-project/pull/95290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Warn when constructor is trylock (PR #95290)
@@ -614,9 +619,23 @@ static bool checkTryLockFunAttrCommon(Sema &S, Decl *D,
const ParsedAttr &AL,
return false;
}
- // check that all arguments are lockable objects
+ // Check that all remaining arguments are lockable objects.
checkAttrArgsAreCapabilityObjs(S, D, AL, Args, 1);
+ // Check that the attribute is applied to a function.
+ if (!D->isFunctionOrFunctionTemplate()) {
dmcardle wrote:
Indeed it is not! Thanks for flagging.
https://github.com/llvm/llvm-project/pull/95290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Warn when constructor is trylock (PR #95290)
https://github.com/dmcardle updated
https://github.com/llvm/llvm-project/pull/95290
>From dc8e7b16d5e7318819e61223aa2fc9f483998806 Mon Sep 17 00:00:00 2001
From: Dan McArdle
Date: Thu, 20 Jun 2024 17:43:16 -0400
Subject: [PATCH] [clang][ThreadSafety] Check trylock function success and
return types
With this change, Clang will generate errors when trylock functions have
improper return types. Today, it silently fails to apply the trylock
attribute to these functions which may incorrectly lead users to believe
they have correctly acquired locks before accessing guarded data.
This change also improves the documentation with descriptions of of the
subtle gotchas that arise from the analysis interpreting the success arg
as a boolean.
Issue #92408
---
clang/docs/ThreadSafetyAnalysis.rst | 52 -
.../clang/Basic/DiagnosticSemaKinds.td| 23 +++-
clang/include/clang/Sema/ParsedAttr.h | 2 +
clang/lib/Analysis/ThreadSafety.cpp | 82 --
clang/lib/Sema/SemaDeclAttr.cpp | 34 --
clang/test/Sema/attr-capabilities.c | 12 +-
.../SemaCXX/warn-thread-safety-analysis.cpp | 96 +++-
.../SemaCXX/warn-thread-safety-parsing.cpp| 107 +++---
clang/unittests/AST/ASTImporterTest.cpp | 6 +-
9 files changed, 332 insertions(+), 82 deletions(-)
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index dcde0c706c704..0ecbebe7a692f 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -420,10 +420,17 @@ TRY_ACQUIRE(, ...), TRY_ACQUIRE_SHARED(, ...)
*Previously:* ``EXCLUSIVE_TRYLOCK_FUNCTION``, ``SHARED_TRYLOCK_FUNCTION``
These are attributes on a function or method that tries to acquire the given
-capability, and returns a boolean value indicating success or failure.
-The first argument must be ``true`` or ``false``, to specify which return value
-indicates success, and the remaining arguments are interpreted in the same way
-as ``ACQUIRE``. See :ref:`mutexheader`, below, for example uses.
+capability, and returns a boolean, integer, or pointer value indicating success
+or failure.
+
+The attribute's first argument defines whether a zero or non-zero return value
+indicates success. Syntactically, it accepts ``NULL`` or ``nullptr``, ``bool``
+and ``int`` literals, as well as enumerator values. *The analysis only cares
+whether this success value is zero or non-zero.* This leads to some subtle
+consequences, discussed in the next section.
+
+The remaining arguments are interpreted in the same way as ``ACQUIRE``. See
+:ref:`mutexheader`, below, for example uses.
Because the analysis doesn't support conditional locking, a capability is
treated as acquired after the first branch on the return value of a try-acquire
@@ -445,6 +452,43 @@ function.
}
}
+Subtle Consequences of Non-Boolean Success Values
+^
+
+The trylock attributes accept non-boolean expressions for the success value,
but
+the analysis only cares whether the value is zero or non-zero.
+
+Suppose you define an enum with two non-zero enumerators: ``LockAcquired = 1``
+and ``LockNotAcquired = 2``. If your trylock function returns ``LockAcquired``
+on success and ``LockNotAcquired`` on failure, the analysis may fail to detect
+access to guarded data without holding the mutex because they are both
non-zero.
+
+.. code-block:: c++
+ // *** Beware: this code demonstrates incorrect usage. ***
+
+ enum TrylockResult { LockAcquired = 1, LockNotAcquired = 2 };
+
+ class CAPABILITY("mutex") Mutex {
+ public:
+TrylockResult TryLock() TRY_ACQUIRE(LockAcquired);
+ };
+
+ Mutex mu;
+ int a GUARDED_BY(mu);
+
+ void foo() {
+if (mu.TryLock()) { // This branch satisfies the analysis, but permits
+// unguarded access to `a`!
+ a = 0;
+ mu.Unlock();
+}
+ }
+
+It's also possible to return a pointer from the trylock function. Similarly,
all
+that matters is whether the success value is zero or non-zero. For instance, a
+success value of `true` means the function returns a non-null pointer on
+success.
+
ASSERT_CAPABILITY(...) and ASSERT_SHARED_CAPABILITY(...)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 9bf55acbac011..f33f4a1782d76 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3272,11 +3272,23 @@ def warn_aligned_attr_underaligned :
Warning,
def err_attribute_sizeless_type : Error<
"%0 attribute cannot be applied to sizeless type %1">;
def err_attribute_argument_n_type : Error<
- "%0 attribute requires parameter %1 to be %select{int or bool|an integer "
- "constant|a string|an identifier|a constant expression|a builtin
function}2">;
+ "%0 attribute
[clang] [clang][ThreadSafety] Check trylock function success and return types (PR #95290)
https://github.com/dmcardle edited https://github.com/llvm/llvm-project/pull/95290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Check trylock function success and return types (PR #95290)
dmcardle wrote: > LGTM -- that new documentation is fantastic, thank you for that! Thank you! I hope it saves future readers some time. I don't have write access, so feel free to merge unless we're waiting for @aaronpuchert to review :) https://github.com/llvm/llvm-project/pull/95290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Check trylock function success and return types (PR #95290)
dmcardle wrote: So... I appear to have accidentally fixed a bug. Looks like enum success values were not being evaluated, and defaulted to false. As a result, the analysis could fail to detect unguarded access [[demo on godbolt.org](https://godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGIAMwA7KSuADJ4DJgAcj4ARpjEIABsAKykAA6oCoRODB7evgHBmdmOAuGRMSzxiam2mPZlDEIETMQE%2BT5%2BQfWNuS1tBBXRcQnJaQqt7Z2FPZODw1U14wCUtqhexMjsHAD0uwDUALJMyMRZB0QHjEyx9JeYkxHAB6hUB7GoBAgHAG5teEMBAUr3e30wlwQxEwTHQBwUTComAIAE8DoYxCjsgoAHQmDQAQXxRLM/jw7zkQmwAH03ATlASLABJUJMgAqAE1ieZ/FgaJEDqEAPJuADSjNC2AO0plsrl8pl1OpTAIBGIeFiXgImCVUFE6VudEIKIg5jMLC1mFUZpWtu5pL5EQhBKEVIASmzqdgABpuUKUpkANRpwrFEBxEZW8qVKrVGstuogTAUSna1P1htoxogSsDBOpBLdAHEhErbSt7bzMPzna7sB7qUIABKF7AAEWpodF4cjCulMdV6s12sTydTBGpCgQbUw6HTTANsSNqJz1LzBeLpep5crjoFPr9AeDnZFoupADE5FE3GymUKoj2cVHFcrB/GR9SoKcAI5ePDQ%2BdF2XE1c3zQsSzLO1CR5PcIWbVsOy7C8rxvO8HwjJ8FQHONhx1T8k2QX9/x1KcZznDMlyzFdQI3CDtygkkqxrA4ryQy9r1ve9H2fPt%2B1fHCE3w6F6GTHVgEYBI8GQQDM2zGjwK3HdoIdasnQOA9/SEIMaTZN0OTYlDOPQ3tsKHQSoDVFFlUIv8AIo4DV3XBTIIrZSmLU%2BC3XbaldP009kI4tDuNlUz3zwiziCsn9bJI6doXIhdZOotcwM3Fzd1U/dfU07STzFUsvIARTkJkvLbYK%2BNjMyPygaEiOhBQZMouSUtoxSGJgzK4JbMq8tFArsGK0r2wqkL%2BOq8KIDqmLGtI%2BKmoc%2BS0vo1zGIaJQMuYrsJSlXi9rGqqwsTWhUGQABrW56A6lTmJdd1PQ0o8Q1PUaX0O3DRxTBIJytZBvGyX4dRO87HNSuilMY2CDju%2BtPU87yu1euVQo%2B/Cx2%2Byc4tnalgbO0G2vStyocerTjwMwKuIwniDhR8yIF%2B/68EBnHTrO6kqC8BgHFyfHnJWzaPJ6hH/PY1DKd7aNxqO/C5ux3H2c57mBF55aIc65jWJFwygqp/aaal1GoE5%2BWOa5poVfB673Kyw9SZ0vTybF4zMNpmr6dUP6vABnVLJNxXzaWy3VvVwWEJ8h2tYp53qddybZbnX3WYVs2ecD9rg5utSSdyrsBqGsrRtjxMGa9pmgdZxrpuI9ALfTgWBXhxDTzzkqC915GDbp%2BOWfOyvMHq2da8JtaGHwKhuUJfZBVZ%2BFMBYIEpJBARaBReuISLORCzbby8zdPXeKL/DgC8NosDnf5iCtqHlE9Det53wt947966fSCdj9P7GL46yfDjcRLmqOEeK8BgK817qWyk9akmsxQBSdhVQ%2BtUGgwiUAtKiIFWp8zVpnBuQsOwwLPKLIySNEFTWQaJTGZE0EtScqrK%2BXUDh3zdNvDsFgOQQFUNTJ%2B%2BsX5uw/sQM%2B1JYgmk4fQ5iN9qRMJYdSNhHCuH7VIW/ak/DBHCLkT/AkU8PAsBYAIcBQg3BCmUMLMUO1uGVTfIbCACg0DpDlqzS6mAxFqQJG4fO3kCTnjZPWJGfZSHRWrsqKg2piBD35kTBhrj3GsOwOeIUXlfGS14ZNAJ814j8GhGE7B1sIRIS8myOQboogjU4eY0h8toQEE2JEGuoiM45MFM3L0kCWGJKSZYumuNGolzPlk5xAoohCh8k2LyBIOxCE8dgTkBYogElCByLSQgsKdzdgwVA1JvjQlhJOREyIorMBXtiDRU83SYGAA1bIAgHiTAOBky4kVcbog6dqEEAB3QgPxDDXAYD4BIKoSDwi8MgbYKZ0TEGPmwQQeJf4HGUOqAFgNiCXIYEvMECAIQYkOXgEE2R6CCBXrcpgdBZyXFQNcf43gVQQgUECkFII2gQsYMCYkU9vgqgOK8hIEJIgfISAcEwSQNBuG9N6CwqBPDhBCWIbAqh0jEAFRoA4aziD8sFUyQQZyEiSr%2BbQBV6JR7ohTL8uE4IWWHApV4CErzkyEtoEoaFBJmBsAUAabYBw2QPNZkIWljwFDYB%2BSwc8YglBRDOSqMu/LAhWGgoSRgPh3WevOqcmltACCRosAcINdBNgQhMP4NsBwNCkAON64Fvr%2BX5oOFwSNbY83RqJISP6Y5GmmIsJKY4lpVDpuJOkLwdwpIgGJNKD1KJcbJq8KmhNKJQiswgFGbOx5fKO2IaWulFZ/D1ulL8VAeA4RyFAbO%2BdkC7bQKiMuoK6760mECLWjdE8CRNtBSO3GbJHhpuvZujt2ou0WmpHWodBwIhpqYNSRhm9mHeVkb%2BqMeaC0aH/TGgkW6d0mrfUI8VtBqRWjlb6nmMGo0AelOSA4EAwBgF/TiEdM6Qa2nTdKKeUQvgQnBIBhgiLtRwliMQQwyAfhoFHjkAQDr5QgesGJjd5ip5sgQNi%2BECANi0E4xCKgBhgDiU42iFj0mtlwiELs1E0MDlYmxWRwjMoKP7txnOhDSGZTXtrYSaU9nuQ3ps/Zg4U8nWPFdRCZ9XqfUpn9T4INdrohhscIDDgaxaCcBSLwPw3BeCoE4G4MTlh4QbC2Lm0kPBSAEE0FFtYZ0Aj%2BBxP4crFXKuVaSPoTgkh4sFdIMljgvAFAgCLfljgWg1hwFgEgNALB0jErIBQCAA2hv0ESE2owGziCczOnwOgIS2sQFiI1pczBIqcFyxttoKIhSxG0JgBw23eADchQQIUoCUSNawJqYA/9aB2tO6QLA88jDiC67wfA0JuaAza19kIqhjuWhe0BhojWsxcb2x4LAjW4wsBe4iz4Sg2xz0MMALMRgCtrBU0wYAChAx4EwK8oUdiEu5f4IIEQYh2BSBkIIRQKh1CA90FwfQGOUBpZsFDtrkA1ioDfrkAHABaSY6BYOmEsNYLg1aRcAHUEjnHlfmzZMJ0Bi/02iEXbZKQ0jpAyZkrJOSwdy6gRF6osB87nb0Y7TQXCjxmH4dnYQnRLDGOzkogm8ieC6HoL3TRFijESOzuwdv%2BhTA6L7woofkFK2aJHoP1QPe2Ej07vQ8x2hJ%2BWFwNYChMvbAkNF2LDXAfNYOKoAAHEkEXSRJAHGmy8CAap5tRggLgQgAKeS594J17rax0WwjGDb4r5WytVYn%2BVmrMWOD1dIAlrQTXOCtfa3lnHpBeuIBABsAgvaCDkEoON4bURWA7CrzXuvDeDBGBIy3hgZ0Vi8FnJ3y3egqfCFEOIen7%2BmdqEa2z0gV5bjdIU7YvDgOLefRrZrIULUPfUECvavWvevRvW/Obe/NvbRCbPlbvR/NfL7W0UgUfUrSfSfWrWfUvRfZrFfDrHHMAswCgpLZfPA/vUgRFZFEASQIAA%3D%3D%3D)]. I think that the old `isIntOrBool()` function accidentally let enumerator values through, but the analysis assumed it would only ever see `CXXBoolLiteralExpr ` or `IntegerLiteral `. I'm adding a regression test that closely resembles the demo, but verifies that the analysis actually detects the unguarded access. I also realized I was supposed to update the release notes. New patch with these changes coming momentarily. https://github.com/llvm/llvm-project/pull/95290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Check trylock function success and return types (PR #95290)
https://github.com/dmcardle updated https://github.com/llvm/llvm-project/pull/95290 >From 66715ea5566946f0608d77dc4c86b23e70179760 Mon Sep 17 00:00:00 2001 From: Dan McArdle Date: Thu, 20 Jun 2024 17:43:16 -0400 Subject: [PATCH] [clang][ThreadSafety] Check trylock function success and return types With this change, Clang will generate errors when trylock functions have improper return types. Today, it silently fails to apply the trylock attribute to these functions which may incorrectly lead users to believe they have correctly acquired locks before accessing guarded data. As a side effect of explicitly checking the success argument type, I seem to have fixed a false negative in the analysis that occurred when a trylock's success argument is negative. I've added a regression test to warn-thread-safety-analysis.cpp named `TrylockSuccessEnumFalseNegative`. This change also improves the documentation with descriptions of of the subtle gotchas that arise from the analysis interpreting the success arg as a boolean. Issue #92408 --- clang/docs/ReleaseNotes.rst | 10 ++ clang/docs/ThreadSafetyAnalysis.rst | 52 +++- .../clang/Basic/DiagnosticSemaKinds.td| 23 +++- clang/include/clang/Sema/ParsedAttr.h | 2 + clang/lib/Analysis/ThreadSafety.cpp | 82 +++- clang/lib/Sema/SemaDeclAttr.cpp | 34 +++-- clang/test/Sema/attr-capabilities.c | 12 +- .../SemaCXX/warn-thread-safety-analysis.cpp | 123 +- .../SemaCXX/warn-thread-safety-parsing.cpp| 107 --- clang/unittests/AST/ASTImporterTest.cpp | 6 +- 10 files changed, 369 insertions(+), 82 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 7ac0fa0141b47..28ec3d54a5a77 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -71,6 +71,11 @@ C++ Specific Potentially Breaking Changes To fix this, update libstdc++ to version 14.1.1 or greater. +- Clang now emits errors when Thread Safety Analysis trylock attributes are + applied to functions or methods with incompatible return values, such as + constructors, destructors, and void-returning functions. This only affects the + ``TRY_ACQUIRE`` and ``TRY_ACQUIRE_SHARED`` attributes (and any synonyms). + ABI Changes in This Version --- - Fixed Microsoft name mangling of implicitly defined variables used for thread @@ -720,6 +725,11 @@ Bug Fixes in This Version - Fixed `static_cast` to array of unknown bound. Fixes (#GH62863). +- Clang's Thread Safety Analysis now evaluates trylock success arguments of enum + types rather than silently defaulting to false. This fixes a class of false + negatives where the analysis failed to detect unchecked access to guarded + data. + Bug Fixes to Compiler Builtins ^^ diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst index dcde0c706c704..0ecbebe7a692f 100644 --- a/clang/docs/ThreadSafetyAnalysis.rst +++ b/clang/docs/ThreadSafetyAnalysis.rst @@ -420,10 +420,17 @@ TRY_ACQUIRE(, ...), TRY_ACQUIRE_SHARED(, ...) *Previously:* ``EXCLUSIVE_TRYLOCK_FUNCTION``, ``SHARED_TRYLOCK_FUNCTION`` These are attributes on a function or method that tries to acquire the given -capability, and returns a boolean value indicating success or failure. -The first argument must be ``true`` or ``false``, to specify which return value -indicates success, and the remaining arguments are interpreted in the same way -as ``ACQUIRE``. See :ref:`mutexheader`, below, for example uses. +capability, and returns a boolean, integer, or pointer value indicating success +or failure. + +The attribute's first argument defines whether a zero or non-zero return value +indicates success. Syntactically, it accepts ``NULL`` or ``nullptr``, ``bool`` +and ``int`` literals, as well as enumerator values. *The analysis only cares +whether this success value is zero or non-zero.* This leads to some subtle +consequences, discussed in the next section. + +The remaining arguments are interpreted in the same way as ``ACQUIRE``. See +:ref:`mutexheader`, below, for example uses. Because the analysis doesn't support conditional locking, a capability is treated as acquired after the first branch on the return value of a try-acquire @@ -445,6 +452,43 @@ function. } } +Subtle Consequences of Non-Boolean Success Values +^ + +The trylock attributes accept non-boolean expressions for the success value, but +the analysis only cares whether the value is zero or non-zero. + +Suppose you define an enum with two non-zero enumerators: ``LockAcquired = 1`` +and ``LockNotAcquired = 2``. If your trylock function returns ``LockAcquired`` +on success and ``LockNotAcquired`` on failure, the analysis may fail to detect +access to guarded data without holding the mutex because they are bot
[clang] [clang][ThreadSafety] Check trylock function success and return types (PR #95290)
https://github.com/dmcardle updated https://github.com/llvm/llvm-project/pull/95290 >From 807953fc74920e3d5ed727a00c0e1177870c56fe Mon Sep 17 00:00:00 2001 From: Dan McArdle Date: Thu, 20 Jun 2024 17:43:16 -0400 Subject: [PATCH] [clang][ThreadSafety] Check trylock function success and return types With this change, Clang will generate errors when trylock functions have improper return types. Today, it silently fails to apply the trylock attribute to these functions which may incorrectly lead users to believe they have correctly acquired locks before accessing guarded data. As a side effect of explicitly checking the success argument type, I seem to have fixed a false negative in the analysis that could occur when a trylock's success argument is an enumerator. I've added a regression test to warn-thread-safety-analysis.cpp named `TrylockSuccessEnumFalseNegative`. This change also improves the documentation with descriptions of of the subtle gotchas that arise from the analysis interpreting the success arg as a boolean. Issue #92408 --- clang/docs/ReleaseNotes.rst | 10 ++ clang/docs/ThreadSafetyAnalysis.rst | 52 +++- .../clang/Basic/DiagnosticSemaKinds.td| 23 +++- clang/include/clang/Sema/ParsedAttr.h | 2 + clang/lib/Analysis/ThreadSafety.cpp | 82 +++- clang/lib/Sema/SemaDeclAttr.cpp | 34 +++-- clang/test/Sema/attr-capabilities.c | 12 +- .../SemaCXX/warn-thread-safety-analysis.cpp | 123 +- .../SemaCXX/warn-thread-safety-parsing.cpp| 107 --- clang/unittests/AST/ASTImporterTest.cpp | 6 +- 10 files changed, 369 insertions(+), 82 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 7ac0fa0141b47..28ec3d54a5a77 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -71,6 +71,11 @@ C++ Specific Potentially Breaking Changes To fix this, update libstdc++ to version 14.1.1 or greater. +- Clang now emits errors when Thread Safety Analysis trylock attributes are + applied to functions or methods with incompatible return values, such as + constructors, destructors, and void-returning functions. This only affects the + ``TRY_ACQUIRE`` and ``TRY_ACQUIRE_SHARED`` attributes (and any synonyms). + ABI Changes in This Version --- - Fixed Microsoft name mangling of implicitly defined variables used for thread @@ -720,6 +725,11 @@ Bug Fixes in This Version - Fixed `static_cast` to array of unknown bound. Fixes (#GH62863). +- Clang's Thread Safety Analysis now evaluates trylock success arguments of enum + types rather than silently defaulting to false. This fixes a class of false + negatives where the analysis failed to detect unchecked access to guarded + data. + Bug Fixes to Compiler Builtins ^^ diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst index dcde0c706c704..0ecbebe7a692f 100644 --- a/clang/docs/ThreadSafetyAnalysis.rst +++ b/clang/docs/ThreadSafetyAnalysis.rst @@ -420,10 +420,17 @@ TRY_ACQUIRE(, ...), TRY_ACQUIRE_SHARED(, ...) *Previously:* ``EXCLUSIVE_TRYLOCK_FUNCTION``, ``SHARED_TRYLOCK_FUNCTION`` These are attributes on a function or method that tries to acquire the given -capability, and returns a boolean value indicating success or failure. -The first argument must be ``true`` or ``false``, to specify which return value -indicates success, and the remaining arguments are interpreted in the same way -as ``ACQUIRE``. See :ref:`mutexheader`, below, for example uses. +capability, and returns a boolean, integer, or pointer value indicating success +or failure. + +The attribute's first argument defines whether a zero or non-zero return value +indicates success. Syntactically, it accepts ``NULL`` or ``nullptr``, ``bool`` +and ``int`` literals, as well as enumerator values. *The analysis only cares +whether this success value is zero or non-zero.* This leads to some subtle +consequences, discussed in the next section. + +The remaining arguments are interpreted in the same way as ``ACQUIRE``. See +:ref:`mutexheader`, below, for example uses. Because the analysis doesn't support conditional locking, a capability is treated as acquired after the first branch on the return value of a try-acquire @@ -445,6 +452,43 @@ function. } } +Subtle Consequences of Non-Boolean Success Values +^ + +The trylock attributes accept non-boolean expressions for the success value, but +the analysis only cares whether the value is zero or non-zero. + +Suppose you define an enum with two non-zero enumerators: ``LockAcquired = 1`` +and ``LockNotAcquired = 2``. If your trylock function returns ``LockAcquired`` +on success and ``LockNotAcquired`` on failure, the analysis may fail to detect +access to guarded data without holding the mutex because they
[clang] [clang][ThreadSafety] Check trylock function success and return types (PR #95290)
https://github.com/dmcardle edited https://github.com/llvm/llvm-project/pull/95290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Fix code block syntax in ThreadSafetyAnalysis.rst (PR #96494)
https://github.com/dmcardle created
https://github.com/llvm/llvm-project/pull/96494
Without a newline, documentation was failing to build with this error:
Warning, treated as error:
/home/runner/work/llvm-project/llvm-project/clang-build/tools/clang/docs/ThreadSafetyAnalysis.rst:466:Error
in "code-block" directive:
maximum 1 argument(s) allowed, 10 supplied.
Issue #92408
>From 3bd8ad70da4c3ab7a89969dd31b728dcf301d7ab Mon Sep 17 00:00:00 2001
From: Dan McArdle
Date: Mon, 24 Jun 2024 10:39:58 -0400
Subject: [PATCH] [clang][ThreadSafety] Fix code block syntax in
ThreadSafetyAnalysis.rst
Without a newline, documentation was failing to build with this error:
Warning, treated as error:
/home/runner/work/llvm-project/llvm-project/clang-build/tools/clang/docs/ThreadSafetyAnalysis.rst:466:Error
in "code-block" directive:
maximum 1 argument(s) allowed, 10 supplied.
Issue #92408
---
clang/docs/ThreadSafetyAnalysis.rst | 1 +
1 file changed, 1 insertion(+)
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index 0ecbebe7a692f..1513719caa464 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -464,6 +464,7 @@ on success and ``LockNotAcquired`` on failure, the analysis
may fail to detect
access to guarded data without holding the mutex because they are both
non-zero.
.. code-block:: c++
+
// *** Beware: this code demonstrates incorrect usage. ***
enum TrylockResult { LockAcquired = 1, LockNotAcquired = 2 };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Fix code block syntax in ThreadSafetyAnalysis.rst (PR #96494)
dmcardle wrote: @AaronBallman PTAL — looks like #95290 broke the documentation. https://github.com/llvm/llvm-project/pull/96494 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][ThreadSafety] Check trylock function success and return types (PR #95290)
dmcardle wrote: Hi @asmok-g, it was definitely not my intention to break any use cases where the analysis was behavior properly! I wanted to verify that the prior release, which predates this change, understands smart pointer returns on trylock functions, but I'm having trouble finding a test case where the analysis behaves that way. In this [example on godbolt.org] with Clang 18.1.0, I mocked up (1) a plain `unique_ptr` return value and (2) a nullable `unique_ptr` type alias. It looks like the analysis incorrectly emits an error for unguarded access in both scenarios. Could you help me construct an example of something that used to work and is now broken? Regardless, it still might make sense to add support for any return type that can be implicitly converted to bool. [example on godbolt.org]: https://godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGIMxqkrgAyeAyYAHI%2BAEaYxCAArAmkAA6oCoRODB7evv6BaRmOAqHhUSyx8Um2mPbFDEIETMQEOT5%2BATV1WY3NBKWRMXGJyQpNLW15nWN9A%2BWVIwCUtqhexMjsHAD0WwDUALJMyMTpu0S7jEzR9GeYY2HAu6hUu9GoBAi7AG7NeIYECieLw%2BmDOCGImCY6F2CiYVEwBAAnrtDGJERkFAA6EwaACCOPxZgAzHgXnIhNgAPpuXHKXEWACSwQZABUAJoE8xErA0cK7YIAeTcAGl6cFsLtJVLpTLZVLKZSmAQCMQ8NEvARMAqoKIUlc6IRERBzGYWBrMKoTYtrZziTywqDcUIKQAlFmU7AADTcwXJDIAalTBSKIJiw4tZQqlSq1ebtRAmAolC1Kbr9bRDRAFf7cZTcS6AOJCBXWxa27mYXmO53YN2UoQACXz2AAIpTg8LQ%2BG5ZKo8rVerNfHE8mCJSFAhmph0KmmHrogakVnKTm84Xi5TS%2BX7XyvT6/YH20LhZSAGJyCJuFkMgURLuYiPyxX92NDylQI4ARy8eAhs/ni5Gtmub5kWJY2niXI7qCjbNm2HZnheV43neYYPnKfYxoOWrvgmyDfr%2BWoTlOM5pguGZLsBa5gZuEGEhWVa7BeCHnpe163vej49r2z5YXGuEQvQiZasAjBxHgyD/ummZUaBG5bpBdqVg6ux7r6QgBlSLIumyLFIexqHdphA78VAKqIoq%2BE/n%2BZGAcuq5yeBZaKQxKmwS6raUtpunHohbEoZx0rGa%2BOFmcQFlftZRGThCpFztJlEriB65Oduym7t66maUeIrFh5ACKcgMh5LaBTx0YmW%2BUAQgREIKFJ5EyUl1HyXRUHpTBTYlTlwp5dghXFa2ZVBbxlWhRANVRfVxGxQ1dmySltHOfRtRKGljEdmKErcTtI0VSF8a0KgyAANZXPQbVKYxTquu6akHkGx7DU%2B%2B3YcOSZxGOFrIN4GRfFqR2nfZyU0Qp9HQbsN21u67meR2z0ysFb24SOn3jjF06UoDJ3Ay1qUuRD90aYeen%2BRxaFcbsSOmRA32/Xg/1Y8dJ2UlQXgMA4WS445S3rW5XVw75rHIeT3aRqNB24TNmPY6z7OcwI3OLWD7WMcxQv6QFFO7VTEvI1A7Oy2zHP1EroOXa5GX7sTWk6aTIuGeh1NVbTqg/V4f1auZRvy6bC3m8tqv83BXl2xrZOO5TzvjdLM7e8zcsm1z/utYHV0qUT2Udn1A0lcN0fxnTHsMwDzP1ZNhHoGbqd83ysPwceOdFXn2uI3rNOx0zp3l5gtXTtX%2BMrQw%2BBUJyeI7PyzMwpgLD/BJgICLQiK16CBZyPmLaeTmLo69xBe4cAXjNFgM4/MQFsQ8o7prxvW/5rvbevTTKRjofx%2BY2fbXj3sbjxY1jh3CeAwJeK9VKZQepSdWIo/IOzKvvaqtRIRKDmhRICzUeYq3TnXAWbYoEnmFgZBG8CJqIOEujEiKCmoOWVhfDquwb4uk3m2CwbIICqEpg/XWT8XZv2ICfSk0QjTsNoYxK%2BlIGFMMpCwthHDdrEJfpSXh/DBEyK/riCeHgWAsAEKAoQbgBTKEFiKLanDyovn1hABQaAUgy2ZudTAIiVK4jcLnTyuJTwslrAjHsxDIqV0VFQTUxAB68wJnQ5xrjmHYFPAKDy3jxbcPGn42asR%2BAQhCZgy2oIEIeRZHIF0EQhrsNMcQ2WEICBrHCFXYRacsn8kbh6cBTD4kJPMTTbG9Ui4nwyY4vkEQBReQbB5XEbYhDuOwOyPMERcTBDZBpIQGF24uwYKgSkHwIRQnHHCBEEVmBLwxGorkYR3ZYF2CYIkbg2DaPCuc7AY9cTMDYAoPUGxdgpAMGESk7M8Dfi1C/YgZyADsVhIJ4h%2BiOepxiLDigOOaVQQKQW4hSF4a4EkQAEklGMdAIAQDfN%2BZSf55y3BhAILcyULJwrBGZhACMmdDzeXtoQlUXgHHnMRZKL4qA8DQjkMA6ltLwE20gRERlAUyxEkRSYQFLY2X3PBUmXYFLETYxZHcAgCKMWws1PCs0lJZV4klCSlElJ6Hr0YZ5aRuqIznJbLsDQ%2Br8QGu%2BFy6EmoxgCNQJ4SkFoUh1QyIra1wLNWGpeBAXVmIlVUqBtaDVTrpRMD1ZYawDrZTht5djGlKapRSplU6nNnJpUOpzbsCe7ymCfLxSyglKp7mPLuC80EDAvC0AMNcL2iIbGKgzImWNuJi5GF2FEFt9iGSCGUCqM5CQrBTvBUYHFhgVlNE1MaMwZgh2tvoFaEwCQZU7rOUSW1WKcWVr%2BTWi5JLblFrBQYBVm1oUSn2HC3tyLUXIHRU69dI6x0TsjfysB1tsoMvDrA5lrKJWas5dypifLo3/qyoePBMDCHislYW8DoLcTysBEqlVare2Skfdq3Yuqs1GoTaa2%2BzDWFWv3ba%2B16HHW4g5S6247q3hep9X6rmgb2VSlJLsMNXhKQRspfynjwapQJuTUmhjqahOYnTfyrNkp815sLZBNDqHbUTybcOttayO1ajEH8BQBIODLFoJwBIvA/DcF4KgTgbhpMWBhKsdYoIuQ8FIAQTQ5nlgnRAJIQFmJiSSEkESVdRIwsRbMNISzHBJA2d86QBzHBeAKBAIEHzHAtDLDgLAJAaAWApDoHEcglAislfoPEWdwAuAAA4uCBBoLQIJGWIDRGSwuZg4VOBee680REApojaEwA4PrvAitsEEAKYBiJktYHVMAX%2BLaMt2dIFgWeRhxA5d4PgCEnN/pra0EEVQY3zQTfIIIWoyWMzRGIINjwWBksxhYJd/6xA3hKBbDPQwwAMxzt28sKgBhgAKH9HgTAAB3AUNjbNef4IIEQYh2BSBkIIRQKh1C7dILoLg%2Bg/soGc/oNUGXIDLFQC/LIa2AC0WKbWmCTZYLgXBdg04AOpxBOMQG16zIToDp9spEbOWzkipDSOkjJmTshtV51AH3VRYDJzSroY36guGHpMPw%2BOQgOnmMMfHhRMgCC13oI39Q5hDHiPjuwauejjFaJ4doehbcKwaA7y3FQDe2Ad6bm3Hu9dW4kMsBQbmNjB/0FZpLOPUu7FUPVgAbDThPkhdi1d2A1zEXBMQaAE7gQgJAznEi4IsXg2XcvLAQPz4YKuAtcAAJyYnC5F6LLfwuR4S9Hk7qX0uZe875vLMBEAgFWAQZFBBysQEq6V4gERWCbHj0nlPaeDADogMyhgJ1S9BHwEQRXehEfCFEOINHh/MdqGS3j0gUOHspAmxZqPpBbPd84AKDU4%2BgRx8T8n1P6fM/Z9zwgE0SqziCLyJBLzLwH38xACJCJExFgIQMQKQI70Syf2Sx71sD73Lz8w7zMC73s04EgKB2WA%2B39T8EkCAA%3D%3D%3D https://github.com/llvm/llvm-project/pull/95290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
