Page MenuHomePhabricator

[analyzer] Track class member initializer constructors path-sensitively within their construction context.
ClosedPublic

Authored by NoQ on May 24 2018, 4:45 PM.

Details

Summary

Same as D47305 but for CXXCtorInitializer-based constructors, based on the discussion in http://lists.llvm.org/pipermail/cfe-dev/2018-May/058055.html

Because these don't suffer from liveness bugs (member variables are born much earlier than their constructors are called and live much longer than stack locals), this is mostly a refactoring pass. It has minor observable side effects, but it will have way more visible effects when we enable C++17 construction contexts because finding the direct constructor would be much harder.

Currently the observable effect is that we can preserve direct bindings to the object more often and we need to fall back to binding the lazy compound value of the initializer expression less often. Direct bindings are modeled better by the store. In the provided test case the default binding produced by trivial-copying s to t.s would overwrite the existing default binding to t. But if we instead preserve direct bindings, only bindings that correspond to t.s will be overwritten and the binding for t.w will remain untouched. This only works for C++17 because in C++11 the member variable is still modeled as a trivial copy from the temporary object, because there semantically *is* a temporary object, while in C++17 it is elided.

So we finally get rid of findDirectConstructorForCurrentCFGElement() which is a CFG look-behind hack. We pay the price of making our trait's key more complex; we should have probably just added state trait specializations for PointerUnion instead.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.May 24 2018, 4:45 PM
NoQ updated this revision to Diff 148502.May 24 2018, 4:48 PM

Add a forgotten FIXME to the test.

george.karpenkov requested changes to this revision.May 30 2018, 11:50 AM

requesting changes due to minor nits inline

lib/StaticAnalyzer/Core/ExprEngine.cpp
161

newline after the brace. here and elsewhere within this class.

170

would it be faster to store a pair in the first place, instead of constructing it on each comparison? Is this operator required to be in a GDM?

771

gotta love state variables used 70 lines later on. Could we add a comment here? What is the semantics of the cleanup action? Running destructors? Maybe a better name would make this more readable?

Also would it be worse to change finishObjectConstruction function to not do anything when there's no object being constructed, and then we could simply call it on all code paths?

This revision now requires changes to proceed.May 30 2018, 11:50 AM
NoQ updated this revision to Diff 149205.May 30 2018, 2:34 PM
NoQ marked 3 inline comments as done.

Refactor everything to address review comments.

george.karpenkov accepted this revision.May 30 2018, 4:17 PM

LGTM, but I would really prefer if you could split that inheritance back into composition.

lib/StaticAnalyzer/Core/ExprEngine.cpp
123

sorry I find that very confusing, and I think the previous version was better (prefer composition over inheritance)

This revision is now accepted and ready to land.May 30 2018, 4:17 PM
NoQ added a comment.EditedMay 30 2018, 4:23 PM
This comment has been deleted.
lib/StaticAnalyzer/Core/ExprEngine.cpp
123

Ok, so, i mean, i don't think it would be faster, i really hope the compiler would know how to inline std::pair methods. I think converting to a std::pair or std::tuple is a fairly standard trick to avoid x < rhs.x || (x == rhs.x && y < rhs.y) kind of code. But with inheritance we at least have boilerplate operators sorted out for free.

May i leave the actual previous version around without fixing it?^^

NoQ updated this revision to Diff 149337.May 31 2018, 12:11 PM

Hmm, actually composition looks very pretty if you use the magic word "impl".

This revision was automatically updated to reflect the committed changes.