This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Introduce weak dependencies to express *preferred* checker callback evaluation order
ClosedPublic

Authored by Szelethus on May 31 2020, 7:25 PM.

Details

Summary

Checker dependencies were added D54438 to solve a bug where the checker names were incorrectly registered, for example, InnerPointerChecker would incorrectly emit diagnostics under the name MallocChecker, or vice versa [1]. Since the system over the course of about a year matured, our expectations of what a role of a dependency and a dependent checker should be crystallized a bit more -- D77474 and its summary, as well as a variety of patches in the stack demonstrates how we try to keep dependencies to play a purely modeling role. In fact, D78126 outright forbids diagnostics under a dependency checkers name.

These dependencies ensured the registration order and enabling only when all dependencies are satisfied. This was a very "strong" contract however, that doesn't fit the dependency added in D79420. As its summary suggests, this relation is directly in between diagnostics, not modeling -- we'd prefer a more specific warning over a general one.

To support this, I added a new dependency kind, weak dependencies. These are not as strict of a contract, they only express a preference in registration order. If a weak dependency isn't satisfied, the checker may still be enabled, but if it is, checker registration, and transitively, checker callback evaluation order is ensured.

If you are not familiar with the TableGen changes, a rather short description can be found in the summary of D75360. A lengthier one is in D58065. Of course, this is a rather rarely touched part of the analyzer and I'm happy to spill the beans if any part of it is unclear!

[1] https://www.youtube.com/watch?v=eqKeqHRAhQM

Diff Detail

Event Timeline

Szelethus created this revision.May 31 2020, 7:25 PM
NoQ added a reviewer: vsavchenko.EditedJun 1 2020, 4:12 AM

I think this patch is a fairly historic moment to celebratte.

checker callback evaluation order is ensured

And it will be the same for all callbacks, right? Like, we're not doing this thing yet where it intentionally goes like

chk1->checkPreCall();
chk2->checkPreCall();
chk2->checkPostCall();
chk1->checkPostCall();

?

clang/include/clang/StaticAnalyzer/Checkers/CheckerBase.td
144–145

I wouldn't mind having predictable callback evaluation order for modeling as well. What's causing you to drop this scenario?

checker callback evaluation order is ensured

And it will be the same for all callbacks, right? Like, we're not doing this thing yet where it intentionally goes like

chk1->checkPreCall();
chk2->checkPreCall();
chk2->checkPostCall();
chk1->checkPostCall();

?

It should work for all callbacks. It'd be a nightmare to test and assert, but when CheckerManager registers a checker, all of its callbacks are registered to the appropriate container, which isn't sorted.

NoQ added a reviewer: vsavchenko.

Woops O:)

Szelethus added a comment.EditedJun 1 2020, 6:21 AM

Mind that strong dependencies also ensure the order of evaluation, but that is a guarantee (either the dependency will be evaluated before the dependency, or none of them will be evaluated), and this patch is about preference (if both of them are enabled, the dependency will be evaluated first).

+Nithin because it may be relevant to the smart pointer checker if inter-checker communication turns out to be necessary (eg., with the move checker).

In D80905#2068320, @NoQ wrote:

+Nithin because it may be relevant to the smart pointer checker if inter-checker communication turns out to be necessary (eg., with the move checker).

If checkers rely on one another, weak dependencies aren't the answer, but rather the guarantee strong dependencies grant. Weak dependencies could be useful however if reports from the checker would be hidden by, say, NullDereferenceChecker.

Szelethus marked 2 inline comments as done.Jun 8 2020, 4:49 AM

Gentle ping

clang/include/clang/StaticAnalyzer/Checkers/CheckerBase.td
144–145
NoQ added inline comments.Jun 8 2020, 5:17 AM
clang/include/clang/StaticAnalyzer/Checkers/CheckerBase.td
144–145

Ok, so do i understand it correctly that the only reason you're omitting the "and modeling" clause here is because in your ultimate vision (which i completely agree with) checkers either do modeling or checking, and modeling-checkers never need to be disabled to begin with, so weak dependencies are useless for them, which is why this whole patch is entirely about diagnostics?

Szelethus marked 3 inline comments as done.Jun 8 2020, 5:40 AM
Szelethus added inline comments.
clang/include/clang/StaticAnalyzer/Checkers/CheckerBase.td
144–145

Pretty much, yes. Its not just useless -- I don't think establishing a preferred evaluation order over guaranteed for modeling checkers could ever be correct. Not only that, I think adding new kinds of dependencies must be very well justified to counterweight the extra complexity, they should be intuitive and "just work". There would be a lot of corner cases if these dependencies could be too intertwined, and not only would they be a mess to implement, it could be confusing on the checker developer's end.

martong accepted this revision.Jun 8 2020, 6:09 AM

Thank you Kristof for working on this. Looks good to me as it is!

This revision is now accepted and ready to land.Jun 8 2020, 6:09 AM
Szelethus marked an inline comment as done.Jun 9 2020, 7:23 AM
This revision was automatically updated to reflect the committed changes.