This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Hotfix for iterator checkers: Mark left-hand side of `SymIntExpr` objects as live in the program state maps.
ClosedPublic

Authored by baloghadamsoftware on Jun 29 2018, 4:23 AM.

Details

Summary

Marking a symbolic expression as live is not recursive. In our checkers we either use conjured symbols or conjured symbols plus/minus concrete integers to represent abstract positions of iterators, so we must mark the left-hand side of these expressions as live to prevent them from getting reaped.

Diff Detail

Event Timeline

NoQ added a comment.Jun 29 2018, 10:39 AM

That's right. You only need to mark "atomic" symbols (SymbolData) as live, and expressions that contain them would automatically become live. So i think you should just iterate through a symbol_iterator and mark all SymbolData symbols you encounter as live.

Is this a hotfix for a test failure? Otherwise it'd be great to have tests.

Updated according to the comments and assertions added to fail the tests without the fix.

george.karpenkov resigned from this revision.Jul 3 2018, 11:31 AM
george.karpenkov added a subscriber: george.karpenkov.
NoQ added a comment.Jul 11 2018, 4:09 PM

Looks good with minor comments.

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
496

Let's only mark SymbolData-type symbols as live; that should be enough.

1179

Typo: "be".

1181–1188

I believe this code should be reworked as follows:

  1. Subtract the operands using evalBinOp().
  2. Assume that the result doesn't overflow.
  3. Compare the result to 0.
  4. Assume the result of the comparison.

This should hopefully yield the same result without ever needing to introspect the SymExpr.

I suggest a FIXME.

1238

Also typo.

NoQ accepted this revision.Jul 13 2018, 5:59 AM

Looks good otherwise, please commit.

This revision is now accepted and ready to land.Jul 13 2018, 5:59 AM
This revision was automatically updated to reflect the committed changes.