[llvm] e6e5ef4 - [basicaa] Fix a latent bug in isGEPBaseAtNegativeOffset

[Philip Reames via llvm-commits]

Author: Philip Reames
Date: 2021-03-03T08:43:32-08:00
New Revision: e6e5ef40cbc239754003c93c46df644dad8a8272

URL: https://github.com/llvm/llvm-project/commit/e6e5ef40cbc239754003c93c46df644dad8a8272
DIFF: https://github.com/llvm/llvm-project/commit/e6e5ef40cbc239754003c93c46df644dad8a8272.diff

LOG: [basicaa] Fix a latent bug in isGEPBaseAtNegativeOffset

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.

Added: 
    

Modified: 
    llvm/include/llvm/Analysis/BasicAliasAnalysis.h
    llvm/lib/Analysis/BasicAliasAnalysis.cpp
    llvm/test/Analysis/BasicAA/negoffset.ll

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/Analysis/BasicAliasAnalysis.h b/llvm/include/llvm/Analysis/BasicAliasAnalysis.h
index e785a3ba438c..6b5e9d5b71ba 100644
--- a/llvm/include/llvm/Analysis/BasicAliasAnalysis.h
+++ b/llvm/include/llvm/Analysis/BasicAliasAnalysis.h
@@ -143,6 +143,9 @@ class BasicAAResult : public AAResultBase<BasicAAResult> {
     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());

diff  --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index d954810e54f6..f20baad23a82 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -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();

diff  --git a/llvm/test/Analysis/BasicAA/negoffset.ll b/llvm/test/Analysis/BasicAA/negoffset.ll
index 1fe69a1f0e85..2963bcddf14f 100644
--- a/llvm/test/Analysis/BasicAA/negoffset.ll
+++ b/llvm/test/Analysis/BasicAA/negoffset.ll
@@ -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)