This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Add missing state transition in IteratorChecker
ClosedPublic

Authored by rnkovacs on May 26 2018, 12:15 PM.

Diff Detail

Repository
rC Clang

Event Timeline

rnkovacs created this revision.May 26 2018, 12:15 PM
xazax.hun accepted this revision.May 26 2018, 12:17 PM
This revision is now accepted and ready to land.May 26 2018, 12:17 PM
MTC added a subscriber: MTC.May 27 2018, 7:08 PM
MTC added inline comments.
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
555

I have two questions may need @NoQ or @xazax.hun who is more familiar with the analyzer engine help to answer.

  • State may not change at all, do we need a check here like if (state != originalState)
  • A more basic problem is that do we need originalState = State trick. It seems that addTransitionImpl() has a check about same state transition, see addTransitionImp().

Thanks in advance!

NoQ added a comment.May 27 2018, 11:10 PM

Nice catch!

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
555

It seems that addTransitionImpl() has a check about same state transition, see addTransitionImp().

Yep, you pretty much answered your question. The check in the checker code is unnecessary.

NoQ accepted this revision.May 27 2018, 11:10 PM
MTC added inline comments.May 28 2018, 1:59 AM
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
555

Thanks, NoQ! It seems that if (state != originalState) in some checkers is misleading and may need to be cleaned up.

Oh, it slipped through somehow. Thanks for fixing this!

Did the tests execute? I am not sure. First problem is the the container may become dead before the iterator, so its Begin and End symbols may be inaccessible. This is easy to solve by marking the container of the iterator as live. However, there is a second problem that disables correct tracking of iterators: memory regions are marked as dead, however there are LazyCompoundVals referring to them. Is this maybe a bug in SymbolReaper?

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.