This reverts commit 873d6bc3b415f1c2d942bbf4e4219c4bdcd4f2f8.
This causes Linux kernel build to fail because it relied on
alias-invalidation in kernel/core/sched.c.
Local variables passed by non-const pointer or reference to a function
were previously invalidated in the LocalVariableMap (VarMapBuilder), on
the assumption that the callee might change what they point to. This
caused false positives when the function also carries ACQUIRE/RELEASE
annotations: handleCall translates those annotations with the pre-call
context, while subsequent guard checks use the post-invalidation
context, producing an expansion mismatch and a spurious warning.
The invalidation rules were a heuristic with significant complexity
(including a special-case carve-out for std::bind/bind_front) and
unclear benefit. Instead of adding more heuristics, drop the
alias-invalidation rules entirely.
Discussion: https://github.com/llvm/llvm-project/pull/183640
Previously, `guarded_by` and `pt_guarded_by` only accepted a single
capability argument. Introduce support for declaring that a variable is
guarded by multiple capabilities, which exploits the following property:
any writer must hold all capabilities, so holding any one of them
(exclusive or shared) guarantees at least shared (read) access.
Therefore, writing requires all listed capabilities to be held
exclusively, while reading only requires at least one to be held.
This synchronization pattern is frequently used where the underlying
lock implementation does not support real reader locking, and instead
several lock "shards" are used to reduce contention for readers. For
example, the Linux kernel makes frequent use of this pattern [1].
Backwards compatibility is not affected by this change: for the time
being we deliberately do not change the semantics of multiple stacked
attributes (this retains existing semantics precisely, while giving a
way to choose the "stricter" semantics if needed).
Previous Version: https://github.com/llvm/llvm-project/pull/185173
Link: https://lore.kernel.org/all/20250307085204.GJ16878@noisy.programming.kicks-ass.net/ [1]
[Thread Analysis] Fix a bug in context update in alias-analysis
Previously, in 'updateLocalVarMapCtx', context was updated to the one
immediately after the provided statement 'S'. It is incorrect,
because 'S' hasn't been processed at that point. This issue could
result in false positives. For example,
```
void f(Lock_t* F)
{
Lock_t* L = F; // 'L' aliases with 'F'
L->Lock(); // 'L' holds the lock
// Before the fix, the analyzer saw the definition of 'L' being cleared before 'L' was unlocked.
unlock(&L); // unlock (*L)
}
```
This commit fixes the issue.
rdar://169236809
The commit b4c98fcbe1504841203e610c351a3227f36c92a4 introduces
alias-analysis and conservatively invalidates variable definitions at
function calls. For each invalidated argument, it creates and pushes a
context. So if there are multiple arguments being invalidated, there are
more than one context being pushed. However, the analysis expects one
context at the program point of a call, causing context mismatch. This
issue could lead to false negatives.
For example,
```
MyLock->Lock(); // 'MyLock' holds the lock
Lock_t *Ptr = MyLock; // 'Ptr' aliases with 'MyLock'
// Before the fix, two contexts are saved and pushed at the call below, causing context mismatch later.
escapeAliasMultiple(&Irrelevant, &Ptr);
Ptr->Unlock(); // 'Ptr' may no longer hold the lock but the analyzer missed it due to context mismatch
```
This commit fixes the issue.
---------
Co-authored-by: Marco Elver <me@marcoelver.com>
Fixes a false positive in thread safety analysis when function
parameters have lock/unlock annotations in their constructor/destructor.
PR #169320 added destructors to the CFG, which introduced false
positives in thread safety analysis for function parameters. According
to [expr.call#6](https://eel.is/c++draft/expr.call#6), parameter
initialization occurs in the caller's context while destruction occurs
in the callee's context.
```cpp
class A {
public:
A() EXCLUSIVE_LOCK_FUNCTION(mu_) { mu_.Lock(); }
~A() UNLOCK_FUNCTION(mu_) { mu_.Unlock(); }
private:
Mutex mu_;
};
int do_something(A a) { // Previously: false positive warning
return 0;
}
```
Previously, thread safety analysis would see the destructor (unlock) in
`do_something` but not the corresponding constructor (lock) that
happened in the caller's context, causing a false positive.
This rename was made as part of
https://github.com/llvm/llvm-project/pull/147835 in order to ease
rebasing the PR, and give a nice window for other patches to get rebased
as well.
It has been a while already, so lets go ahead and rename it back.
We observed slowdowns in auto-generated million+ line C++ source files
due to recent fixes to LocalVariableMap.
Rather than recompute the canonical underlying non-reference
VarDefinition every time we need to perform variable definition
intersection at CFG merge points, pre-compute the canonical references
once on construction. Ensure to
maintain the invariant that if both the direct and canonical reference
are being invalidated, both become 0.
Reported-by: Bogdan Graur <bgraur@google.com>
Fix a false positive in thread safety alias analysis caused by incorrect
late resolution of aliases. The analysis previously failed to
distinguish between an alias and its defining expression; reassigning a
variable within that expression (e.g., `ptr` in `alias = ptr->field`)
would incorrectly change the dependent alias as well.
The fix is to properly use LocalVariableMap::lookupExpr's updated
context in a recursive lookup.
Reported-by: Christoph Hellwig <hch@lst.de>
Link: https://lkml.kernel.org/r/20250919140803.GA23745@lst.de
Add basic alias analysis for capabilities by reusing LocalVariableMap,
which tracks currently valid definitions of variables. Aliases created
through complex control flow are not tracked. This implementation would
satisfy the basic needs of addressing the concerns for Linux kernel
application [1].
For example, the analysis will no longer generate false positives for
cases such as (and many others):
void testNestedAccess(Container *c) {
Foo *ptr = &c->foo;
ptr->mu.Lock();
c->foo.data = 42; // OK - no false positive
ptr->mu.Unlock();
}
void testNestedAcquire(Container *c) EXCLUSIVE_LOCK_FUNCTION(&c->foo.mu)
{
Foo *buf = &c->foo;
buf->mu.Lock(); // OK - no false positive
}
Given the analysis is now able to identify potentially unsafe patterns
it was not able to identify previously (see added FIXME test case for an
example), mark alias resolution as a "beta" feature behind the flag
`-Wthread-safety-beta`.
**Fixing LocalVariableMap:** It was found that LocalVariableMap was not
properly tracking loop-invariant aliases: the old implementation failed
because the merge logic compared raw VarDefinition IDs. The algorithm
for handling back-edges (in createReferenceContext()) generates new
'reference' definitions for loop-scoped variables. Later ID comparison
caused alias invalidation at back-edge merges (in intersectBackEdge())
and at subsequent forward-merges with non-loop paths (in
intersectContexts()).
Fix LocalVariableMap by adding the getCanonicalDefinitionID() helper
that resolves any definition ID down to its non-reference base. As a
result, a variable's definition is preserved across control-flow merges
as long as its underlying canonical definition remains the same.
Link:
https://lore.kernel.org/all/CANpmjNPquO=W1JAh1FNQb8pMQjgeZAKCPQUAd7qUg=5pjJ6x=Q@mail.gmail.com/
[1]
This reintroduces `Type.h`, having earlier been renamed to `TypeBase.h`,
as a redirection to `TypeBase.h`, and redirects most users to include
the former instead.
This is a preparatory patch for being able to provide inline definitions
for `Type` methods which would otherwise cause a circular dependency
with `Decl{,CXX}.h`.
Doing these operations into their own NFC patch helps the git rename
detection logic work, preserving the history.
This patch makes clang just a little slower to build (~0.17%), just
because it makes more code indirectly include `DeclCXX.h`.
This is a preparatory patch, to be able to provide inline definitions
for `Type` functions which depend on `Decl{,CXX}.h`. As the latter also
depends on `Type.h`, this would not be possible without some
reorganizing.
Splitting this rename into its own patch allows git to track this as a
rename, and preserve all git history, and not force any code
reformatting.
A later NFC patch will reintroduce `Type.h` as redirection to
`TypeBase.h`, rewriting most places back to directly including `Type.h`
instead of `TypeBase.h`, leaving only a handful of places where this is
necessary.
Then yet a later patch will exploit this by making more stuff inline.
Both these attributes were introduced in ab1dc2d54db5 ("Thread Safety
Analysis: add support for before/after annotations on mutexes") back in
2015 as "beta" features.
Anecdotally, we've been using `-Wthread-safety-beta` for years without
problems.
Furthermore, this feature requires the user to explicitly use these
attributes in the first place.
After 10 years, let's graduate the feature to the stable feature set,
and reserve `-Wthread-safety-beta` for new upcoming features.
This is a major change on how we represent nested name qualifications in
the AST.
* The nested name specifier itself and how it's stored is changed. The
prefixes for types are handled within the type hierarchy, which makes
canonicalization for them super cheap, no memory allocation required.
Also translating a type into nested name specifier form becomes a no-op.
An identifier is stored as a DependentNameType. The nested name
specifier gains a lightweight handle class, to be used instead of
passing around pointers, which is similar to what is implemented for
TemplateName. There is still one free bit available, and this handle can
be used within a PointerUnion and PointerIntPair, which should keep
bit-packing aficionados happy.
* The ElaboratedType node is removed, all type nodes in which it could
previously apply to can now store the elaborated keyword and name
qualifier, tail allocating when present.
* TagTypes can now point to the exact declaration found when producing
these, as opposed to the previous situation of there only existing one
TagType per entity. This increases the amount of type sugar retained,
and can have several applications, for example in tracking module
ownership, and other tools which care about source file origins, such as
IWYU. These TagTypes are lazily allocated, in order to limit the
increase in AST size.
This patch offers a great performance benefit.
It greatly improves compilation time for
[stdexec](https://github.com/NVIDIA/stdexec). For one datapoint, for
`test_on2.cpp` in that project, which is the slowest compiling test,
this patch improves `-c` compilation time by about 7.2%, with the
`-fsyntax-only` improvement being at ~12%.
This has great results on compile-time-tracker as well:
This patch also further enables other optimziations in the future, and
will reduce the performance impact of template specialization resugaring
when that lands.
It has some other miscelaneous drive-by fixes.
About the review: Yes the patch is huge, sorry about that. Part of the
reason is that I started by the nested name specifier part, before the
ElaboratedType part, but that had a huge performance downside, as
ElaboratedType is a big performance hog. I didn't have the steam to go
back and change the patch after the fact.
There is also a lot of internal API changes, and it made sense to remove
ElaboratedType in one go, versus removing it from one type at a time, as
that would present much more churn to the users. Also, the nested name
specifier having a different API avoids missing changes related to how
prefixes work now, which could make existing code compile but not work.
How to review: The important changes are all in
`clang/include/clang/AST` and `clang/lib/AST`, with also important
changes in `clang/lib/Sema/TreeTransform.h`.
The rest and bulk of the changes are mostly consequences of the changes
in API.
PS: TagType::getDecl is renamed to `getOriginalDecl` in this patch, just
for easier to rebasing. I plan to rename it back after this lands.
Fixes#136624
Fixes https://github.com/llvm/llvm-project/issues/43179
Fixes https://github.com/llvm/llvm-project/issues/68670
Fixes https://github.com/llvm/llvm-project/issues/92757
The FactManager managing the FactEntrys stays alive for the analysis of
a single function. We are already using that by allocating TIL
S-expressions via BumpPtrAllocator. We can do the same with FactEntrys.
If we allocate the facts in an arena, we won't get destructor calls for
them, and they better be trivially destructible. This required replacing
the SmallVector member of ScopedLockableFactEntry with TrailingObjects.
FactEntrys are now passed around by plain pointer. However, to hide the
allocation of TrailingObjects from users, we introduce `create` methods,
and this allows us to make the constructors private.
The point of reentrant capabilities is that they can be acquired
multiple times, so they should probably be excluded from requiring a
negative capability on acquisition via -Wthread-safety-negative.
However, we still propagate explicit negative requirements.
In ScopedLockableFactEntry::unlock(), we can avoid a second search,
pop_back(), and push_back() if we use the already obtained iterator into
the FactSet to replace the old FactEntry and take its position in the
vector.
These are identified by misc-include-cleaner. I've filtered out those
that break builds. Also, I'm staying away from llvm-config.h,
config.h, and Compiler.h, which likely cause platform- or
compiler-specific build failures.
Introduce the `reentrant_capability` attribute, which may be specified
alongside the `capability(..)` attribute to denote that the defined
capability type is reentrant. Marking a capability as reentrant means
that acquiring the same capability multiple times is safe, and does not
produce warnings on attempted re-acquisition.
The most significant changes required are plumbing to propagate if the
attribute is present to a CapabilityExpr, and introducing
ReentrancyDepth to the LockableFactEntry class.
The last use was removed by:
commit f8afb8fdedae04ad2670857c97925c439d47d862
Author: Aaron Puchert <aaron.puchert@sap.com>
Date: Fri Apr 29 22:12:21 2022 +0200
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 (instead of an
error) when the try_acquire_capability attribute is used on something
else than a function. The alternative would have been to produce an
error for the old spelling, but we seem to only warn for all function
attributes, so this is arguably more consistent.
The second change is that we also check the first argument (which is the
value returned for a successful try-acquire) for `this`. But from what I
can tell, this code is defunct anyway at the moment (see #31414).
Introduce `-Wthread-safety-pointer` to warn when passing or returning
pointers to guarded variables or guarded data. This is is analogous to
`-Wthread-safety-reference`, which performs similar checks for C++
references.
Adding checks for pointer passing is required to avoid false negatives
in large C codebases, where data structures are typically implemented
through helpers that take pointers to instances of a data structure.
The feature is planned to be enabled by default under `-Wthread-safety`
in the next release cycle. This gives time for early adopters to address
new findings.
Pull Request: https://github.com/llvm/llvm-project/pull/127396
Correctly analyze expressions where the address of a guarded variable is
taken and immediately dereferenced, such as (*(type-specifier *)&x).
Previously, such patterns would result in false negatives.
Pull Request: https://github.com/llvm/llvm-project/pull/127396
This is helpful when multiple functions operate on the same
capabilities, but we still want to use scoped lockable types for
readability and exception safety.
- Introduce support for thread safety annotations on function parameters
marked with the 'scoped_lockable' attribute.
- Add semantic checks for annotated function parameters, ensuring
correct usage.
- Enhance the analysis to recognize and handle parameters annotated for
thread safety, extending the scope of analysis to track these across
function boundries.
- Verify that the underlying mutexes of function arguments match the
expectations set by the annotations.
Limitation: This does not work when the attribute arguments are class
members, because attributes on function parameters are parsed
differently from attributes on functions.
This fixes false positives related to returning a scoped lockable
object. At the end of a function, we check managed locks instead of
scoped locks.
At real join points, we skip checking managed locks because we assume
that the scope keeps track of its underlying mutexes and will release
them at its destruction. So, checking for the scopes is sufficient.
However, at the end of a function, we aim at comparing the expected and
the actual lock sets. There, we skip checking scoped locks to prevent to
get duplicate warnings for the same lock.
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.
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
Extends the lifetime of the map `ConstructedObjects` to be of the
whole CFG so that the map can connect temporary Ctor and Dtor in
different CFG blocks.
…… (#68394)"
The new warnings are now under a separate flag
`-Wthread-safety-reference-return`, which is on by default under
`-Wthread-safety-reference`.
- People can opt out via `-Wthread-safety-reference
-Wnothread-safety-reference-return`.
This reverts commit 859f2d032386632562521a99db20923217d98988.
… (#67776)"
This detects issues in `scudo`. Reverting until these are fixed.
```
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd.h:74:12: error: returning variable 'QuarantineCache' by reference requires holding mutex 'Mutex' exclusively [-Werror,-Wthread-safety-reference]
74 | return QuarantineCache;
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:248:28: note: in instantiation of member function 'scudo::TSD<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::getQuarantineCache' requested here
248 | Quarantine.drain(&TSD->getQuarantineCache(),
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd.h:57:15: note: in instantiation of member function 'scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>::commitBack' requested here
57 | Instance->commitBack(this);
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:172:27: note: in instantiation of member function 'scudo::TSD<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::commitBack' requested here
172 | TSDRegistryT::ThreadTSD.commitBack(Instance);
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:33:46: note: in instantiation of function template specialization 'scudo::teardownThread<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>' requested here
33 | CHECK_EQ(pthread_key_create(&PThreadKey, teardownThread<Allocator>), 0);
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:42:5: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::init' requested here
42 | init(Instance); // Sets Initialized.
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:130:5: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::initOnceMaybe' requested here
130 | initOnceMaybe(Instance);
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:74:5: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::initThread' requested here
74 | initThread(Instance, MinimalInit);
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:221:17: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::initThreadMaybe' requested here
221 | TSDRegistry.initThreadMaybe(this, MinimalInit);
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:790:5: note: in instantiation of member function 'scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>::initThreadMaybe' requested here
790 | initThreadMaybe();
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/wrappers_c.inc:36:25: note: in instantiation of member function 'scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>::canReturnNull' requested here
36 | if (SCUDO_ALLOCATOR.canReturnNull()) {
```
This reverts commit 6dd96d6e80e9b3679a6161c590c60e0e99549b89.
...of guarded variables, when the function is not marked as requiring
locks:
```
class Return {
Mutex mu;
Foo foo GUARDED_BY(mu);
Foo &returns_ref_locked() {
MutexLock lock(&mu);
return foo; // BAD
}
Foo &returns_ref_locks_required() SHARED_LOCKS_REQUIRED(mu) {
return foo; // OK
}
};
```
Review on Phabricator: https://reviews.llvm.org/D153131
This patch uses castAs instead of getAs which will assert if the type doesn't match and adds nullptr check if needed.
Also this patch improves the codes and passes I.getData() instead of doing a lookup in dumpVarDefinitionName()
since we're iterating over the same map in LocalVariableMap::dumpContex().
Reviewed By: aaron.ballman, aaronpuchert
Differential Revision: https://reviews.llvm.org/D153033
