This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Support returning objects by value.
ClosedPublic

Authored by NoQ on Feb 2 2018, 6:25 PM.

Details

Summary

Now that we've added the necessary information into the CFG in D42875, we can use it to support returning objects by value.

Unfortunately, we still do not support the constructions into temporaries which are later copied via copy-constructors, which is common in statements like return Class(). But we can still support objects constructed in a different manner, say return getObject() or return { 1, 2 }. Also, we've always supported return Obj; when Obj has a trivial copy constructor, but now we support it even when the constructor is non-trivial.

This patch adds a facility to prevent the new approach from being activated when temporary destructors are disabled, because we have a mechanism for suppressing hilarious false positives that wouldn't work in this case - see D42779. Namely, i'm adding a new EvalCallOption that will indicate that we're constructing a temporary object.

Diff Detail

Event Timeline

NoQ created this revision.Feb 2 2018, 6:25 PM
NoQ planned changes to this revision.Feb 5 2018, 5:50 PM

Destructor for the returned temporary is still evaluated conservatively:

class C {
public:
  int &x, &y;
  C(int &_x, int &_y) : x(_x), y(_y) { ++x; }
  C(const C &c) : x(c.x), y(c.y) { ++x; }
  ~C() { ++y; }
};

C make(int &x, int &y) {
  return { x, y };
}

void test() {
  int x = 0, y = 0;
  {
    C c = make(x, y);
  }
  clang_analyzer_dump(x); // 2 S32b
  clang_analyzer_dump(y); // 1 S32b
}

Also there's a weird rebound at the call site - from the callee-context-temporary to the caller-context-temporary - which is currently implemented as a trivial copy and not a full-featured constructor call. I guess there shouldn't be a copy here (because there is no constructor or assignment operator), so i suspect we should directly construct into the caller context's temporary for the call-expression.

These issues are known, i'm just pointing out that we'd need to tackle them to make at least this simple example work correctly.

NoQ updated this revision to Diff 133933.Feb 12 2018, 2:31 PM

Well, it still seems to be a reasonable improvement given how all temporary materialization works now, and it's under the flag (-analyzer-config cfg-temporary-dtors=true), so i guess i'll mark it as TODO and commit, and i'll keep an eye on that when looking at real-world code analysis results. Destructor fixes and tests are also coming in D43104.

NoQ accepted this revision.Feb 14 2018, 6:42 PM

Committed this but put a wrong phabricator link in the commit message, sorry >_<
rC325202

This revision is now accepted and ready to land.Feb 14 2018, 6:42 PM
NoQ closed this revision.Feb 14 2018, 6:42 PM