caio/clang-p2996
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[basicaa] Fix a latent bug in isGEPBaseAtNegativeOffset

Browse Source 
This was pointed out in review of D97520 by Nikita, but existed in the original code as well.

The basic issue is that a decomposed GEP expression describes (potentially) more than one getelementptr.  The "inbounds" derived UB which justifies this aliasing rule requires that the entire offset be composed of "inbounds" geps.  Otherwise, as can be seen in the recently added and changes in this patch test, we can end up with a large commulative offset with only a small sub-offset actually being "inbounds".  If that small sub-offset lies within the object, the result was unsound.

We could potentially be fancier here, but for the moment, simply be conservative when any of the GEPs parsed aren't inbounds.
This commit is contained in:
Philip Reames
2021-03-03 08:43:32 -08:00
parent dd9922c487
commit e6e5ef40cb
3 changed files with 14 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
llvm/include/llvm/Analysis/BasicAliasAnalysis.h
View File

@@ -143,6 +143,9 @@ private:
SmallVector<VariableGEPIndex, 4> VarIndices;
// Is GEP index scale compile-time constant.
bool HasCompileTimeConstantScale;
// Are all operations inbounds GEPs or non-indexing operations?
// (None iff expression doesn't involve any geps)
Optional<bool> InBounds;
void dump() const {
print(dbgs());

9
llvm/lib/Analysis/BasicAliasAnalysis.cpp
View File

@@ -477,6 +477,13 @@ BasicAAResult::DecomposeGEPExpression(const Value *V, const DataLayout &DL,
return Decomposed;
}
// Track whether we've seen at least one in bounds gep, and if so, whether
// all geps parsed were in bounds.
if (Decomposed.InBounds == None)
Decomposed.InBounds = GEPOp->isInBounds();
else if (!GEPOp->isInBounds())
Decomposed.InBounds = false;
// Don't attempt to analyze GEPs over unsized objects.
if (!GEPOp->getSourceElementType()->isSized()) {
Decomposed.Base = V;
@@ -1051,7 +1058,7 @@ bool BasicAAResult::isGEPBaseAtNegativeOffset(const GEPOperator *GEPOp,
const DecomposedGEP &DecompGEP, const DecomposedGEP &DecompObject,
LocationSize MaybeObjectAccessSize) {
// If the object access size is unknown, or the GEP isn't inbounds, bail.
if (!MaybeObjectAccessSize.hasValue() || !GEPOp->isInBounds())
if (!MaybeObjectAccessSize.hasValue() || !*DecompGEP.InBounds)
return false;
const uint64_t ObjectAccessSize = MaybeObjectAccessSize.getValue();

6
llvm/test/Analysis/BasicAA/negoffset.ll
View File

@@ -130,11 +130,11 @@ define void @one_size_unknown(i8* %p, i32 %size) {
; If part of the addressing is done with non-inbounds GEPs, we can't use
; properties implied by the last gep w/the whole offset. In this case,
; %random = %alloc - 4 bytes is well defined, and results in %p1 == %alloca.
; %random = %alloc - 4 bytes is well defined, and results in %step == %alloca,
; leaving %p as an entirely inbounds gep pointing inside %alloca
; CHECK-LABEL: Function: all_inbounds:
; CHECK: MayAlias: i32* %alloca, i8* %p0
; CHECK: NoAlias: i32* %alloca, i8* %p1
; FIXME: Result produced is currently wrong.
; CHECK: MayAlias: i32* %alloca, i8* %p1
define void @all_inbounds() {
%alloca = alloca i32, i32 4
%random = call i8* @random.i8(i32* %alloca)