On Apr 3, 2019, Richard Biener <rguent...@suse.de> wrote: > My solution is to instead of dropping the debug binds still > move them to the destination but then reset them. This solves > the wrong-debug issue.
*nod*, yeah, that's probably the best we can do without major surgery. > But it of course possibly degrades debug information for the > other paths into the destination block. If we could find a condition that took control flow through the forwarding path vs other paths, we could try to preserve them with a ternary expression that uses an unbound debug temp as the reset value, the closest to a conditional debug bind we could have right now. But I guess it would make no difference once we attempted to resolve the expression and found a subexpression to be reset. > I'm less sure what would be the correct action for DEBUG_BEGIN > stmts (the patch contines to drop them on the floor, leaving > reset debug-binds possibly surrounded by wrong stmt context?). Dropping them is the best we can do now. > Not sure what else debug stmt kinds we have and what the proper > action for those would be. Inline entry point markers, also to be dropped for now. > Somewhere I've written that debug-stmts living on edges would > also solve the issue on the GIMPLE (and RTL) side, not sure > if we can make sense of those in the end though. Once you go down that path, I guess you'll soon find a need for control flow graphs within edges, at which point it gets really ugly. I'm slightly more hopeful about identifying guarding conditions to introduce conditional debug stmts. > Having stmts permanently on edges is also sth new on GIMPLE so I'm > staying away from that at this moment... *firm nod* :-) > I've noted that for the specific case where there is > (before the next DEBUG_BEGIN_STMT? before the next real stmt?) > (debug-) definitions of the debug vars we reset in the destination > block then dropping the debug stmt might be better and avoids > the debug-info quality degadation. If there's any instruction or view that would be reached by the earlier bind (the one that precedes the one we'd drop or reset), it would get wrong debug information if we were to drop the bind rather than reset it. If there isn't, whatever happens to the bind will have no effect. This suggests to me that resetting it is the right thing to do. Now, if we were to try to duplicate the logic that decides whether the bind might possibly be observable, in order to drop it on the floor instead of resetting it, we should look for another bind of the same variable before any real stmt or debug marker. If we find one, it would be ok to drop the bind instead of resetting it. I suppose we might even make this part of the debug bind resetting logic, or introduce separate but reusable functionality for this sort of cleanup. But didn't we have something to that effect already? I vaguely recall Jakub implemented something that would drop binds that could not be observed, and that it became a lot less effective after adjusting it to deal with view markers. > I guess that at least looking at all PHI nodes of the destination > might be a good idea because those defs happen before the "new" debug > reset. /me mumbles something about PHI debug binds ;-) > 2019-04-03 Richard Biener <rguent...@suse.de> > PR debug/89892 > PR debug/89905 > * tree-cfgcleanup.c (remove_forwarder_block): Always move > debug bind stmts but reset them if they are not valid at the > destination. > * gcc.dg/guality/pr89892.c: New testcase. > * gcc.dg/guality/pr89905.c: Likewise. LGTM, thanks. Any reason to mention PR 88882 in the Subject: but not in the ChangeLog? -- Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo Be the change, be Free! FSF Latin America board member GNU Toolchain Engineer Free Software Evangelist Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
