This is an archive of the discontinued LLVM Phabricator instance.

Make vector operation with variable index unsafe to speculate
AbandonedPublic

Authored by arsenm on Sep 13 2016, 8:18 PM.

Details

Reviewers
None
Summary

A branch condition could be guarding an out of bounds access of the vector.

Diff Detail

Event Timeline

arsenm updated this revision to Diff 71288.Sep 13 2016, 8:18 PM
arsenm retitled this revision from to Make vector operation with variable index unsafe to speculate.
arsenm updated this object.
arsenm added a subscriber: llvm-commits.
reames added a subscriber: reames.Sep 19 2016, 9:25 AM

It's not clear this is the right model for this. I agree we have to consider out of bounds indices (as specified by the LangRef), but perhaps we'd be better off treating the result as undef/poison as we do for other bits of undefined behaviour? I'm not really the right person to help debate this; you might want to get Sanjoy or David Majnemer involved. They tend to be really good with this family of issues.

LangRef says "If idx exceeds the length of val, the results are undefined." That implies that the operation itself isn't undefined behavior, and therefore it's safe to speculate.

That said, the current implementation of DAGTypeLegalizer::GetVectorElementPointer can lead to undefined behavior... so we need to either fix that to mask the index, or change LangRef. I would lean towards fixing GetVectorElementPointer.

lib/Analysis/ValueTracking.cpp
3239

isa<Constant> doesn't prove anything useful.

D26174 makes sure the index is in bounds when legalizing

arsenm abandoned this revision.Jan 11 2017, 9:51 AM

The legalization of variable indexes doesn't introduce undefined behavior now