[PATCH] 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)

[David Blaikie]

On Mon, Aug 18, 2014 at 6:24 PM,  <jingham at apple.com> wrote:
>
>
>> On Aug 18, 2014, at 5:09 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>
>> [+]Jim,
>>
>> I wonder what the LLDB perspective on this is:
>> For code like this example from http://reviews.llvm.org/D4956:
>>
>>> 300 if (foo f = foo())
>>> 301        body(); // this might throw
>>
>> where foo is a C++ class with a destructor, and body() may need a cleanup, which line table entry would the debugger prefer to see for the implicit call to ~foo()?
>>
>> // Pseudo-expanded code for the above example:
>> foo f = foo();       // 300
>> if (f) {             // 300
>>   body();           // 301
>>   _cleanups();      // 301
>> }
>> ~foo();              // ???
>>
>> According to David, GCC emits both the cleanup and the call to ~foo() on line 301, and GDB apparently has some heuristic that makes it skip 301 if the condition is false. (For reference, if I am on line 300 in LLDB and “next”, it will (unsurprisingly) unconditionally go to line 301.)
>
> It would be interesting to know how gdb figures this out.  If it seems to be doing the right thing with the gcc output, we can try to get lldb to use the same heuristics...
>
> However, there isn't much semantic information to help you out in the line tables.  The line table entries have is_stmt and basic_block flags, So, for instance, if the ~foo line range were marked line 301 && is_stmt == false, then the debugger could know that  when it got here from 300->301, it was not in the beginning part of 301, so it should probably just finish it up.  But I don't think gdb actually uses this, or at least it didn't last time I worked on this, and neither gcc nor clang take these flags very seriously.
>
> I bet it is just because gdb is more aggressive about never stopping in the middle of a line range than lldb.  So if gcc emits ONE line table entry that goes from the start of "body" to the end of "~foo", or maybe even from the cleanups to ~foo, in the false case you step from 300 to the middle of line 301, then you'd step through the ~foo() part of 301 because you started in the middle of it.

Good call!

  $ cat scope.cpp
  struct foo {
    operator bool() { return false; }
    ~foo() {}
  };
  void func() {}
  int main() {
    for (int i = 0; ++i != 10;)
      if (foo f = foo())
        func();
  }
  $ g++-4.8 scope.cpp -g && gdb ./a.out
  (gdb) start
  Temporary breakpoint 1, main () at scope.cpp:7
  7         for (int i = 0; ++i != 10;)
  (gdb) n
  8           if (foo f = foo())
  (gdb)
  7         for (int i = 0; ++i != 10;)
  (gdb)

Now, if you remove the func call and leave just a semicolon you get
this behavior:

  7         for (int i = 0; ++i != 10;)
  (gdb) n
  8           if (foo f = foo())
  (gdb)
  9             ;

So if there's no body in the if, the dtor call is the 'start' of the
address range for line 9 and GDB steps into it. But if the 'if' has a
body and thus the dtor call appears not at the start of the address
range of line 9, GDB skips it.

This pretty much works for most real-world code, I imagine, though
possibly by accident more than by design. (not sure - can't speak for
whomever implemented the GDB behavior here, nor whether any GCC
developer gave great thought to how the debug info would cause GDB to
behave)

> gdb may even consider itself to be in the "middle" of a line range if the range before it had the same line number, IIRC it coalesces the line tables like this as it ingests them.
>
> We could try making lldb more like gdb in this regard.
>
> If you are mostly concerned with stepping behavior, marking artificial lines as 0 means you don't have to play games like this.  And you can generally tell when you step out where you came from if that is an issue.

I haven't tried this - but I'll give it a go, just to see what the
user experience is like.

- David

>
> Jim
>
>
>>
>> -- adrian
>>
>> On Aug 18, 2014, at 3:24 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>> On Mon, Aug 18, 2014 at 2:23 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>>> I tested the patch with LLDB, and it takes the line table literally:
>>>
>>> What about in the non-exception test case?
>>>
>>> What's the nexting behavior in LLDB without the patch? In GDB I find I
>>> have to "next" twice on the 'if' line as it is today.
>>>
>>>>
>>>> ```
>>>> 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.
>>>
>>> That seems kind of awkward if you backtrace through a function with
>>> more than one 'foo' object - having the location information would
>>> help you know which 'foo' was being destroyed. (though arguably this
>>> applies to the cleanup/EH dtor call too - having it attributed to the
>>> end of the function is weird too - I'd sort of rather attribute it to
>>> the end of its scope but don't know what ramifications that has)
>>>
>>> Might need to talk to the GDB folks about what GDB does here and/or
>>> what the overall DWARF solution should be for both producers and
>>> consumers.
>>>
>>>>
>>>> http://reviews.llvm.org/D4956
>>>>
>>>>
>>
>