Page MenuHomePhabricator

bug33388 - Fix formatv_objet copy & move constructors

Authored by belleyb on Jun 21 2017, 10:02 AM.



formatv_object currently uses the implicitly defined move and copy constructors. It turns out these are buggy. In typical use-cases, the problem doesn't show-up because every single call to the move and copy constructors are elided. Thus, the buggy constructors are never invoked.

The issue especially shows-up when code is compiled using the -fno-elide-constructors compiler flag. For instance, this is useful when attempting to collect accurate code coverage statistics.

The exact issue is the following:

The Parameters data member is correctly moved or copied, thus making the parameters occupy new memory locations in the target object. Unfortunately, the default copying of the Adapters blindly copies the vector of pointers, leaving each of these pointers referencing the parameters in the original object instead of the copied one. These pointers quickly become dangling when the original object is deleted. This quickly leads to crashes.

The solution is to update the Adapters pointers when performing a move. The copy constructor isn't useful for format objects and can thus be deleted.


Diff Detail


Event Timeline

belleyb created this revision.Jun 21 2017, 10:02 AM
belleyb added inline comments.
555 ↗(On Diff #103416)

The following tests are failing without the fix and passing with it...

zturner edited edge metadata.Jun 22 2017, 10:14 AM

I'm wondering if we shouldn't just delete the copy constructor. Definitely having a move constructor seems reasonable, but I don't see any reason why we should ever be copying format objects. What do you think?

I wonder the exact same thing! We have no use for a copy constructor ourselves. Maybe, it's safer to delete it for now. We can always re-introduce it back if a clear need shows up.

Cool, lgtm after you delete the copy constructor.

belleyb updated this revision to Diff 103623.Jun 22 2017, 1:17 PM

Delete the formatv_object copy constructor as suggested @zturner.

belleyb added inline comments.Jun 22 2017, 1:18 PM
97 ↗(On Diff #103623)

Deleted copy constructor!

162 ↗(On Diff #103623)

Deleted copy constructor!

belleyb edited the summary of this revision. (Show Details)Jun 22 2017, 1:19 PM
zturner accepted this revision.Jun 22 2017, 1:19 PM
This revision is now accepted and ready to land.Jun 22 2017, 1:19 PM
belleyb marked 2 inline comments as done.Jun 22 2017, 1:19 PM

@zturner : Would you mind committing on my behalf ? (I don't have commit access...)

Sure, thanks for the fix.

belleyb edited the summary of this revision. (Show Details)Jun 26 2017, 5:06 AM
This revision was automatically updated to reflect the committed changes.