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

+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/386e1bac/attachment.html>