[PATCH] D57149: [StackProtector] Skip analysing dead users in HasAddressTaken, PR40436

[Matt Davis via Phabricator via llvm-commits]

mattd added a comment.

In D57149#1388361 <https://reviews.llvm.org/D57149#1388361>, @uabelho wrote:

> In D57149#1387325 <https://reviews.llvm.org/D57149#1387325>, @mattd wrote:
>
> > Hi @uabelho
> >
> > The test case in your patch definitely causes a problem, but that's because the -start-before/-stop-after is used.  If I run that same test through llc, the UnreachableBlockElim pass kicks-in and drops the dead code.
> >  I took a look at the code in your Janurary 24th comment.  I was unable to get the same dead block to surface, but most likely I had the wrong flag set.  How did you build the sample in that comment?
>
>
> It can be reproduced with the full llc -O1 pipe on current trunk using:
>
>   llc -O1 -o - pr40436.ll
>
> F8236901: pr40436.ll <https://reviews.llvm.org/F8236901>


I was able to repro your issue.  As pointed out earlier, CGP does not cover this particular case.  
It might be useful to eliminate the dead code there, in CGP, if possible.  At least we wouldn't be passing the unused statements around to other passes. 
In any case  I'd suggest tossing an assert in the StackProtector ensuring that such a the pathologic recursion does not occur.

I do see the point of ensuring the sanity of StackProtector, and I'm not opposed to patching that up (as you have done here in this patch); however, I do see benefit in preventing the dead code from reaching the StackProtector in the first place.   I'm not sure how others feel.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57149/new/

https://reviews.llvm.org/D57149