[Loads/SROA] Remove blatantly incorrect code and fix a bug revealed in the process
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.
Removing this code revealed two issues:
1) As noted by jdoerfert the removed code incorrectly handled external globals. The test update in SROA is to stop testing incorrect behavior.
2) SROA was confusing bytes and bits, but this wasn't obvious as the Size parameter was being essentially ignored anyway. Fixed.
Differential Revision: https://reviews.llvm.org/D66778
llvm-svn: 370102
diff --git a/llvm/lib/Analysis/Loads.cpp b/llvm/lib/Analysis/Loads.cpp
index 43cfe3d..a40f8dd 100644
--- a/llvm/lib/Analysis/Loads.cpp
+++ b/llvm/lib/Analysis/Loads.cpp
@@ -210,51 +210,13 @@
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
- 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)
return false;
+ 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,
@@ -302,7 +264,8 @@
continue;
// Handle trivial cases.
- if (AccessedPtr == V)
+ if (AccessedPtr == V &&
+ LoadSize <= DL.getTypeStoreSize(AccessedTy))
return true;
if (AreEquivalentAddressValues(AccessedPtr->stripPointerCasts(), V) &&