================
@@ -3648,35 +3648,38 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // See if we're releasing memory while inlining a destructor
- // (or one of its callees). This turns on various common
- // false positive suppressions.
- bool FoundAnyDestructor = false;
- for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
- if (const auto *DD = dyn_cast<CXXDestructorDecl>(LC->getDecl())) {
- if (isReferenceCountingPointerDestructor(DD)) {
- // This immediately looks like a reference-counting destructor.
- // We're bad at guessing the original reference count of the
object,
- // so suppress the report for now.
- BR.markInvalid(getTag(), DD);
- } else if (!FoundAnyDestructor) {
- assert(!ReleaseDestructorLC &&
+ // See if we're releasing memory while inlining a destructor or
+ // functions that decrement reference counters (or one of its callees).
+ // This turns on various common false positive suppressions.
+ bool FoundAnyReleaseFunction = false;
+ for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ if (const auto *DD = dyn_cast<CXXDestructorDecl>(LC->getDecl())) {
+ if (isReferenceCountingPointerDestructor(DD)) {
+ // This immediately looks like a reference-counting destructor.
+ // We're bad at guessing the original reference count of the
+ // object, so suppress the report for now.
+ BR.markInvalid(getTag(), DD);
+ continue;
+ }
+ }
+
+ if (!FoundAnyReleaseFunction) {
----------------
NagyDonat wrote:
The trickery with this boolean `FoundAny` is just useless cruft now, because
`ReleaseFunctionLC` will always be equal to `CurrentLC` (unless we hit the
`continue` after the `markInvalid()` call, but that case is irrelevant).
Previously the `for` loop was walking upwards on the call stack, looking for
the innermost destructor call that contained the `free()` call -- now you
immediately bind `ReleaseFunctionLC` in the first iteration of the loop.
This means that while your change can suppress more reports in situation where
the call stack doesn't contain a destructor, it can also reduce the amount of
suppression in cases where instead of a destructor stack frame it will look for
the atomic ops only in the stack frame that directly contains the `free()`
This means that the suppression will no longer work for something like
```cpp
SmartPointr::~SmartPointr() {
if (__c11_atomic_fetch_sub(refcount, 1, memory_order_relaxed) == 1)
release_resources();
}
void SmartPointr::release_resources() {
free(buffer);
// probably some additional cleanup happens here
}
```
because the old code would've set `ReleaseDestructorLC` to the stack frame of
the destructor `~SmartPointr()` (and finds that it contains an atomic
subtraction call), while the new code sets `ReleaseFunctionLC` to
`release_resources()` which presumably doesn't use atomic add/sub.
(By the way, please add a testcase that tests this situation -- I'm a bit
surprised that we don't already have a test like this. Note that I used the
distorted name `SmartPointr` to avoid the other heuristic which checks the
class name.)
I think this loss of suppression may be "acceptable collateral damage" for your
change, but you should think about it and perhaps consider alternative
solutions. (E.g. you could initialize `ReleaseFunctionLC` to `CurrentLC`, but
then if you find a destructor on the stack, then you switch to using that
(innermost) destructor as `ReleaseFunctionLC`.)
https://github.com/llvm/llvm-project/pull/104599
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
