Page MenuHomePhabricator

[Analyzer] Model `empty()` member function of containers
Needs ReviewPublic

Authored by baloghadamsoftware on Mar 23 2020, 12:07 AM.

Details

Reviewers
NoQ
Szelethus
Summary

Modeling the empty() method of containers both improves iterator range checking and enables the possibility to create new checkers for containers.

Diff Detail

Event Timeline

baloghadamsoftware marked 3 inline comments as done.Mar 23 2020, 12:14 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
492

Maybe I should move these lines into a separate function in the library to avoid repetition.

511–512

This is way longer than the previously used processComparison() but I am reluctant to pass CheckerContext to a non-member function. (And especially add transitions there. Even worse, to a function in a common library.)

martong added inline comments.Mar 23 2020, 8:17 AM
clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
34–75

Perhaps it would be easier to read these functions if we had a documentation for their parameters. Maybe we could have some of them documented in one comment since many seem to have similar params.
Or maybe just renaming Cont to Container would help also.

56

I understand that the container SVal is no longer const, but why did the iterator loose its constness?

85–86

Just a nit, if we are at counting below (one, two) then perhaps Zero would be more coherent with the other typedefs?

270–271

Why do we skip the const qualifiers here?

432

Is there really no sense to continue the analysis here? In what state we are here exactly? Is there an inconsistency between the begin , end and the return value?

clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
21

Docs, docs, docs.

312

Perhaps we should guard Op with an assert, I mean it should be either == or !=.

314

Could we spare the if with just keeping the declaration of State and then

return std::make_pair(State, nullptr);

?

315–316

This is hard to read for me. Also, Isn't it enough just to write TruthVal->getValue() ?

328

Why do we assume here again? We already had an assume in relateSymbols, hadn't we?

339

Please explain in the comment what is the problem with the current impl.

351–352

Could we merge the declaration of the new state into the parens of the if?

clang/lib/StaticAnalyzer/Checkers/Iterator.h
182

Documentation would be helpful.

clang/test/Analysis/container-modeling.cpp
19

It seems like we don't use it anywhere.

martong added inline comments.Mar 23 2020, 8:42 AM
clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
335

Would assumeEqual be a better name?

Rebased, updated.

baloghadamsoftware marked 2 inline comments as done.Aug 4 2020, 6:55 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
56

We passed it earlier as a constant reference, but SVal is small, it should be passed by value.

270–271

Do you mean for SVal? See above.

432

Yes, see the comment I added. If data about emptiness based on the difference of begin and and and the return value of empty() contradict each other then we are on an impossible branch.

clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
21

This function is moved here, it is nothing new.

312

Actually, this works for any comparison operator. So it is enough to assume that Op is a comparison operator.

328

In relateSymbols we assumed the comparison of the two symbols. Here we assume the RetVal. The new states are nullptr if we cannot assume the same for both.

335

This method is just moved from another source file.

339

Ditto. The community requested a future refactoring.

351–352

No, because we use it later in the function. (See below.)

clang/test/Analysis/container-modeling.cpp
19

We use this in the definition of the assert() macro. And we use that macro in the new test cases.

Updated according to the comments.

baloghadamsoftware marked 2 inline comments as done.

Minor fix.

baloghadamsoftware marked an inline comment as done.Aug 4 2020, 7:12 AM

isIterator() updated because it did not work perfectly with the refactored handleBegin() after rebase. (Why did it work before the rebase?) The problem was that it only looked for the necessary operators in the actual class but not in its bases.

I'm in favor of most, if not all of the changes, though I will admit that this patch seems pretty cluttered, you are doing a lot of refactoring under the same hood. You're moving, adding, removing and changing helper functions and their invocations. Would be possible to make this patch a bit leaner?

clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
36

Hmm, this was changed to an optional, unnamed parameter without docs... Might be a bit cryptic :) Also, this seems to be orthogonal to the patch, is it not? Does the modeling of empty() change something that affects this function?

420

assumpotions > assumptions

420–427

You will have to help me out here -- if the analyzer couldn't return a sensible symbol, is it okay to just create one? When does UnknownVal even happen, does it ever happen? Also, if we're doing this anyways, wouldn't using evalCall be more appropriate?

clang/unittests/StaticAnalyzer/TestReturnValueUnderConstruction.cpp
71 ↗(On Diff #289981)

Did you mean to upload changed to this file from D85351 to this patch as well?

Wrong diff uploaded previously. (Accidentally compared to master instead of the prerequisite.)

baloghadamsoftware marked an inline comment as done.Sep 7 2020, 5:01 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
36

We discussed with @NoQ a few patches earlier that UnknownVal is not a nullptr-like value for SVals. That time I changed this bad practice in my code everywhere, only forgot this particular place. I do not think that this deserves its own patch, it is really a tiny thing.

420–427

Actually, both UnknownVal and SymbolVal containing a SymbolConjured without constraints are unknown. The main difference (for us) is that we cannot assign constraints to UnknownVal but we can for SymbolConjured. This is the reason for replacing it. However, this is not new. We do it in comparison too. The best would be to change the infrastructure to never return UnknownVal but conjure a new symbol instead if we known nothing about the return value. Maybe I will put a FIXME there. evalCall() is not an option, because we do not want to lose the possibility that the implementation of empty() is inlined by the engine.

clang/unittests/StaticAnalyzer/TestReturnValueUnderConstruction.cpp
71 ↗(On Diff #289981)

No, I just uploaded the wrong diff here.