Page MenuHomePhabricator

[analyzer] Fix modeling of ctors
ClosedPublic

Authored by alexshap on Aug 17 2017, 4:19 PM.

Details

Summary

This diff attempts to fix analyzer's crash (triggered assert) on the newly added test case.
The assert being discussed is assert(!B.lookup(R, BindingKey::Direct)) in lib/StaticAnalyzer/Core/RegionStore.cpp,
however the root cause appears to be different.
For classes with empty bases the offsets might be tricky.
For example, let's assume we have

struct S: NonEmptyBase, EmptyBase {
   ...
};

In this case Clang applies empty base class optimization and the offset of EmptyBase will be 0
(it can be verified via clang -cc1 -x c++ -v -fdump-record-layouts main.cpp -emit-llvm -o /dev/null).
When the analyzer tries to do zero initialization of EmptyBase it will hit the assert because that region has already been
"written" by the constructor of NonEmptyBase.

Test plan:
make check-all

Diff Detail

Event Timeline

alexshap created this revision.Aug 17 2017, 4:19 PM
alexshap edited the summary of this revision. (Show Details)Aug 17 2017, 4:21 PM
dcoughlin edited edge metadata.Aug 17 2017, 4:45 PM

Thanks for the patch!

@NoQ and I were discussing this approach this morning. One alternative we discussed was performing this logic in RegionStore instead and skipping the default binding there if we saw that the base region was empty. What do you think of that approach? (We would have to be careful for exactly the reasons described in your FIXME).

>One alternative we discussed was performing this logic in RegionStore instead and skipping the default binding there 
>if we saw that the base region was empty. What do you think of that approach? (We would have to be careful for exactly the reasons described in your FIXME)

yeah, i thought about this option as well, i can update my diff to try that and see how it works.

alexshap updated this revision to Diff 111622.Aug 18 2017, 12:32 AM

Skip the default binding for empty bases.

NoQ accepted this revision.Aug 18 2017, 1:25 AM

Yeah, thanks!

After the FIXME about record layout is addressed, i guess the next action item is to make sure the trivial constructor for the empty base is not evaluated conservatively (doh!). Because whatever bindings we make, for now they'd be blown away immediately during "construction".

This revision is now accepted and ready to land.Aug 18 2017, 1:25 AM
This revision was automatically updated to reflect the committed changes.