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

[Eric Christopher]

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.

On Mon Feb 09 2015 at 9:39:12 AM David Blaikie <dblaikie at gmail.com> wrote:

> +Eric - this was the thread about is_stmt, scope locations, etc, I was
> talking about last week.
>
> On Fri, Feb 6, 2015 at 2:52 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Tue, Aug 26, 2014 at 1:45 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Mon, Aug 18, 2014 at 2:33 PM, Adrian Prantl <aprantl at apple.com>
>>> wrote:
>>> > 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.
>>>
>>> OK. The "let's do better when there's braces" isn't something I'm
>>> worrying about for now - it's a possible incremental improvement after we
>>> figure out the "worst case" behavior.
>>>
>>> >
>>> > ```
>>> > if (c)
>>> >   foo(); // cleanups = line 0
>>> >
>>> > if (c) {
>>> > }         // cleanups
>>> >
>>> > if (c)
>>> >   foo();
>>> > else {
>>> >   bar();
>>> > }       // cleanups
>>>
>>> In this case, I'm not sure the close } to the else is any better than
>>> the last line of the else, it still feels like part of the else, not the
>>> end of the if scope.
>>>
>>> Anyway, using line zero (did this by hand and used no-integrated-as
>>> because Clang doesn't cope well with emitting line table entries for line
>>> 0) here's GDB's behavior in some examples:
>>>
>>> GCC is GCC 4.8
>>> ToT is Clang ToT
>>> End is this patch applied to Clang ToT to use the end of the if as the
>>> location
>>> Zer is the end patch applied, and then manually editing the assembly
>>> (and assembling with the system assembler, not the integrated assembler) to
>>> have the non-exceptional dtor call be on line 0.
>>>
>>> So we get the totally right behavior from GDB the same as we would by
>>> assigning it to the end. In the cases where we get different behavior, it's
>>> better - we don't step to lines that don't execute, but we still get a bad
>>> location walking up from the dtor call (end up wherever the preceeding code
>>> was - which means sometimes we end up on the last line of the function
>>> where the EH cleanup is ascribed to). Seems better, but not perfect. (but
>>> we have no way to get perfect)
>>>
>>> 1:   for
>>> 2:     if
>>> 3:       func();
>>> 4: }
>>>
>>> GCC: 1 2 3 1 2 1 2 3 1 4
>>> ToT: 1 2 3 2 1 2 2 1 2 3 2 1 4
>>> End: 1 2 3 3 1 2 3 1 2 3 3 1 4
>>> Zer: 1 2 3 1 2 1 2 3 1 4, dtor skipped and ascribed to 4
>>>
>>> 1:   for
>>> 2:     if
>>> 3:       func();
>>> 4:     else
>>> 5:       func();
>>> 6: }
>>>
>>> GCC: 1 2 3 1 2 5 1 2 3 1 6
>>> ToT: 1 2 3 2 1 2 5 2 2 3 2 1 6
>>> End: 1 2 3 1 2 5 1 2 3 1 6
>>> Zer: 1 2 3 1 2 5 1 2 3 1 6, dtor skipped and ascribed to 5
>>>
>>> 1:   for
>>> 2:     if
>>> 3:       ;
>>> 4:     else
>>> 5:       ;
>>> 6: }
>>>
>>> GCC: 1 2 5 1 2 5 1 2 5 1 6
>>> ToT: 1 2 2 1 2 2 1 2 2 1 6
>>> End: 1 2 5 1 2 5 1 2 5 1 6
>>> Zer: 1 2 1 2 1 2 1 6, dtor skipped and ascribed to 6
>>>
>>> 1:   for
>>> 2:     if
>>> 3:       ;
>>> 4: }
>>>
>>> GCC: 1 2 3 1 2 3 1 2 3 1 4
>>> ToT: 1 2 2 1 2 2 1 2 2 1 4
>>> End: 1 2 3 1 2 3 1 2 3 1 4
>>> Zer: 1 2 1 2 1 2 1 4, dtor skipped and ascribed to 4
>>>
>>
>> Updating these tables with some more ideas...
>>
>> TL;DR; We should put cleanup code on the right line (whatever that may
>> be, I think the end of the scope is best) with is_stmt 0. GDB essentially
>> ignores is_stmt 0 line table entries and attributes those lines to the
>> previous line entry which is wrong but fairly beningly so. (it'll just mean
>> the backtrace is at the last line we codegen'd which is /probably/ the if
>> block instead of the else block, if the else block is empty) ASan can do
>> the right thing and we can file a bug on GDB in case they care. But this
>> gets all the /stepping/ behavior as right as we can get it, really (empty
>> blocks like the first two cases will still step right past them and get a 1
>> 2 1 2 1 2 sort of behavior, but that seems OK)
>>
>> That said, implementing this means plumbing is_stmt through from the
>> frontend, and that seems like more work than I'm willing to put in right
>> now - but at least it gives us a plan for if/when this bug becomes a
>> priority for anyone to implement.
>>
>
> One other thing in favor of the is_stmt solution is that it's fairly
> robust - almost all the other solutions depend on block ordering for their
> GDB behavior (zero doesn't - but it seems to have some rather bad behavior,
> perhaps that's just GDB bugs, though). The GDB behavior of not breaking
> when stepping into the middle of a line doesn't seem entirely unreasonable,
> and that's all that's 'saving' any of the current solutions (including
> GCC's) and requires specific block ordering for it to do so. Using is_stmt
> 0 is block ordering independent with some minor GDB bugs (GDB just
> completely ignores the is_stmt 0 line entries, as though they weren't there
> at all - causing them to be wherever the previous line entry is, getting
> that GDB "don't break in the middle of a line" behavior).
>
>
>>
>> Sound plausible to everyone?
>>
>>
>> 1: for // loop twice
>> 2:   if // true on the first call, false on the second
>> 3:     CALL;
>> 4:   else
>> 5:     CALL;
>>
>> CALL=, exceptions
>>
>>     GCC: 1 2 5 1 2 5 1 (dtor: 5)
>>     ToT: 1 2 1 2 1 (dtor: 2)
>>     End: 1 2 1 2 1 (dtor: 5)
>>    Zero: 1 2 1 2 1 (dtor: 2)
>> no stmt: <same as ToT, already has is_stmt 0 there by coincidence>
>>
>> CALL=, no-exceptions
>>
>>     GCC: 1 2 5 1 2 5 1 (dtor: 5)
>>     ToT: 1 2 1 2 1 (dtor: 2)
>>     End: 1 2 5 1 2 5 1 (dtor: 5)
>>    Zero: 1 2 0x400603 (dtor: 0x400608) ??
>> no stmt: <same as ToT, already has is_stmt 0 there by coincidence>
>>
>> CALL=func(), exceptions
>>
>>     GCC: 1 2 3 1 2 5 1 (dtor: 5)
>>     ToT: 1 2 3 2 1 2 5 2 1 (dtor: 2)
>>     End: 1 2 3 1 2 1 (dtor: 5)*
>>    Zero: 1 2 3 0x4007ed (dtor: 0x4007f2) ??
>> no stmt: 1 2 3 1 2 5 1 (dtor: 5)
>>
>> CALL=func(), no-exceptions
>>
>>     GCC: 1 2 3 1 2 5 1 (dtor: 5)
>>     ToT: 1 2 3 2 1 2 5 2 1 (dtor: 2)
>>     End: 1 2 3 1 2 5 1 (dtor: 5)
>>    Zero: 1 2 3 0x40060b (dtor: 0x400610)
>> no stmt: 1 2 3 1 2 5 1 (dtor: 5)
>>
>> * because the EH cleanup code appears before the else block, the else
>> block is a line continuation and stepping to it skips over :/ is_stmt might
>> help? Let's see. Yep. Once I make the EH cleanup block is_stmt 0 and the
>> else block is_stmt 1 (on the same source line) I get the obvious 1 2 3 1 2
>> 5 1 (dtor: 5) behavior.
>>
>>
>>>
>>> > ```
>>> >
>>> > http://reviews.llvm.org/D4956
>>> >
>>> >
>>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150209/256838d5/attachment.html>