This is an archive of the discontinued LLVM Phabricator instance.

[RewriteStatepoints] Fix stale parse points
ClosedPublic

Authored by yrouban on Mar 1 2018, 1:17 AM.

Details

Summary

RewriteStatepointsForGC collects parse points for further processing. During the collection if a callsite is found in an unreachable block (DominatorTree::isReachableFromEntry()) then all unreachable blocks are removed by removeUnreachableBlocks(). Some of the removed blocks could have been reachable according to DominatorTree::isReachableFromEntry(). In this case the collected parse points became stale and resulted in a crash when accessed.

The fix does the following: if a parse point is detected in an unreachable block then the removeUnreachableBlocks() method is called, the DominatorTree is updated and all parse points are collected again.

The proposed test crashes with the old version and passes with the new.

Diff Detail

Event Timeline

yrouban created this revision.Mar 1 2018, 1:17 AM
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a subscriber: llvm-commits.
anna requested changes to this revision.Mar 1 2018, 7:31 AM

Good find Yevgeny!
So, removeUnreachableBlocks is much more aggressive than DT.isReachableFromEntry. Did you check if there's some function that just keeps exactly what isReachableFromEntry computes?

Other comments inline.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
2542

I'm not a fan of double collection of statepoints :) How about making this clearer:

  1. Unconditionally run canonicalizing step: removeUnreachableBlocks.
  2. Compute statepoints using line 2561 - 2565. At this point, just an assert that DT.isReachableFromEntry is enough.

Also, pls add an explicit comment stating that removeUnreachableBlocks is stronger than isReachableFromEntry.

test/Transforms/RewriteStatepointsForGC/unreachable-regression.ll
8

This was very confusing "why would removeUnreachableBlocks remove reachable blocks :)".

Once the unconditional canonicalization step is done, pls modify this comment and state that "Parse points are calculated only for reachable blocks".

This revision now requires changes to proceed.Mar 1 2018, 7:31 AM
yrouban added inline comments.Mar 1 2018, 6:12 PM
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
2542

Ok. Make sense.
I'm not sure if removeUnreachanbleBlocks() is easy. That is why I propose its lazy run. I will make it simple as you suggested.

yrouban updated this revision to Diff 136699.Mar 2 2018, 1:44 AM

made the removeUnreachableBlocks() call unconditinal

anna accepted this revision.Mar 3 2018, 7:50 AM

LGTM w/ comments.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
2535

I don't think this last line is needed here. removeUnreachableBlocks seems to be rather fancy compared to isReachableFromEntry ;)

2538

Nit: extra semicolon.

2542

Note that apart from canonicalizing the IR, we also got rid of the isReachableFromEntry check on every parse point.

This revision is now accepted and ready to land.Mar 3 2018, 7:50 AM
yrouban updated this revision to Diff 136948.Mar 4 2018, 7:57 PM

done the minor corrections as Anna suggested.
Could anyone with appropriate rights merge the patch?

inouehrs added inline comments.Mar 4 2018, 8:06 PM
test/Transforms/RewriteStatepointsForGC/unreachable-regression.ll
6

Minor typo: unreachanble -> unreachable

yrouban planned changes to this revision.Mar 4 2018, 9:36 PM
yrouban marked an inline comment as done.

hold on. I see unexpected test failure.

yrouban updated this revision to Diff 136951.Mar 4 2018, 11:13 PM
  • fixed the typos
  • fixed two tests to get rid of unreachability of tested statepoints
This revision is now accepted and ready to land.Mar 4 2018, 11:13 PM
anna added a comment.Mar 5 2018, 8:01 AM

done the minor corrections as Anna suggested.
Could anyone with appropriate rights merge the patch?

I'll land this today.

anna added a comment.Mar 5 2018, 12:06 PM

Yevgeny, Just for record - Dan will be trying to drop it. There's something wrong with my local repo and rebuilding it still doesn't allow me to git svn dcommit.

This is the patch details:

commit 854c9fce69c2593c1a74de4ff94914574f9e2356 (HEAD -> master)
Author: Anna Thomas <anna@azul.com>
Date:   Mon Mar 5 11:58:18 2018 -0500

    [RewriteStatepoints] Fix stale parse points
    
    RewriteStatepointsForGC collects parse points for further processing.
    During the collection if a callsite is found in an unreachable block
    (DominatorTree::isReachableFromEntry()) then all unreachable blocks are
    removed by removeUnreachableBlocks(). Some of the removed blocks could
    have been reachable according to DominatorTree::isReachableFromEntry().
    In this case the collected parse points became stale and resulted in a
    crash when accessed.
    
    The fix is to unconditionally canonicalize the IR to
    removeUnreachableBlocks and then collect the parse points.
    
    The added test crashes with the old version and passes with this patch.
    
    Patch by Yevgeny Rouban!
    
    Reviewed by: Anna
    
    Differential Revision: https://reviews.llvm.org/D43929
This revision was automatically updated to reflect the committed changes.