This is an archive of the discontinued LLVM Phabricator instance.

Fix a use-after-RAUW bug in large GEP splitting
ClosedPublic

Authored by amharc on Sep 11 2018, 9:02 AM.

Details

Summary

Large GEP splitting, introduced in rL332015, uses a DenseMap<AssertingVH<Value>, ...>. This causes an assertion to fail (in debug builds) or undefined behaviour to occur (in release builds) when a value is RAUWed.

This manifested itself in the 7zip benchmark from the llvm test suite built on ARM with -fstrict-vtable-pointers enabled while RAUWing invariant group launders and splits in CodeGenPrepare.

This patch merges the large offsets of the argument and the result of an invariant.group strip/launder intrinsic before RAUWing.

Diff Detail

Repository
rL LLVM

Event Timeline

amharc created this revision.Sep 11 2018, 9:02 AM

The other AssertingVHs held by large GEP splitting code are either AssertingVH<GetElementPtrInst> or refer to possibly bitcasted GetElementPtrInsts. As far as I can see, there is no RAUWing of those being done in CGP at the moment.

This patch should probably include a custom ValueMapConfig which merges the vectors of GEPs while RAUWing.

Prazek accepted this revision.Sep 11 2018, 12:21 PM

LGTM, but please wait for Javed to review

This revision is now accepted and ready to land.Sep 11 2018, 12:21 PM
efriedma added inline comments.Sep 13 2018, 11:06 AM
llvm/lib/CodeGen/CodeGenPrepare.cpp
281 ↗(On Diff #164901)

Please add a comment explaining why the keys can be RAUW'ed, but the values can't.

Please add a comment explaining what happens if the result of the RAUW already exists in the map... or if that isn't possible, why it isn't possible.

What happened to this patch?

amharc updated this revision to Diff 170138.Oct 18 2018, 6:31 PM
amharc edited the summary of this revision. (Show Details)

Updated the patch to manually merge vectors of large GEP offsets when doing a RAUW of an invariant.group strip/launder intrinsic. This requires only handling keys and not values of the map, as values are always GetElementPtr instructions and not intrinsic calls.

I do not have the necessary level of understanding of CGP to determine if there are any other RAUWs there that need attention too, but so far I wasn't able to reproduce in cases that do not relate to invariant.group strips/launders.

This revision was automatically updated to reflect the committed changes.