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

Tue Feb 19 06:01:35 PST 2019

uabelho added a comment.

In D57149#1401627 <https://reviews.llvm.org/D57149#1401627>, @mattd wrote:

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

To me there are two different issues.

1. Fixing the immediate problem (in StackProtector)
2. Doing optimizations, e.g. make CGP better in cleaning up the code.

My focus with this patch is to address 1, and if someone wants to do 2 and optimize CGP, then please go ahead with that too.

I didn't think fixing 1 would be controversial, apart from that possibly someone would have an opinion about the DT requirement.

In D57149#1369777 <https://reviews.llvm.org/D57149#1369777>, @rnk wrote:

> An alternative way to handle this would be to remove unreachable code. I'm surprised unreachable code reaches this pass in a normal pipeline. I thought CGP removed it in normal operation, even at O0. However, removing unreachable code might invalidate the domtree (unless our utility for that updates it?).

I don't know CGP at all, but just from a quick look it seems like it doesn't require DT and it doesn't update it so I've no idea how messy it would be to go down that route.

There is some code in CodeGenPrepare::runOnFunction that tries to remove dead basic blocks, but those blocks are identified by having zero predecessors, so more complex situations with e.g. a loop that is unreachable from entry doesn't seem to be handled.

In this exact case the dead loop consists of several blocks from the beginning and then it's reduced down to

  for.body2:                                        ; preds = %for.body2
    %spec.select = select i1 undef, i16* %d, i16* %spec.select
    br label %for.body2

by CodeGenPrepare::eliminateFallThrough but there doesn't seem to be anything after that tries to remove things.

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

https://reviews.llvm.org/D57149