Page MenuHomePhabricator

CFG: Delay creating Dtors for CompoundStmts which end in ReturnStmt
ClosedPublic

Authored by mgehre on Oct 21 2015, 11:04 PM.

Details

Summary

VisitReturnStmt would create a new block with including Dtors, so the Dtors created
in VisitCompoundStmts would be in an unreachable block.

Example:

struct S {

~S();

}

void f()
{

S s;
return;

}

void g()
{

S s;

}

Before this patch, f has one additional unreachable block containing just the
destructor of S. With this patch, both f and g have the same blocks.

Diff Detail

Repository
rL LLVM

Event Timeline

mgehre updated this revision to Diff 38089.Oct 21 2015, 11:04 PM
mgehre retitled this revision from to CFG: Delay creating Dtors for CompoundStmts which end in ReturnStmt.
mgehre updated this object.
mgehre added a reviewer: krememek.
mgehre added a subscriber: cfe-commits.
mgehre updated this revision to Diff 38090.Oct 21 2015, 11:12 PM

Fix formatting

klimek added a subscriber: klimek.Oct 22 2015, 2:55 AM

A test that fails before this patch and passes afterwards is needed :) If you need help writing that test, let me know.

mgehre updated this revision to Diff 38157.Oct 22 2015, 12:29 PM

Add test case

klimek added inline comments.Oct 23 2015, 2:31 AM
lib/Analysis/CFG.cpp
1949–1952 ↗(On Diff #38157)

If the body is non-empty, but the return is not the last statement, won't that still call addAutomaticObjDtors twice?

mgehre added inline comments.Oct 25 2015, 11:44 AM
lib/Analysis/CFG.cpp
1949–1952 ↗(On Diff #38157)

Yes, if there are statements after a "return" (i.e. dead code), it will still call addAutomaticObjDtors twice, just like before this patch.
I guess that this case is not to common, and would be flagged by other checks.

majnemer added inline comments.
lib/Analysis/CFG.cpp
1949 ↗(On Diff #38157)

Please don't use dyn_cast if you just need to perform a type test, use isa instead.
You need a space between if and (.

klimek added inline comments.Oct 25 2015, 1:30 PM
lib/Analysis/CFG.cpp
1949–1952 ↗(On Diff #38157)

Is there a reason to not fix that while we're here (seems like a related issue into which somebody might run down the road otherwise?)

mgehre updated this revision to Diff 38359.Oct 25 2015, 2:27 PM

Update for review comments

klimek added inline comments.Nov 1 2015, 10:21 AM
lib/Analysis/CFG.cpp
1949–1952 ↗(On Diff #38157)

I'd still be curious about an answer to my question :)

mgehre added inline comments.Nov 1 2015, 1:38 PM
lib/Analysis/CFG.cpp
1949–1952 ↗(On Diff #38359)

Yes, sorry, I was busy with something else. I was reluctant to fix the more general case, because that would introduce a second loop, and may (or may not) have performance implications. On the other hand, unreachable block don't seem to have any negative impact.

+jordan for an opinion on whether we want to fix the more general case.

My main problem is that while we're at it we need to fully understand the change anyway, and if somebody runs into this later, they'll need a long time debugging this surprising effect (like what happened to you here ;)

I see. Thinking about this, the same issue should also apply to a throw
inside the compound stmt. And it's not only about suppressing the dtors but
any CFG entries after return/throw. I'll prepare an updated patch.

Manuel Klimek <klimek@google.com> schrieb am So., 1. Nov. 2015 23:04:

klimek added a reviewer: jordan_rose.
klimek added a comment.

+jordan for an opinion on whether we want to fix the more general case.

My main problem is that while we're at it we need to fully understand the
change anyway, and if somebody runs into this later, they'll need a long
time debugging this surprising effect (like what happened to you here ;)

http://reviews.llvm.org/D13973

jordan_rose edited edge metadata.Nov 2 2015, 9:40 AM
jordan_rose added a subscriber: dcoughlin.

Hm. On the one hand, getting this right seems like a nice property, and even adding another loop wouldn't change the order of growth. On the other hand, not having the extra destructors seems like the same sort of property as not having unreachable statements, and if you do have unreachable statements then also having unreachable destructors seems reasonably consistent.

Devin, what would you think?

mgehre added a comment.Nov 2 2015, 1:32 PM

Basically, there are the following cases:
A)

void f() {
  S s;
}

Everything OK, only one (reachable) destructor is generated
B)

int f() {
  S s;
  return 1;
}

In current trunk, this first generates dtor in a block when visiting the CompoundStmt,
and then abandons that block when visiting the ReturnStmt. Thus we have one unreachable block just containing the dtor.
C)

int f() {
  S s;
  return 1;
  int i = 1;
}

Like B), but the unreachable block also contains "int i = 1".
D)

int f(bool b) {
  S s;
  if(b)
    return 1;
  else
    return 17;
}

Like B), but not fixed by this patch.
E)
Any combination of B) or D) with "throw" or a noreturn function instead of "return". Not fixed by the current patch.

I think that case B) is a common thing, and thus it's handled by this patch. Handling case B) with a 'throw' instead of 'return'
should be easy enough, too.
C) is a bit unusual due to the unreachable statement, thus I think that we should not optimize for that corner case.
D) more usual, but not easy to fix.

I currently see the following ways forward:

  1. Extend this patch to cover also "throw", fixing case B)
  2. Do 1) plus loop over the whole body to also fix case C)
  3. Drop this patch and instead remove unreachable blocks after the whole CFG has been generated (by mark-and-sweep), fixing B), C) and D)

The following comment at the beginning of VisitReturnStmt() indicates that this was planned

// NOTE: If a "return" appears in the middle of a block, this means that the
//       code afterwards is DEAD (unreachable).  We still keep a basic block
//       for that code; a simple "mark-and-sweep" from the entry block will be
//       able to report such dead blocks.

I'm not sure if that is actually done.

klimek added a comment.Nov 6 2015, 3:23 AM

I currently see the following ways forward:

  1. Extend this patch to cover also "throw", fixing case B)
  2. Do 1) plus loop over the whole body to also fix case C)
  3. Drop this patch and instead remove unreachable blocks after the whole CFG has been generated (by mark-and-sweep), fixing B), C) and D)

How would that work in a way that we don't lose the unreachable warnings that are actually correct?

I won't be able to do more about this issue than what the fix currently does (+ corrections/maybe a loop).
If this is not the preferred solution, and something more general should be done,
I will abandon this patch for now.

Currently, I cannot work on a more general solution. I don't have the experience with the
clang static analyzer that a more general solution
would require.

Jordan, your call :)

jordan_rose accepted this revision.Nov 12 2015, 9:36 AM
jordan_rose edited edge metadata.

Let's just go with your simple version that makes the common case better. There are a lot of problems with throw, so I wouldn't worry about that right now.

(By the way, we don't remove unreachable blocks because we want to be able to diagnose unreachable code.)

This revision is now accepted and ready to land.Nov 12 2015, 9:36 AM
This revision was automatically updated to reflect the committed changes.