Page MenuHomePhabricator

[analyzer] When creating a temporary object copy, properly copy the value into it.

Authored by NoQ on Mar 2 2017, 6:25 AM.



The test case provided demonstrates a regression due to my earlier attempt to fix temporaries in D26839. Neither before nor after, we did not properly copy the symbolic rvalue of the object to the newly created memory region; in different ways though.

In the test case:

  1. there are two classes - Super (smaller) and Sub (bigger);
  2. a temporary object of Sub is created;
  3. a Sub-specific field not present in Super is initialized during construction;
  4. the temporary Sub-class object is casted to Super to call method m() that is declared in Super;
  5. a temporary region is retroactively created to represent this-region during method call;
  6. value of the Super-object is copied over the c++ base-object region;
  7. the rest of the temporary remains uninitialized;
  8. m() calls a virtual method mImpl() that resolves to the definition in Sub;
  9. mImpl() uses the Sub-specific field initialized on step 3 but not copied on step 6, as explained on step 7;
  10. a false warning regarding usage of uninitialized value is thrown.

The problem (apart from step 5 which is the reason for all the hassle, but needs AST fixes) is of course on step 6, where we should have just copied the whole Sub object. But - surprise! - the value of this object is no longer there in the Environment, because the cast on step 4 was already successfully modeled.

I'm uncertain of the proper solution. We could probably prevent objects from disappearing from the Environment upon unchecked-derived-to-base casts, but that'd require performance evaluations on C++-rich codebases.

For now, i propose to invalidate the Sub-object before binding the Super-object. This way at least we don't produce false accusations.

There's also one open question for the case when the value wasn't removed from the environment, but we still need to bind Super - i'm seeing failing tests when i disable binding Super, but i didn't investigate them yet.

Diff Detail


Event Timeline

NoQ created this revision.Mar 2 2017, 6:25 AM
a.sidorin edited edge metadata.Mar 3 2017, 12:14 AM

Hi Artem! Thank you for this patch. It looks very promising, but I have some questions and remarks.

187 ↗(On Diff #90326)

If we are touching names, should we rename Ex to InitWithAdjustments (or smth like this) and ExV correspondingly?

281 ↗(On Diff #90326)

Should we do all these operations with ExV/Reg if the InitV is known? There is a FIXME but I think it is related to all this code, not to the bindLoc only. And what happens if we remove this code?

285 ↗(On Diff #90326)

Seems like WipeV in comment should be InitV?

286 ↗(On Diff #90326)

If I understand correcly, if we call bindLoc(), we call checkRegionChanges() callbacks. And if we bindLoc() twice, we call them twice too. Is this what we want here?

NoQ planned changes to this revision.Mar 6 2017, 5:46 AM
NoQ added inline comments.
286 ↗(On Diff #90326)

This is actually an excellent question.

NoQ updated this revision to Diff 92618.Mar 22 2017, 4:53 AM
NoQ marked 4 inline comments as done.

Fix review comments!

Add a callback order test to ensure that checkRegionChanges is called as many (or rather as few) times as necessary.

281 ↗(On Diff #90326)

My bad: i overwrote SVal of Result above, and Result might have been equal to Init, which caused conflict between InitV and ExV.

This revision was automatically updated to reflect the committed changes.