This revision was automatically updated to reflect the committed changes.
Closed by commit rC347953: [analyzer] Fix the Zombie Symbols bug.
(authored by dergachev, committed by ).
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D18860/new/
NoQ added a comment.
> I'd very strongly prefer to have this goldnugget copy-pasted even to a simple
> txt file, and have it commited with this patch.
I guess i'll do it a bit later because it needs to be cleaned up after the
behavior changes with this patch. There are a few other such
Szelethus added a comment.
Let's also have the link to your most recent mail about this issue here:
http://lists.llvm.org/pipermail/cfe-dev/2018-November/060038.html
I re-read the mailing list conversation from 2016, @szepet's
`MisusedMoveChecker` patch, so I have a better graps on whats
Szelethus added a comment.
In https://reviews.llvm.org/D18860#1297742, @NoQ wrote:
> Nope, unit tests aren't quite useful here. I can't unit-test the behavior of
> API that i'm about to //remove//... right?
Hmmm, can't really argue with that, can I? :D
https://reviews.llvm.org/D18860
NoQ added a comment.
Nope, unit tests aren't quite useful here. I can't unit-test the behavior of
API that i'm about to //remove//... right?
https://reviews.llvm.org/D18860
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
NoQ updated this revision to Diff 173932.
NoQ added a comment.
In https://reviews.llvm.org/D18860#1284805, @NoQ wrote:
> `RetainCountChecker` is affected, as demonstrated by the newly added test.
`RetainCountChecker` is affected due to temporary insanity in the checker that
was induced by
NoQ added a comment.
The manual region cleanup is removed from `MisusedMovedObjectChecker` in
https://reviews.llvm.org/D54372.
> Another thing, since we're directly testing the functionality of
> `SymbolReaper`, maybe it'd be worth to also include unit tests.
Sounds like a great idea, and/or
NoQ updated this revision to Diff 173487.
NoQ added a comment.
Add an interesting test for the `MisusedMovedObject` checker that demonstrates
one more potential source of false positives caused by the zombie symbol
problem. In this test there are, well, //no symbols//. Therefore, there are no
Szelethus added a comment.
I read through your patch multiple times, have read your mail on cfe-dev, and I
still couldn't find eve minor nits, well done!
However, both in the mail, and in this discussion, there are a lot of
invaluable information about the inner workings of the analyzer, that
NoQ added a comment.
Just to be clear - this newly found problem is also automatically fixed by that
patch.
https://reviews.llvm.org/D18860
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
NoQ updated this revision to Diff 172263.
NoQ added a comment.
Ha! Apart from Zombie symbols that are neither dead nor alive and therefore not
buried properly, there are also Schrödinger symbols that are dead and alive at
the same time. Moreover, asking whether a Schrödinger symbol is
NoQ added inline comments.
Comment at: test/Analysis/simple-stream-checks.c:96
+ fp = 0;
+} // expected-warning {{Opened file is never closed; potential resource leak}}
george.karpenkov wrote:
> Woo-hoo, were we losing this case before?
Yes, this is the whole
george.karpenkov accepted this revision.
george.karpenkov added inline comments.
This revision is now accepted and ready to land.
Comment at: test/Analysis/self-assign.cpp:42
str = rhs.str;
- rhs.str = nullptr; // FIXME: An improved leak checker should warn here
+ rhs.str =
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/Environment.cpp:180
- for (; SI != SE; ++SI)
-SymReaper.maybeDead(*SI);
}
zaks.anna wrote:
> We are removing this because the maybeDead is no longer used, correct?
Yup.
NoQ updated this revision to Diff 171006.
NoQ marked 2 inline comments as done.
NoQ added a comment.
Herald added subscribers: dkrupp, donat.nagy, Szelethus, mikhail.ramalho.
Rebase!
Changes on large codebase runs still look mild and reasonable, with much less
slowdown than before. Even if
george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: baloghadamsoftware.
The change LGTM after nits and rebasing IF the new evaluation will not show a
too large regression.
george.karpenkov added a comment.
@NoQ can we get the evaluation re-done?
https://reviews.llvm.org/D18860
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
xazax.hun added a comment.
What is the status of this patch? Is there a consensus?
https://reviews.llvm.org/D18860
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
18 matches
Mail list logo
