From f84407be32116bd50b9074b51ea3e37ec68515f8 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Sun, 19 Oct 2014 00:42:16 +0000 Subject: [PATCH] Switch how the datalayout availability test is handled in this code to make much more sense and in theory be more correct. If you trace the code alllll the way back to when it was first introduced, the comments make it slightly more clear what was going on here. At that time, the only way Base != V was if DL (then TD) was non-null. As a consequence, if DL *was* null, that meant we were loading directly from the alloca or global found above the test. After refactoring, this has become at least terribly subtle and potentially incorrect. There are many forms of pointer manipulation that can be traversed without DataLayout, and some of them would in fact change the size of object being loaded vs. allocated. Rather than this subtlety, I've hoisted the actual 'return true' bits into the code which actually found an alloca or global and based them on the loaded pointer being that alloca or global. This is both more clear and safer. I've also added comments about exactly why this set of predicates is used. I've also corrected a misleading comment about globals -- if overridden they may not just have a different size, they may be null and completely unsafe to load from! Hopefully this confuses the next reader a bit less. I don't have any test cases or anything, the patch is motivated strictly to improve the readability of the code. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@220156 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Analysis/Loads.cpp | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/lib/Analysis/Loads.cpp b/lib/Analysis/Loads.cpp index 5aa6068b56a..f5eb7055726 100644 --- a/lib/Analysis/Loads.cpp +++ b/lib/Analysis/Loads.cpp @@ -73,26 +73,39 @@ bool llvm::isSafeToLoadUnconditionally(Value *V, Instruction *ScanFrom, Type *BaseType = nullptr; unsigned BaseAlign = 0; if (const AllocaInst *AI = dyn_cast(Base)) { + // Loading directly from an alloca is trivially safe. We can't even look + // through pointer casts here though, as that might change the size loaded. + if (AI == V) + return true; + // 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(Base)) { - // Global variables are safe to load from but their size cannot be - // guaranteed if they are overridden. + // Global variables are not necessarily safe to load from if they are + // overridden. Their size may change or they may be weak and require a test + // to determine if they were in fact provided. if (!GV->mayBeOverridden()) { + // Loading directly from the non-overridden global is trivially safe. We + // can't even look through pointer casts here though, as that might change + // the size loaded. + if (GV == V) + return true; + BaseType = GV->getType()->getElementType(); BaseAlign = GV->getAlignment(); } } - if (BaseType && BaseType->isSized()) { - if (DL && BaseAlign == 0) + // 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, so only try this when we have the DataLayout available. + if (BaseType && BaseType->isSized() && DL) { + if (BaseAlign == 0) BaseAlign = DL->getPrefTypeAlignment(BaseType); if (Align <= BaseAlign) { - if (!DL) - return true; // Loading directly from an alloca or global is OK. - // Check if the load is within the bounds of the underlying object. PointerType *AddrTy = cast(V->getType()); uint64_t LoadSize = DL->getTypeStoreSize(AddrTy->getElementType()); -- 2.34.1