Page MenuHomePhabricator

[ValueTracking] avoid crashing from bad assumptions (PR31809)
ClosedPublic

Authored by spatel on Feb 1 2017, 7:33 AM.

Details

Summary

A program may contain llvm.assume info that disagrees with other analysis. This may be caused by UB in the program, so we must not crash because of that.

As noted in the code comments and PR31809:
https://llvm.org/bugs/show_bug.cgi?id=31809
...we can do better, but this at least avoids the assert/crash in the bug report.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Feb 1 2017, 7:33 AM
majnemer accepted this revision.Feb 1 2017, 7:38 AM

LGTM

This revision is now accepted and ready to land.Feb 1 2017, 7:38 AM
This revision was automatically updated to reflect the committed changes.
davide edited edge metadata.Feb 1 2017, 8:32 AM

I have a question about the improvement you proposed (inline).

llvm/trunk/lib/Analysis/ValueTracking.cpp
800–803

Have you looked into how hard it would be to implement this invalidation? Also, why is that a stronger version? (i.e. there are cases where this actually matters or you're just speculating?)
Also, clearing the whole assumption cache is really what we want? I mean, what if there are other informations that are not inconsistent and we want to use them anyway?

davide added inline comments.Feb 1 2017, 8:36 AM
llvm/trunk/lib/Analysis/ValueTracking.cpp
800–803

In other words, I'm not entirely sure we should add this FIXME.

hfinkel added inline comments.Feb 1 2017, 8:42 AM
llvm/trunk/lib/Analysis/ValueTracking.cpp
800–803

I agree. This is a rare case and I don't think we need to make the infrastructure more complicated than necessary to handle it. Plus, it is a useful invariant that all of the assumptions are in the cache.

spatel added inline comments.Feb 1 2017, 8:51 AM
llvm/trunk/lib/Analysis/ValueTracking.cpp
800–803

Yes, I looked at putting Q.AC->clear() here. Let me attach a draft of that patch (or should I open a new review?).
I think that would be stronger in the sense that we'd be less likely to "leak" a transform based on a bad assumption.

I have no evidence that this matters or works the way I'm imagining (other than it affects the test cases included in this patch). That's why I didn't include it here. I was (possibly incorrectly) assuming that once we hit a bad assumption, we go into "all bets are off" mode, so we might as well nuke the cache. Let's remove the FIXME if that's wrong.

Personally, I think emitting the warning is the more important thing to do because we're now silently continuing compilation when we know something bad has happened.

Attaching a draft of the assumption cache invalidation patch that I was working on. This is just for reference. I don't think we want to pursue it because it's more trouble than it's worth even if it is "stronger".

Attaching a draft of the assumption cache invalidation patch that I was working on. This is just for reference. I don't think we want to pursue it because it's more trouble than it's worth even if it is "stronger".

I don't think it's stronger, but it's definitely more trouble. Remove the FIXME from the original patch and LGTM.