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

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/20150206/ce07d2ae/attachment.html>