llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-analysis
Author: Ziqing Luo (ziqingluo-90)
<details>
<summary>Changes</summary>
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
---
Full diff: https://github.com/llvm/llvm-project/pull/178952.diff
2 Files Affected:
- (modified) clang/lib/Analysis/ThreadSafety.cpp (+11-5)
- (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+32)
``````````diff
diff --git a/clang/lib/Analysis/ThreadSafety.cpp
b/clang/lib/Analysis/ThreadSafety.cpp
index df4bae89f62df..54d4e23e4c551 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
@@ -1726,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.
@@ -1740,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 d9efa745b7d59..3971772d7973d 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -7675,4 +7675,36 @@ 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 *);
+
+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 'Fp' may no
+ // longer point to the lock. So it does not report an issue at the
+ // '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}}
+
+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
``````````
</details>
https://github.com/llvm/llvm-project/pull/178952
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
