Page MenuHomePhabricator

[PartialInlining] Fix Crash from holding a reference to a destructed ORE
ClosedPublic

Authored by sfertile on Feb 12 2018, 7:41 PM.

Details

Summary

The Legacy Partial Inlining pass has an ownership issue related to the OptimizationRemarkEmitter objects it creates. The call back used to create an ORE caches the allocated object in a unique_ptr in the runOnModule function, and returns a reference to that object. In the case we find a multi-region outline candidate that fails to get inlined into any of its users we end up with a dangling reference. This occurs because we get a reference to an ORE in unswitchFunction which gets destructed when tryPartialInline calls the callback to create other ORE objects. After we fail to inline in the multi-region case, we continue on with the single-region case which tries to emit a remark with the now deleted emitter.

I've reverted back to creating the OREs directly rather they trying to get them as an analysis pass.

Diff Detail

Repository
rL LLVM

Event Timeline

sfertile created this revision.Feb 12 2018, 7:41 PM
sfertile added a subscriber: graham-yiu-huawei.

Are there any though on this? I would really appreciate feedback since I'm not very familiar with either the new-pass-manager or the OptimizationRemarkEmitter. One thing I'm unclear on is if there is some limitation forcing us to get a OptimizationRemarkEmitterAnalysis from the FunctionAnalysisManager. If that's not a hard limitation and we can simply create the OREs when needed then ends up pretty trivial to fix.

Adding Brian here since I know he's looked at optimization remarks lately and I haven't.

Thanks for tracking down this bug! You may wish to ask @anemet for a review, I think he would have a good idea of what a proper fix would look like. As-is this LGTM, although in my opinion the test case could benefit from being reduced further.

Thanks for tracking down this bug! You may wish to ask @anemet for a review, I think he would have a good idea of what a proper fix would look like. As-is this LGTM, although in my opinion the test case could benefit from being reduced further.

Thanks, asking @anemet is a good idea. I actually have a new patch where I roll back to just creating the OREs directly at the point of use rather then trying to get it 2 different ways from the legacy and new passes. This makes the code nice and straightforward, but I don't know if there are any drawbacks to doing it that way. I got sidetracked testing that though when using the new pass manager and --pgo in the test-suite caused a number of failures. (Ended up being a know issue https://bugs.llvm.org/show_bug.cgi?id=33773) .

The test I included here is so big because it had to fail being inlined to trigger the crash, I've realized now I can probably manually set the inline threshold to something lower and reduce it down to something reasonable.

sfertile updated this revision to Diff 139160.Mar 20 2018, 10:39 AM

Create the OREs at the point of use rather then trying to get them from the FunctionAnalysisManagerModuleProxy in the new-pass and allocating them in the legacy-pass.

Will try to cut down the test size next.

@sfertile Sorry for the late response. AFAIK there shouldn't be a downside to creating the ORE objects at the point of use, I only did it the original way to conform to the style I saw in other passes and to avoid passing the ORE object from function to function. However, my knowledge in the ORE area is limited so someone else may contradict what I just said. Good luck!

@sfertile Sorry for the late response. AFAIK there shouldn't be a downside to creating the ORE objects at the point of use, I only did it the original way to conform to the style I saw in other passes and to avoid passing the ORE object from function to function. However, my knowledge in the ORE area is limited so someone else may contradict what I just said. Good luck!

np Graham, I appreciate you taking the time to look at this again.

While it's preferred to use ORE as an analysis pass, sometimes that's hard (e.g because it's a function pass, or simply because it's hard to thread the ORE instance through the many layers). In these cases it's fine to construct one inline. When remarks are requested this will amount to repopulating BFI for the function as the ORE instance is created.

sfertile updated this revision to Diff 139551.Mar 22 2018, 8:08 PM
sfertile edited the summary of this revision. (Show Details)

Further reduced the test case.

I think this is fine. Go ahead.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 20 2018, 12:59 PM
This revision was automatically updated to reflect the committed changes.