https://github.com/ziqingluo-90 created
https://github.com/llvm/llvm-project/pull/178952
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
>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/4] [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/4] 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/4] 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/4] [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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
