This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.
ClosedPublic

Authored by NoQ on Dec 14 2017, 5:17 PM.

Details

Summary

Default global operator new(), like malloc(), should return heap pointers, which in the analyzer are represented by SymbolicRegions with HeapSpaceRegion as their parent.

In the -analyzer-config c++-allocator-inlining mode, this was broken, and regular SymbolicRegions were returned instead, which have UnknownSpaceRegion as their parent.

This patch fixes this straightforwardly on ExprEngine side. We may want to delegate this job to the checkers though evalCall, but for now this mode doesn't support evalCall for operator new(), and i'm not sure if it'd be used much.

With this patch going on top of previous patches, enabling c++-allocator-inlining by default causes no regressions on tests (causes some improvements though). It doesn't mean it works (we still have callbacks broken, path diagnostic pieces unsupported, and i've just noticed one more void element region crash), just a psychological checkpoint.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Dec 14 2017, 5:17 PM
xazax.hun accepted this revision.Dec 16 2017, 9:51 AM

In the future, we might want to model the standard placement new and return a symbol with the correct SpaceRegion (i.e.: the space region of the argument).

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
523

The parens are redundant here.

This revision is now accepted and ready to land.Dec 16 2017, 9:51 AM
NoQ updated this revision to Diff 127560.Dec 19 2017, 10:33 AM

Rebase.

Remove the redundant cast that is done in c++-allocator-inlining mode when modeling array new. After rebase it started causing two identical element regions top appear on top of each other.

dcoughlin added inline comments.Jan 8 2018, 6:31 PM
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
513

I realize this isn't your code, but could we use FunctionDecl::isReplaceableGlobalAllocationFunction() here?

NoQ updated this revision to Diff 129213.Jan 9 2018, 7:37 PM

Rebase. Address the comment.

In D41266#959763, @NoQ wrote:

Remove the redundant cast that is done in c++-allocator-inlining mode when modeling array new. After rebase it started causing two identical element regions top appear on top of each other.

Remove this workaround because i reverted the change for which it is a workaround in D41250#971888: we no longer add the redundant ElementRegion, so we don't need to defend from double ElementRegions here. I guess it all finally starts to make sense.

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
513

Hmm. Totally. That's much better.

NoQ updated this revision to Diff 129367.Jan 10 2018, 4:19 PM

Rebase (fix minor conflict).

dcoughlin accepted this revision.Jan 12 2018, 4:59 PM

LGTM as well.

This revision was automatically updated to reflect the committed changes.