Page MenuHomePhabricator

[analyzer] Model base to derived casts more precisely.
ClosedPublic

Authored by xazax.hun on Aug 1 2016, 7:27 AM.

Details

Summary

Dynamic casts are handled relatively well by the static analyzer. BaseToDerived casts however are handled conservatively. This can cause some false positives with the NewDeleteLeaks checker.

This patch alters the behavior of BaseToDerived casts. In case a dynamic cast would succeed use the same semantics. Otherwise fall back to the conservative approach.

A slightly related question: in case a cast on a pointer can not be modelled precisely, maybe that should be handled as a pointer escape to avoid false positives in some cases?

Diff Detail

Repository
rL LLVM

Event Timeline

xazax.hun updated this revision to Diff 66315.Aug 1 2016, 7:27 AM
xazax.hun retitled this revision from to [analyzer] Model base to derived casts more precisely..
xazax.hun updated this object.
xazax.hun added a subscriber: cfe-commits.
xazax.hun added inline comments.Aug 1 2016, 7:43 AM
test/Analysis/NewDelete-checker-test.cpp
394 ↗(On Diff #66315)

Before the modification the analyzer reports a leak here, since the symbol returned by the BaseToDerived cast is independent of the original symbol.

NoQ added a subscriber: NoQ.Aug 1 2016, 10:16 AM
NoQ added inline comments.Aug 2 2016, 6:32 AM
lib/StaticAnalyzer/Core/ExprEngineC.cpp
423 ↗(On Diff #66315)

I guess if val is a non-zero constant, it wouldn't make much difference.

xazax.hun added inline comments.Aug 2 2016, 6:35 AM
lib/StaticAnalyzer/Core/ExprEngineC.cpp
423 ↗(On Diff #66315)

I might be wrong, but isn't the only valid constant value for a pointer the zero constant?

NoQ added inline comments.Aug 2 2016, 7:01 AM
lib/StaticAnalyzer/Core/ExprEngineC.cpp
423 ↗(On Diff #66315)

Even if forbidden by the Standard in well-formed programs, we'd have to expect it here - after all, it's great if we analyze a program that has a bug :)

We also have this FixedAddressChecker thing, so even in our simplified model, there actually do exist non-zero constant pointers. That said, there's some bug i stepped into in explain-svals.cpp, which produces more zero constant pointers that one would expect, didn't have time to investigate yet...

dcoughlin edited edge metadata.Aug 2 2016, 9:02 AM

Other than a naming/documentation suggestion, looks good to me. Thanks Gábor!

lib/StaticAnalyzer/Core/ExprEngineC.cpp
424 ↗(On Diff #66315)

It seems a bit weird to call a method named "evalDynamicCast" when handling a static cast. What do you think about renaming evalDynamicCast() to to attemptDowncast() and updating its documentation to reflect its now-dual use?

xazax.hun updated this revision to Diff 66632.Aug 3 2016, 3:07 AM
xazax.hun edited edge metadata.
xazax.hun marked 4 inline comments as done.
  • Improvements according to review comments.
NoQ accepted this revision.Aug 3 2016, 7:57 AM
NoQ added a reviewer: NoQ.
This revision is now accepted and ready to land.Aug 3 2016, 7:57 AM

What do you think about escaping pointers that gone through conservatively evaluated casts?

This revision was automatically updated to reflect the committed changes.