Page MenuHomePhabricator

[Analyzer] Debug Checkers for Container and Iterator Inspection
Needs ReviewPublic

Authored by baloghadamsoftware on Wed, Sep 4, 5:26 AM.

Details

Reviewers
NoQ
Szelethus
Summary

For white-box testing correct container and iterator modelling it is essential to access the internal data structures stored for container and iterators. This patch introduces two simple debug checkers called debug.ContainerInspection and debug.IteratorInspection to achieve this.

Diff Detail

Event Timeline

I did not add debug calls to the tests of the existing iterator checkers yet, they will come in the next patch. After that I think the next step is to refactor the monolithic checker class into smaller ones: the iterator modelling into one file, and all the other checkers in small separate files. The modelling checker may also be split into two: container modelling and iterator modelling. Of course the is a connection between them, but they can be separated logically. All the common function must go into a library "module" (header and body file). However, the prerequisite for such a huge refactoring is a thorough whitebox testing.

Niiiiice! Thank you so much for sorting this out! I think this will make reviewing far more accessible for newcomers to the IteratorChecker family. I have a couple nits to comment on, but I won't clutter the code just yet. @NoQ, do you have any high level objections to this?

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
1689

Ah, right, so this would fire for clang_analyzer_iterator_position()?

NoQ added a comment.Fri, Sep 6, 5:06 PM

Yup, thanks, this is really nice!

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
243–244

We usually define such getters for stuff that the programmer cannot obtain otherwise during normal program execution. These two functions look like they're probably equivalent to normal .begin() and .end() calls. I don't really object but do we really ever need them other than for testing the trivial implementations of .begin() and .end()?

1659–1660

CallDescriptionMap please? ^.^

test/Analysis/iterator-inspection.cpp
41–42

Slightly more robust: clang_analyzer_eval(clang_analyzer_iterator_container(b0) == &v0); // expected-warning{{TRUE}}?

NoQ added inline comments.Fri, Sep 6, 5:07 PM
include/clang/StaticAnalyzer/Checkers/Checkers.td
1328–1337

Dunno, i would keep it all in one checker, just to save a few // RUN: lines :)

I'm sadly not knowledgeable enough with CallDescriptionMap, so let's have another round of review on this, otherwise, its perfect.

We talked about dividing this checker into multiple files, which would also make reviewing a bit easier. With that done, combined with this patch, I am very confident that we could enable parts of this checker by default by, well, you know, soon enough :^)

include/clang/StaticAnalyzer/Checkers/Checkers.td
1328–1337

I agree. Let's combine these into DebugIteratorModeling, because, as I understand it, that is what we're doing!

test/Analysis/iterator-inspection.cpp
1–2

Could you please format these? c:

38–43

Yea, I agree, let's add a testcase with a little more substance.

NoQ added a comment.Fri, Sep 6, 6:24 PM

I'm sadly not knowledgeable enough with CallDescriptionMap

D62557 contains a basic example.