Page MenuHomePhabricator

[CFG] Fix crash in thread sanitizer.
ClosedPublic

Authored by nandor on Jul 15 2016, 11:56 AM.

Details

Summary

clang was crashing with -Wthread-safety due to the incorrect construction of the CFG when dealing with C++ lifetime-extending constructs.

struct Holder {
  virtual ~Holder() throw() {}
  int i = 0;
};

int main() {
  const auto &value = Holder().i;
}

This patch fixes the crash by correctly identifying the destructor.

Event Timeline

nandor retitled this revision from to [CFG] Fix crash in thread sanitizer..
nandor updated this object.
nandor added a reviewer: krememek.
nandor updated this revision to Diff 64368.Jul 18 2016, 1:09 PM

Fixed typo

zaks.anna edited reviewers, added: NoQ, dexonsmith; removed: doug.gregor, krememek.Jul 25 2016, 10:12 AM
NoQ edited edge metadata.Jul 26 2016, 2:17 AM

Thanks for working on this!

While i probably won't be of much help (no expert in temporaries yet), i notice that this code piece (with getReferenceInitTemporaryType()) seems to be duplicated at least three times in this file, with slight variations and FIXMEs. So i guess this needs some care, not necessarily in this patch.

lib/Analysis/CFG.cpp
3910 ↗(On Diff #65545)

Simple ty->isReferenceType() should work as well (the overloaded operator ->() in QualType).

3912–3914 ↗(On Diff #65545)

Is this sequence of checks certainly needed? All tests seem to pass without it. Or maybe it's also needed in addAutomaticObjectDtors()? Perhaps just nest these checks inside getReferenceInitTemporaryType()?

nandor updated this revision to Diff 65545.Jul 26 2016, 9:43 AM
nandor edited edge metadata.
nandor marked an inline comment as done.

removed redundant checks

I'm not an expert in temporaries either, I think for a proper fix a lot more work is required.

It seems that the problem is with locating destructors in destructor calls. I get the impression that currently a destructor node pointing to the declaration is added and the type that is actually destroyed is inferred from the expression every time it is needed. I think destructors should be able to point to specific temporary subexpressions that they destroy in order to deal with multiple temporaries from the same expression.

This fix only ensures that all scenarios which lead to a placement of such a destructor node are handled so no SIGSEV occurs due to a destructor being added, but not handled at all later on.

lib/Analysis/CFG.cpp
3912–3914 ↗(On Diff #65545)

It seems like getReferenceInitTemporaryType includes these two statements, we can remove them from here.

dcoughlin accepted this revision.Aug 2 2016, 1:58 PM
dcoughlin edited edge metadata.

LGTM. I will commit. Thanks Nandor!

This revision is now accepted and ready to land.Aug 2 2016, 1:58 PM
This revision was automatically updated to reflect the committed changes.