This is an archive of the discontinued LLVM Phabricator instance.

Add verifier pass for finding GC relocation bugs
ClosedPublic

Authored by anna on Jan 6 2016, 4:29 PM.

Details

Summary

Original Patch and summary by Philip Reames.

RewriteStatepointsForGC tries to rewrite a function in a manner where the optimizer can't end up using a pointer value after it might have been relocated by a safepoint. This pass checks the invariant that RSForGC is supposed to establish and that (if we constructed semantics correctly) later passes must preserve.

This has been a really useful diagnostic tool when initially developing the rewriting scheme and has found numerous bugs. Manuel recently pointed out that this had never gotten upstreamed. Given both Manuel and the Microsoft team are in the process of trying to ramp up both the amount of IR being fed through and the optimization passes run, it seems useful to have this available for them as well. Manuel in particular has already found and fixed a couple of relocation bugs the hard way.

In terms of placement and structure, I left this in the IR directory which is where we'd happened to drop it in our tree. I could see an argument that this should be in either Analysis or possibly Transform. I'm happy to move it if anyone has a preference.

Diff Detail

Repository
rL LLVM

Event Timeline

reames updated this revision to Diff 44168.Jan 6 2016, 4:29 PM
reames retitled this revision from to Add verifier pass for finding GC relocation bugs.
reames updated this object.
reames added a subscriber: llvm-commits.
swaroop.sridhar accepted this revision.Jan 6 2016, 6:24 PM
swaroop.sridhar edited edge metadata.

Thanks for adding the IR checker.
My preference is for keeping it within the Analysis (rather than IR) tree.

This revision is now accepted and ready to land.Jan 6 2016, 6:24 PM
mjacob added a subscriber: mjacob.Jan 6 2016, 7:56 PM

I'm fine if this lives in the IR directory, next to Verifier.

Please change the function names to start with a lowercase letter.

mjacob added a comment.Jan 7 2016, 3:22 PM

Have you run clang-format on the source code? I saw some consecutive empty lines which should be removed by clang-format?

lib/IR/SafepointIRVerifier.cpp
77 ↗(On Diff #44168)

This is already implied by AU.setPreservesAll();

190 ↗(On Diff #44168)

Can you restate this comment, so it tells me what it is for instead of which funtion it calls?

223–224 ↗(On Diff #44168)

I think this should be in a DEBUG(...) and not guarded by PrintOnly.

ping

I have have patches which make the verifier accept the "non-constant GC pointer with constant base" case. Should I submit them for review after this patch here is committed?

anna commandeered this revision.Jun 29 2017, 10:44 AM
anna added a reviewer: reames.
anna added a subscriber: anna.

Taking this over from Philip. Will address the review comments.
Looks like there's conflicting opinion about where to place this - in the IR directory next to verifier, or in the analysis directory.
I prefer keeping it in the IR directory itself if no one has objections.

anna edited the summary of this revision. (Show Details)Jun 29 2017, 10:46 AM
This revision was automatically updated to reflect the committed changes.
anna marked 3 inline comments as done.Jul 4 2017, 6:23 PM
anna added inline comments.
lib/IR/SafepointIRVerifier.cpp
223–224 ↗(On Diff #44168)

I've added a DEBUG as well as a separate statement. We need PrintOnly guard as part of testing.