[Loads/SROA] Remove blatantly incorrect code and fix a bug revealed in the process

[reames]

Summary

The code we had isSafeToLoadUnconditionally was blatantly wrong. This function takes a "Size" argument which is supposed to describe the span loaded from. Instead, the code use the size of the pointer passed (which may be unrelated!) and only checks that span. For any Size > LoadSize, this can and does lead to miscompiles.

Worse, the generic code just a few lines above correctly handles the cases which *are* valid. So, let's delete said code.

(Note: There's another occurrence of the same bug in the same function in the scan code. I'll get that next.)

Removing this exposes a bits vs bytes confusion in SROA + one other test change where the semantics of the test aren't clear to me. I propose to simply change the test output (as in the diff) and move on.

[jdoerfert]

I think ByteOffset is used *but* the logic you remove is just a copy of what isDereferenceableAndAlignedPointer is doing already only worse and also wrong (see my comment below).

LGTM.

test/Transforms/SROA/addrspacecast.ll

289

Maybe add something like ; Note that the external linkage of @gv doesn't guarantee dereferenceable memory to make sure people will not fall into the same trap again and understand easily what is happening.

[reames]

Updating patch, and requesting re-review.

I screwed up in posting the original patch. I managed to leave out part of the diff, and my patch description was unclear.

[reames]

Fix the other occurrence of the Size vs LoadSize bug.

[reames]

Improve test coverage to make bugs being fixed clear.

[jdoerfert]

Removing the duplicated (wrong) logic is still fine. Using Size is also the right thing. The bits/bytes size mixup seems unrelated but given that bytes is the right thing I'm OK with it.

The one thing I would like to add is a test for the Size vs PtrSize issue, see my comment below. Otherwise, LGTM.

lib/Analysis/Loads.cpp

268

Can we have a test for this? I would hope something where we want to pre-loads a 64 bit pointer but only have a 32 bit access?

[reames]

lib/Analysis/Loads.cpp

268

I've been sitting here trying to write that test for most of the morning. :)

Most existing callers pass a size which is the pointer size. The sole exception is the phi-speculation code in SROA, and I haven't figured out how to trigger it there just yet. My previously added select tests demonstrate the *spirit* of the problem, but don't actually expose the current bug.

lib/Analysis/Loads.cpp

if (Align == 0)
     Align = DL.getABITypeAlignment(V->getType()->getPointerElementType());
   assert(isPowerOf2_32(Align));
 
   // If DT is not specified we can't make context-sensitive query
   const Instruction* CtxI = DT ? ScanFrom : nullptr;
   if (isDereferenceableAndAlignedPointer(V, Align, Size, DL, CtxI, DT))
     return true;
 
-  int64_t ByteOffset = 0;
-  Value *Base = V;
-  Base = GetPointerBaseWithConstantOffset(V, ByteOffset, DL);
-
-  if (ByteOffset < 0) // out of bounds
+  if (!ScanFrom)
     return false;
 
-  Type *BaseType = nullptr;
-  unsigned BaseAlign = 0;
-  if (const AllocaInst *AI = dyn_cast<AllocaInst>(Base)) {
-    // An alloca is safe to load from as load as it is suitably aligned.
-    BaseType = AI->getAllocatedType();
-    BaseAlign = AI->getAlignment();
-  } else if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(Base)) {
-    // Global variables are not necessarily safe to load from if they are
-    // interposed arbitrarily. Their size may change or they may be weak and
-    // require a test to determine if they were in fact provided.
-    if (!GV->isInterposable()) {
-      BaseType = GV->getType()->getElementType();
-      BaseAlign = GV->getAlignment();
-    }
-  }
-
-  PointerType *AddrTy = cast<PointerType>(V->getType());
-  uint64_t LoadSize = DL.getTypeStoreSize(AddrTy->getElementType());
-
-  // If we found a base allocated type from either an alloca or global variable,
-  // try to see if we are definitively within the allocated region. We need to
-  // know the size of the base type and the loaded type to do anything in this
-  // case.
-  if (BaseType && BaseType->isSized()) {
-    if (BaseAlign == 0)
-      BaseAlign = DL.getPrefTypeAlignment(BaseType);
-
-    if (Align <= BaseAlign) {
-      // Check if the load is within the bounds of the underlying object.
-      if (ByteOffset + LoadSize <= DL.getTypeAllocSize(BaseType) &&
-          ((ByteOffset % Align) == 0))
-        return true;
-    }
-  }
-
-  if (!ScanFrom)
+  if (Size.getBitWidth() > 64)
     return false;
+  const uint64_t LoadSize = Size.getZExtValue();
 
   // Otherwise, be a little bit aggressive by scanning the local block where we
   // want to check to see if the pointer is already being loaded or stored
   // from/to.  If so, the previous load or store would have already trapped,
   // so there is no harm doing an extra load (also, CSE will later eliminate
   // the load entirely).
   BasicBlock::iterator BBI = ScanFrom->getIterator(),
                        E = ScanFrom->getParent()->begin();
while (BBI != E) {
 
     Type *AccessedTy = AccessedPtr->getType()->getPointerElementType();
     if (AccessedAlign == 0)
       AccessedAlign = DL.getABITypeAlignment(AccessedTy);
     if (AccessedAlign < Align)
       continue;
 
     // Handle trivial cases.
-    if (AccessedPtr == V)
+    if (AccessedPtr == V &&
+        LoadSize <= DL.getTypeStoreSize(AccessedTy))
       return true;
 
     if (AreEquivalentAddressValues(AccessedPtr->stripPointerCasts(), V) &&
         LoadSize <= DL.getTypeStoreSize(AccessedTy))
       return true;
   }
   return false;
 }

lib/Transforms/Scalar/SROA.cpp

if (LI->getParent() != BB)
       return false;
 
     // Ensure that there are no instructions between the PHI and the load that
     // could store.
     for (BasicBlock::iterator BBI(PN); &*BBI != LI; ++BBI)
       if (BBI->mayWriteToMemory())
         return false;
 
-    uint64_t Size = DL.getTypeStoreSizeInBits(LI->getType());
+    uint64_t Size = DL.getTypeStoreSize(LI->getType());
     MaxAlign = std::max(MaxAlign, LI->getAlignment());
     MaxSize = MaxSize.ult(Size) ? APInt(APWidth, Size) : MaxSize;
     HaveLoad = true;
   }
 
   if (!HaveLoad)
     return false;
 

test/Transforms/SROA/addrspacecast.ll

;
   %p.0.c = select i1 undef, i64* %c, i64* %c
   %asc = addrspacecast i64* %p.0.c to i64 addrspace(1)*
 
   %cond.in = select i1 undef, i64 addrspace(1)* %asc, i64 addrspace(1)* null
   %cond = load i64, i64 addrspace(1)* %cond.in, align 8
   ret void
 }
 
-@gv = external addrspace(1) global i64
+;; If this was external, we wouldn't be able to prove dereferenceability
+;; of the location.
+@gv = addrspace(1) global i64 zeroinitializer
 
 define void @select_addrspacecast_gv(i1 %a, i1 %b) {
 ; CHECK-LABEL: @select_addrspacecast_gv(
 ; CHECK-NEXT:    [[COND_SROA_SPECULATE_LOAD_FALSE:%.*]] = load i64, i64 addrspace(1)* @gv, align 8
 ; CHECK-NEXT:    [[COND_SROA_SPECULATED:%.*]] = select i1 undef, i64 undef, i64 [[COND_SROA_SPECULATE_LOAD_FALSE]]
 ; CHECK-NEXT:    ret void
 ;
   %c = alloca i64, align 8
   %p.0.c = select i1 undef, i64* %c, i64* %c
   %asc = addrspacecast i64* %p.0.c to i64 addrspace(1)*
 
   %cond.in = select i1 undef, i64 addrspace(1)* %asc, i64 addrspace(1)* @gv
   %cond = load i64, i64 addrspace(1)* %cond.in, align 8
   ret void
 }
 
-; CHECK-LABEL: @select_addrspacecast_i8(
-; CHECK: [[SEL:%.*]] = select i1 undef, i8 undef, i8 undef
-; CHECK-NEXT: ret i8 [[SEL]]
 define i8 @select_addrspacecast_i8() {
+; CHECK-LABEL: @select_addrspacecast_i8(
+; CHECK-NEXT:    [[RET_SROA_SPECULATED:%.*]] = select i1 undef, i8 undef, i8 undef
+; CHECK-NEXT:    ret i8 [[RET_SROA_SPECULATED]]
+;
   %a = alloca i8
   %b = alloca i8
 
   %a.ptr = addrspacecast i8* %a to i8 addrspace(1)*
   %b.ptr = addrspacecast i8* %b to i8 addrspace(1)*
 
   %ptr = select i1 undef, i8 addrspace(1)* %a.ptr, i8 addrspace(1)* %b.ptr
   %ret = load i8, i8 addrspace(1)* %ptr
   ret i8 %ret
 }
 
 !0 = !{!1, !1, i64 0, i64 1}
 !1 = !{!2, i64 1, !"type_0"}
 !2 = !{!"root"}
 !3 = !{!4, !4, i64 0, i64 1}
 !4 = !{!2, i64 1, !"type_3"}