PR19864: Revert r209764 in favor of the fix in r215766 (loop backedge jump attributed to confusing location when the last source line of a loop is inside a conditional)

[dblaikie]

Summary

The original fix for PR19864 (r209764) indirectly caused the loop
backedge of a (for \ if \ x) construct to be attributed to the 'if' line
(rather than the 'x' line where it was previously). It did this by
changing the location of the cleanup for the 'if'.

This was confusing all around - it's not the job of the 'if' to decide
where a loop backedge is located, source-wise, it was just happening by
accident because no other location was being set*. Instead, if we
specifically set the location before the backedge is emitted, its
location is now independent of that of the if's cleanup (this change was
made in r215766). So now you don't get strange nexting behavior in your
debugger where you could go "for -> if -> x -> /if/ -> for" as the
backedge was attributed to the 'if' line. Now the backedge is attributed
to the 'for' so it looks more natural (for -> if -> x -> for -> if ->
... ).

That is, unless the 'if' has cleanup (dtors) to run. In which case those
lines are attributed to the 'x' line and, in some circumstances, the
debugger may 'next' to them.

It looks as though GCC (though I don't know about LLDB) will avoid
nexting onto the dtor line somehow (I'm guessing some kind of
heuristic, but I haven't experimented too carefully to see just how/when
it works) so, even though the dtor is attributed to the 'x' line, and if
you break in the dtor and 'next' up you reach the 'x' line, if you next
from the 'if' and the condition is false, you'll end up at the 'for'
line and the dtor will have already been executed.

This breaks down when both the natural dtor /and/ exception handling
cleanup are required. GCC's output places the EH cleanup code after the
non-exceptional dtor invocation codepath. Clang places the EH cleanup
code before the non-exceptional dtor. This difference seems to cause GDB
to not skip the dtor line when 'next'ing with Clang's debug info, which
is unfortunate. I'm not sure if it's better fixed in GDB or Clang.

In brief:

Given "for / if / x"
Without any dtors in the 'if' scope, no behavior change.
With dtors:
Previous behavior (if true): for -> if -> x -> if -> for -> ...

(if false): for -> if -> if -> for -> if -> if

New behavior (under GDB):
Without EH cleanup (if true): for -> if -> x -> for -> ...

(if false): for -> if -> for -> ...

With EH cleanup (eg: if x can throw), even if false:

for -> if -> x -> for -> ...

[aprantl]

I tested the patch with LLDB, and it takes the line table literally:

void body() { printf("body\n"); }
foo::operator bool() throw() { return false; }
foo::~foo() { printf("~foo\n"); }

* thread #1: tid = 0x2a38e9, 0x0000000100000d8d debug-info-line-if`main + 221 at debug-info-line-if.cpp:51, queue = 'com.apple.main-thread', stop reason = step over
    frame #0: 0x0000000100000d8d debug-info-line-if`main + 221 at debug-info-line-if.cpp:51
   48  	  // CHECK: call void @_ZN3fooD1Ev{{.*}}, !dbg [[DBG3:!.*]]
   49  	
   50  	  //#line 400
-> 51  	  if (foo f = foo())
   52  	    body(); // this might throw
   53  	
   54  	  // GDB currently steps from the 'foo f' line to the 'body()' line, even if 'f'
(lldb) 
Process 10528 stopped
* thread #1: tid = 0x2a38e9, 0x0000000100000dc9 debug-info-line-if`main + 281 at debug-info-line-if.cpp:52, queue = 'com.apple.main-thread', stop reason = step over
    frame #0: 0x0000000100000dc9 debug-info-line-if`main + 281 at debug-info-line-if.cpp:52
   49  	
   50  	  //#line 400
   51  	  if (foo f = foo())
-> 52  	    body(); // this might throw
   53  	
   54  	  // GDB currently steps from the 'foo f' line to the 'body()' line, even if 'f'
   55  	  // is false, because the call to 'f's dtor is attribute to the 'body()' line.
(lldb) 
~foo
Process 10528 stopped
* thread #1: tid = 0x2a38e9, 0x0000000100000dce debug-info-line-if`main + 286 at debug-info-line-if.cpp:78, queue = 'com.apple.main-thread', stop reason = step over
    frame #0: 0x0000000100000dce debug-info-line-if`main + 286 at debug-info-line-if.cpp:78
   75  	  // CHECK: [[DBG5]] = metadata !{i32 401, i32 0, metadata !{{.*}}, null}
   76  	
   77  	  //#line 404
-> 78  	}
(lldb)

I know there is no satisfying line number for the call to ~foo, but putting it on a line that may not even get executed feels wrong to me. If necessary, I'd rather associate it with line 0, which is what the standard "no such location" location for the debugger to ignore.

[aprantl]

IMO, the best we can do is to use the end of the CompoundStmt as the cleanup location if there is one, and otherwise give up and use line 0.

if (c)
  foo(); // cleanups = line 0

if (c) {
}         // cleanups

if (c)
  foo();
else {
  bar();
}       // cleanups

[dblaikie]

+Eric - this was the thread about is_stmt, scope locations, etc, I was
talking about last week.

[echristo]

I'm generally in favor of plumbing is_stmt through the front end for
another reason as well: in general, looking at the front end we'll know
what's likely a "better" place to put a breakpoint. We can then use the
plumbing to make the experience good in general.

[chandlerc]

Dave, not sure there is anything else you need from me here, but if so, add me back and let me know. Until then, i'm moving this off my review queue.

lib/CodeGen/CGStmt.cpp

void CodeGenFunction::EmitIndirectGotoStmt(const IndirectGotoStmt &S) {
   cast<llvm::PHINode>(IndGotoBB->begin())->addIncoming(V, CurBB);
 
   EmitBranch(IndGotoBB);
 }
 
 void CodeGenFunction::EmitIfStmt(const IfStmt &S) {
   // C99 6.8.4.1: The first substatement is executed if the expression compares
   // unequal to 0.  The condition must be a scalar type.
-  LexicalScope ConditionScope(*this, S.getCond()->getSourceRange());
+  LexicalScope ConditionScope(*this, S.getSourceRange());
   RegionCounter Cnt = getPGORegionCounter(&S);
 
   if (S.getConditionVariable())
     EmitAutoVarDecl(*S.getConditionVariable());
 
   // If the condition constant folds and can be elided, try to avoid emitting
   // the condition and the dead arm of the if/else.
   bool CondConstant;

test/CodeGenCXX/debug-info-line-if.cpp

-// RUN: %clang_cc1 -g -std=c++11 -S -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -g -fexceptions -std=c++11 -S -emit-llvm %s -o - | FileCheck %s
 // PR19864
 extern int v[2];
 int a = 0, b = 0;
+struct foo {
+  ~foo();
+  operator bool() throw();
+};
+void body();
 int main() {
 #line 100
   for (int x : v)
     if (x)
       ++b; // CHECK: add nsw{{.*}}, 1
     else
       ++a; // CHECK: add nsw{{.*}}, 1
   // The continuation block if the if statement should not share the
   // location of the ++a statement. The branch back to the start of the loop
   // should be attributed to the loop header line.
 
   // CHECK: br label
   // CHECK: br label
   // CHECK: br label {{.*}}, !dbg [[DBG1:!.*]]
 
 #line 200
   while (a)
     if (b)
       ++b; // CHECK: add nsw{{.*}}, 1
     else
       ++a; // CHECK: add nsw{{.*}}, 1
 
   // CHECK: br label
   // CHECK: br label {{.*}}, !dbg [[DBG2:!.*]]
+
+#line 300
+  if (foo f = foo())
+    ++b;
+  else
+    ++a;
+
+  // The dtor of 'foo' should be attributed to the last line of the if/else
+  // scope, which is the "++a" line, as confusing as that is (especially if the
+  // ++a is never executed because 'f' evaluates to true). But GDB does the
+  // right thing and skips over this (so if you step from "++b" you step outside
+  // to the line outside the else) in this case, but it does get confused by
+  // LLVM's output when exceptions are involved. See the next test case.
+
+  // CHECK: call void @_ZN3fooD1Ev{{.*}}, !dbg [[DBG3:!.*]]
+
+#line 400
+  if (foo f = foo())
+    body(); // this might throw
+
+  // GDB currently steps from the 'foo f' line to the 'body()' line, even if 'f'
+  // is false, because the call to 'f's dtor is attribute to the 'body()' line.
+  // This is the right line for it (the end of the scope) but GDB fails to skip
+  // it when 'next'ing through the code - it seems that if we reorder the output
+  // so that the EH cleanup block (that calls the dtor, with a line table entry
+  // pointing to the end of the function) comes after the normal cleanup, GDB is
+  // fine.
+  // Not sure if this is better (or easier) fixed by changing Clang's output to
+  // match GDB, or fixing GDB's "skip dtors" heuristic (if that's what's going
+  // on on the GDB side here). If this is fixed in Clang, DBG4 and 5 line
+  // numbers should be be the inverse of what's tested below (first call should
+  // be the end-of-scope call, at line 401, second call should be the EH Block
+  // that's currently attributed to the function close '}', line 404)
+
+  // CHECK: call void @_ZN3fooD1Ev{{.*}}, !dbg [[DBG4:!.*]]
+  // CHECK: call void @_ZN3fooD1Ev{{.*}}, !dbg [[DBG5:!.*]]
+
   // CHECK: [[DBG1]] = metadata !{i32 100, i32 0, metadata !{{.*}}, null}
   // CHECK: [[DBG2]] = metadata !{i32 200, i32 0, metadata !{{.*}}, null}
+  // CHECK: [[DBG3]] = metadata !{i32 303, i32 0, metadata !{{.*}}, null}
+  // CHECK: [[DBG4]] = metadata !{i32 404, i32 0, metadata !{{.*}}, null}
+  // CHECK: [[DBG5]] = metadata !{i32 401, i32 0, metadata !{{.*}}, null}
+
+#line 404
 }