CFG: Delay creating Dtors for CompoundStmts which end in ReturnStmt

[mgehre]

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.

[mgehre]

Fix formatting

[klimek]

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

[mgehre]

Add test case

[klimek]

lib/Analysis/CFG.cpp

1949–1952

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

[mgehre]

lib/Analysis/CFG.cpp

1949–1952

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]

lib/Analysis/CFG.cpp

1949

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]

lib/Analysis/CFG.cpp

1949–1952

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]

Update for review comments

[klimek]

lib/Analysis/CFG.cpp

1949–1952

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

[mgehre]

lib/Analysis/CFG.cpp

1949–1952

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.

[klimek]

+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 ;)

[mgehre]

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 <[e-mail]> 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]

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]

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]

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?

[mgehre]

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.

[klimek]

Jordan, your call :)

[jordan_rose]

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.)

lib/Analysis/CFG.cpp

CFGBlock *CFGBuilder::VisitChooseExpr(ChooseExpr *C,
   addSuccessor(Block, KnownVal.isFalse() ? nullptr : LHSBlock);
   addSuccessor(Block, KnownVal.isTrue() ? nullptr : RHSBlock);
   Block->setTerminator(C);
   return addStmt(C->getCond());
 }
 
 
 CFGBlock *CFGBuilder::VisitCompoundStmt(CompoundStmt *C) {
-  addLocalScopeAndDtors(C);
+  LocalScope::const_iterator scopeBeginPos = ScopePos;
+  if (BuildOpts.AddImplicitDtors) {
+    addLocalScopeForStmt(C);
+  }
+  if(!C->body_empty() && !dyn_cast<ReturnStmt>(*C->body_rbegin())) {
+    // If the body ends with a ReturnStmt, the dtors will be added in VisitReturnStmt
+    addAutomaticObjDtors(ScopePos, scopeBeginPos, C);
+  }
+
   CFGBlock *LastBlock = Block;
 
   for (CompoundStmt::reverse_body_iterator I=C->body_rbegin(), E=C->body_rend();
        I != E; ++I ) {
     // If we hit a segment of code just containing ';' (NullStmts), we can
     // get a null block back.  In such cases, just use the LastBlock
     if (CFGBlock *newBlock = addStmt(*I))
       LastBlock = newBlock;