Page MenuHomePhabricator

[Analyzer] Debug Checkers for Container and Iterator Inspection

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



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?

1689 ↗(On Diff #218657)

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

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

Yup, thanks, this is really nice!

243–244 ↗(On Diff #218657)

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 ↗(On Diff #218657)

CallDescriptionMap please? ^.^

41–42 ↗(On Diff #218657)

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

NoQ added inline comments.Sep 6 2019, 5:07 PM
1328–1337 ↗(On Diff #218657)

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 :^)

1328–1337 ↗(On Diff #218657)

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

1–2 ↗(On Diff #218657)

Could you please format these? c:

38–43 ↗(On Diff #218657)

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

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

I'm sadly not knowledgeable enough with CallDescriptionMap

D62557 contains a basic example.

Updated according to the comments.

baloghadamsoftware marked 6 inline comments as done.Oct 3 2019, 4:16 AM
baloghadamsoftware added inline comments.
1328–1337 ↗(On Diff #218657)

I did it on the short term, but on the long term iterator and container modelling are planned to be two different things, where iterator modelling depends on container modelling, but the latter works alone as well to support container checkers, such as e.g. copying data into a container with insufficient size. Furthermore I do not really like the name debug.DebugIteratorModeling.

243–244 ↗(On Diff #218657)

Not exactly. These functions return the internal representation of their .begin() and .end(). These symbols are conjured by the iterator checker and are bound to the return value of .begin() and .end() in the container data map. It is an extra check to check whether they return the same internal symbol as clang_analyzer_iterator_position() for the return value of .begin() and .end().

1689 ↗(On Diff #218657)

Yes, and also for the other two.

Szelethus accepted this revision.Oct 11 2019, 8:33 AM

This is amazing, thanks!! LGTM.

This revision is now accepted and ready to land.Oct 11 2019, 8:33 AM
This revision was automatically updated to reflect the committed changes.
baloghadamsoftware marked an inline comment as done.