https://github.com/ziqingluo-90 updated https://github.com/llvm/llvm-project/pull/178952
>From 02dd8f8318f7cccd8f785b5abe5c9c2c866f1333 Mon Sep 17 00:00:00 2001 From: Ziqing Luo <[email protected]> Date: Thu, 29 Jan 2026 18:59:23 -0800 Subject: [PATCH 1/5] [Thread Safety Analysis] Fix a bug of context saving in alias-analysis The commit b4c98fcbe1504841203e610c351a3227f36c92a4 introduces alias-analysis. That commit also 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. This commit fixes the issue. --- clang/lib/Analysis/ThreadSafety.cpp | 5 ++- clang/test/Sema/warn-thread-safety-bug-fix.c | 36 ++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) create mode 100644 clang/test/Sema/warn-thread-safety-bug-fix.c diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index df4bae89f62df..d89429e104e3e 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -726,11 +726,10 @@ void VarMapBuilder::VisitCallExpr(const CallExpr *CE) { } } - if (VDec && Ctx.lookup(VDec)) { + if (VDec) Ctx = VMap->clearDefinition(VDec, Ctx); - VMap->saveContext(CE, Ctx); - } } + VMap->saveContext(CE, Ctx); } // Computes the intersection of two contexts. The intersection is the diff --git a/clang/test/Sema/warn-thread-safety-bug-fix.c b/clang/test/Sema/warn-thread-safety-bug-fix.c new file mode 100644 index 0000000000000..cedecaa7b8496 --- /dev/null +++ b/clang/test/Sema/warn-thread-safety-bug-fix.c @@ -0,0 +1,36 @@ +// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta %s + +typedef int __attribute__((capability("lock"))) lock_t; + +typedef struct { + lock_t lock; +} * map_t; + +typedef struct task { + map_t map; +} *task_t; + +#define ACQUIRES_LOCK(map) \ + __attribute__((acquire_capability((map)->lock))) +#define RELEASES_LOCK(map) \ + __attribute__((release_capability((map)->lock))) + +extern void lock_map(map_t map) ACQUIRES_LOCK(map); +extern void unlock_map_indirect(map_t *mapp) RELEASES_LOCK(*mapp); +extern void f(void *, void *, void *); + +static void saveContexBug(task_t task) +{ + map_t map; + map = task->map; + lock_map(map); // expected-note{{lock acquired here}} + map_t *mapp = ↦ + // Previously, a local-variable-definition-context was created and + // pushed for each of the argument below, resulting context + // mismatch. The analyzer missed the fact that 'mapp' may no + // longer point to the lock. So it does not report an issue at the + // 'unlock_map_indirect' call. + f(&map, &map, &mapp); + unlock_map_indirect(mapp); // expected-warning{{releasing lock 'mapp->lock' that was not held}} +} // expected-warning{{lock 'task->map->lock' is still held at the end of function}} + >From b9742a6924c3a0c39a652a489e77ae15c25a98ad Mon Sep 17 00:00:00 2001 From: Ziqing Luo <[email protected]> Date: Fri, 30 Jan 2026 11:13:45 -0800 Subject: [PATCH 2/5] Address comments --- clang/test/Sema/warn-thread-safety-bug-fix.c | 36 ------------------- .../SemaCXX/warn-thread-safety-analysis.cpp | 21 +++++++++++ 2 files changed, 21 insertions(+), 36 deletions(-) delete mode 100644 clang/test/Sema/warn-thread-safety-bug-fix.c diff --git a/clang/test/Sema/warn-thread-safety-bug-fix.c b/clang/test/Sema/warn-thread-safety-bug-fix.c deleted file mode 100644 index cedecaa7b8496..0000000000000 --- a/clang/test/Sema/warn-thread-safety-bug-fix.c +++ /dev/null @@ -1,36 +0,0 @@ -// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta %s - -typedef int __attribute__((capability("lock"))) lock_t; - -typedef struct { - lock_t lock; -} * map_t; - -typedef struct task { - map_t map; -} *task_t; - -#define ACQUIRES_LOCK(map) \ - __attribute__((acquire_capability((map)->lock))) -#define RELEASES_LOCK(map) \ - __attribute__((release_capability((map)->lock))) - -extern void lock_map(map_t map) ACQUIRES_LOCK(map); -extern void unlock_map_indirect(map_t *mapp) RELEASES_LOCK(*mapp); -extern void f(void *, void *, void *); - -static void saveContexBug(task_t task) -{ - map_t map; - map = task->map; - lock_map(map); // expected-note{{lock acquired here}} - map_t *mapp = ↦ - // Previously, a local-variable-definition-context was created and - // pushed for each of the argument below, resulting context - // mismatch. The analyzer missed the fact that 'mapp' may no - // longer point to the lock. So it does not report an issue at the - // 'unlock_map_indirect' call. - f(&map, &map, &mapp); - unlock_map_indirect(mapp); // expected-warning{{releasing lock 'mapp->lock' that was not held}} -} // expected-warning{{lock 'task->map->lock' is still held at the end of function}} - diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index d9efa745b7d59..92afb6e14c8de 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -7675,4 +7675,25 @@ void testLoopConditionalReassignment(Foo *f1, Foo *f2, bool cond) { f1->data = 42; ptr->mu.Unlock(); // expected-warning{{releasing mutex 'ptr->mu' that was not held}} } // expected-warning{{mutex 'f1->mu' is still held at the end of function}} + + +void unlock_Foo(Foo **Fp) __attribute__((release_capability((*Fp)->mu))); +// A function that may do anything to the objects referred to by the inputs: +void f(void *, void *, void *); + +static void saveContexBug(Foo *F) +{ + Foo *L; + L = F; + L->mu.Lock(); // expected-note{{mutex acquired here}} + Foo ** Fp = &L; + // Previously, a local-variable-definition-context was created and + // pushed for each of the argument below, resulting context + // mismatch. The analyzer missed the fact that 'mapp' may no + // longer point to the lock. So it does not report an issue at the + // 'unlock_map_indirect' call. + f(&L, &L, &Fp); + unlock_Foo(Fp); // expected-warning{{releasing mutex 'Fp->mu' that was not held}} +} // expected-warning{{mutex 'F->mu' is still held at the end of function}} + } // namespace CapabilityAliases >From 61e5d9170c339813554d8d661d97df23ed34669e Mon Sep 17 00:00:00 2001 From: Ziqing Luo <[email protected]> Date: Fri, 30 Jan 2026 11:29:28 -0800 Subject: [PATCH 3/5] Fix test comments --- clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 92afb6e14c8de..ce4cc3ec560df 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -7681,7 +7681,7 @@ void unlock_Foo(Foo **Fp) __attribute__((release_capability((*Fp)->mu))); // A function that may do anything to the objects referred to by the inputs: void f(void *, void *, void *); -static void saveContexBug(Foo *F) +void saveContexBug(Foo *F) { Foo *L; L = F; @@ -7689,9 +7689,9 @@ static void saveContexBug(Foo *F) Foo ** Fp = &L; // Previously, a local-variable-definition-context was created and // pushed for each of the argument below, resulting context - // mismatch. The analyzer missed the fact that 'mapp' may no + // mismatch. The analyzer missed the fact that 'Fp' may no // longer point to the lock. So it does not report an issue at the - // 'unlock_map_indirect' call. + // 'unlock_Foo' call. f(&L, &L, &Fp); unlock_Foo(Fp); // expected-warning{{releasing mutex 'Fp->mu' that was not held}} } // expected-warning{{mutex 'F->mu' is still held at the end of function}} >From e97ed1e98ce1aaa07420ea13b468ea63f337b170 Mon Sep 17 00:00:00 2001 From: Ziqing Luo <[email protected]> Date: Thu, 29 Jan 2026 19:32:20 -0800 Subject: [PATCH 4/5] [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. This commit fixes the issue. rdar://169236809 --- clang/lib/Analysis/ThreadSafety.cpp | 11 +++++++++-- clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 13 ++++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index d89429e104e3e..54d4e23e4c551 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1725,13 +1725,14 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> { FactSet FSet; // The fact set for the function on exit. const FactSet &FunctionExitFSet; + // Use `LVarCtx` to keep track of the context at each program point, which + // will be used to translate DREs (by `SExprBuilder::translateDeclRefExpr`) + // to their canonical definitions: LocalVariableMap::Context LVarCtx; unsigned CtxIndex; // To update and adjust the context. void updateLocalVarMapCtx(const Stmt *S) { - if (S) - LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx); if (!Analyzer->Handler.issueBetaWarnings()) return; // The lookup closure needs to be reconstructed with the refreshed LVarCtx. @@ -1739,6 +1740,12 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> { [this, Ctx = LVarCtx](const NamedDecl *D) mutable -> const Expr * { return Analyzer->LocalVarMap.lookupExpr(D, Ctx); }); + // The `setLookupLocalVarExpr` call above copies the context immediately + // before the statement `S`. That is the context that should be used during + // visiting `S`. Then we update `LVarCtx` to hold the context immediately + // after `S` for the next `Visit*` method. + if (S) + LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx); } // helper functions diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index ce4cc3ec560df..3971772d7973d 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -7695,5 +7695,16 @@ void saveContexBug(Foo *F) f(&L, &L, &Fp); unlock_Foo(Fp); // expected-warning{{releasing mutex 'Fp->mu' that was not held}} } // expected-warning{{mutex 'F->mu' is still held at the end of function}} - + +void useContextImmediatelyBeforeStmt(Foo* F) +{ + Foo* L; + L = F; + L->mu.Lock(); + // Previously, the local-variable-definition-context used for + // visiting the call below was the context immediately after the + // call, where 'L' has been invalidated. It became an false alarm + // when the call attempted to release 'L'. + unlock_Foo(&L); +} } // namespace CapabilityAliases >From f9fa5f63bf4fa85be0300dfac2310dc7c5b6c212 Mon Sep 17 00:00:00 2001 From: Ziqing Luo <[email protected]> Date: Thu, 29 Jan 2026 19:32:20 -0800 Subject: [PATCH 5/5] Address comments. Use the message below for the final commit message. [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 --- clang/lib/Analysis/ThreadSafety.cpp | 16 +++++++------- .../SemaCXX/warn-thread-safety-analysis.cpp | 21 +++++++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 54d4e23e4c551..944826a36d95b 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1733,19 +1733,21 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> { // To update and adjust the context. void updateLocalVarMapCtx(const Stmt *S) { + LocalVariableMap::Context OldCtx = LVarCtx; + + if (S) + LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx); if (!Analyzer->Handler.issueBetaWarnings()) return; // The lookup closure needs to be reconstructed with the refreshed LVarCtx. Analyzer->SxBuilder.setLookupLocalVarExpr( - [this, Ctx = LVarCtx](const NamedDecl *D) mutable -> const Expr * { + [this, Ctx = OldCtx](const NamedDecl *D) mutable -> const Expr * { return Analyzer->LocalVarMap.lookupExpr(D, Ctx); }); - // The `setLookupLocalVarExpr` call above copies the context immediately - // before the statement `S`. That is the context that should be used during - // visiting `S`. Then we update `LVarCtx` to hold the context immediately - // after `S` for the next `Visit*` method. - if (S) - LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx); + // The `setLookupLocalVarExpr` call above copies the old context, the one + // immediately before the statement `S`. This context will be used in + // visiting `S`. The updated `LVarCtx` holds the context immediately after + // `S`, which is for the next `Visit*` method. } // helper functions diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 3971772d7973d..6342314e85ce8 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -7434,6 +7434,27 @@ void testPointerAliasEscapeAndReset(Foo *f) { ptr->mu.Unlock(); } +// A function that may do anything to the objects referred to by the inputs. +void escapeAliasMultiple(void *, void *, void *); +void testPointerAliasEscapeMultiple(Foo *F) +{ + Foo *L; + F->mu.Lock(); // expected-note{{mutex acquired here}} + Foo *Fp = F; + escapeAliasMultiple(&L, &L, &Fp); + Fp->mu.Unlock(); // expected-warning{{releasing mutex 'Fp->mu' that was not held}} +} // expected-warning{{mutex 'F->mu' is still held at the end of function}} + +void unlockFooWithEscapablePointer(Foo **Fp) __attribute__((release_capability((*Fp)->mu))); +void testEscapeInvalidationHappensRightAfterTheCall(Foo* F) +{ + Foo* L; + L = F; + L->mu.Lock(); + // Release the lock hold by 'L' before clear its definition. + unlockFooWithEscapablePointer(&L); +} + void testPointerAliasTryLock1() { Foo *ptr = returnsFoo(); if (ptr->mu.TryLock()) { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
