This is an archive of the discontinued LLVM Phabricator instance.

[PM] Introduce a PoisoningVH as a (more expensive) alternative to AssertingVH that delays any reported error until the handle is *used*.
ClosedPublic

Authored by chandlerc on Jan 23 2017, 5:01 PM.

Details

Summary

This allows data structures to contain handles which become dangling
provided the data structure is cleaned up afterward rather than used for
anything interesting.

The implementation is moderately horrible in part because it works to
leave AssertingVH in place, undisturbed. If at some point there is
consensus that this is simply how AssertingVH should be used, it can be
substantially simplified.

This remains a boring pointer in a non-asserts build as you would
expect. The only place we pay cost is in asserts builds.

I plan to use this as a basis for replacing the asserting VHs that
currently dangle in the new PM until invalidation occurs in both LVI and
SCEV.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.Jan 23 2017, 5:01 PM
chandlerc updated this revision to Diff 85501.Jan 23 2017, 5:32 PM

Fix a minor compile issue.

sanjoy requested changes to this revision.Jan 23 2017, 6:40 PM
sanjoy added inline comments.
include/llvm/IR/ValueHandle.h
410 ↗(On Diff #85501)

I know you'll hate me for this, but this needs to be conditional on LLVM_ENABLE_ABI_BREAKING_CHECKS also.

383 ↗(On Diff #85492)

Reflow comment here.

This revision now requires changes to proceed.Jan 23 2017, 6:40 PM
chandlerc marked an inline comment as done.Jan 23 2017, 7:12 PM
chandlerc added inline comments.
include/llvm/IR/ValueHandle.h
410 ↗(On Diff #85501)

No more or less than AssertingVH?

I assumed that the design idea of AssertingVH was that *users* must leverage LLVM_ENABLE_ABI_BREAKING_CHECKS if they expose the AssertingVH. Maybe that's wrong or maybe that's a bad model, but I'd rather the two handles follow the same model (and be fixed or changed at the same time).

Similarly, if today's users of AssertingVH are failing to use this macro, they were before my changes, and I'd rather not have to boil that ocean.

chandlerc updated this revision to Diff 85519.Jan 23 2017, 7:13 PM
chandlerc edited edge metadata.

Reflow comment Sanjoy spotted.

sanjoy accepted this revision.Jan 23 2017, 7:14 PM

lgtm

include/llvm/IR/ValueHandle.h
410 ↗(On Diff #85501)

SGTM!

As I said on IRC, I was under the impression that AssertingVH was using LLVM_ENABLE_ABI_BREAKING_CHECKS. Given that it does not, this looks fine.

This revision is now accepted and ready to land.Jan 23 2017, 7:14 PM
This revision was automatically updated to reflect the committed changes.