[llvm-commits] [llvm] r157261 - in /llvm/trunk: include/llvm/InitializePasses.h include/llvm/LinkAllPasses.h include/llvm/Transforms/Scalar.h include/llvm/Transforms/Utils/Local.h lib/Transforms/InstCombine/InstCombine.h lib/Transforms/InstCombine/InstCombineAddSub.cpp lib/Transforms/InstCombine/InstructionCombining.cpp lib/Transforms/Scalar/BoundsChecking.cpp lib/Transforms/Scalar/Scalar.cpp
John Criswell
criswell at illinois.edu
Tue May 22 11:32:36 PDT 2012
On 5/22/12 12:19 PM, Nuno Lopes wrote:
> Author: nlopes
> Date: Tue May 22 12:19:09 2012
> New Revision: 157261
>
> URL: http://llvm.org/viewvc/llvm-project?rev=157261&view=rev
> Log:
> add a new pass to instrument loads and stores for run-time bounds checking
> move EmitGEPOffset from InstCombine to Transforms/Utils/Local.h
>
> (a draft of this) patch reviewed by Andrew, thanks.
Have you measured the performance overhead of the code when this pass is
used?
Comments inline.
>
> Added:
> llvm/trunk/lib/Transforms/Scalar/BoundsChecking.cpp
> Modified:
> llvm/trunk/include/llvm/InitializePasses.h
> llvm/trunk/include/llvm/LinkAllPasses.h
> llvm/trunk/include/llvm/Transforms/Scalar.h
> llvm/trunk/include/llvm/Transforms/Utils/Local.h
> llvm/trunk/lib/Transforms/InstCombine/InstCombine.h
> llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
> llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
> llvm/trunk/lib/Transforms/Scalar/Scalar.cpp
>
> [snip]
>
> Modified: llvm/trunk/include/llvm/Transforms/Scalar.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Transforms/Scalar.h?rev=157261&r1=157260&r2=157261&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Transforms/Scalar.h (original)
> +++ llvm/trunk/include/llvm/Transforms/Scalar.h Tue May 22 12:19:09 2012
> @@ -327,6 +327,14 @@
>
> //===----------------------------------------------------------------------===//
> //
> +// BoundsChecking - This pass instruments the code to perform run-time bounds
> +// checking on loads, stores, and other memory intrinsics.
> +// Penalty is the maximum run-time that is acceptable for the user.
> +//
> +FunctionPass *createBoundsCheckingPass(unsigned Penalty = 5);
> +
> +//===----------------------------------------------------------------------===//
> +//
> // ObjCARCAPElim - ObjC ARC autorelease pool elimination.
> //
> Pass *createObjCARCAPElimPass();
>
> Modified: llvm/trunk/include/llvm/Transforms/Utils/Local.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Transforms/Utils/Local.h?rev=157261&r1=157260&r2=157261&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Transforms/Utils/Local.h (original)
> +++ llvm/trunk/include/llvm/Transforms/Utils/Local.h Tue May 22 12:19:09 2012
> @@ -15,6 +15,11 @@
> #ifndef LLVM_TRANSFORMS_UTILS_LOCAL_H
> #define LLVM_TRANSFORMS_UTILS_LOCAL_H
>
> +#include "llvm/Support/GetElementPtrTypeIterator.h"
> +#include "llvm/Support/IRBuilder.h"
> +#include "llvm/Target/TargetData.h"
> +#include "llvm/Operator.h"
> +
> namespace llvm {
>
> class User;
> @@ -160,6 +165,62 @@
> return getOrEnforceKnownAlignment(V, 0, TD);
> }
>
> +/// EmitGEPOffset - Given a getelementptr instruction/constantexpr, emit the
> +/// code necessary to compute the offset from the base pointer (without adding
> +/// in the base pointer). Return the result as a signed integer of intptr size.
> +template<typename IRBuilderTy>
> +Value *EmitGEPOffset(IRBuilderTy *Builder, const TargetData&TD, User *GEP) {
> + gep_type_iterator GTI = gep_type_begin(GEP);
> + Type *IntPtrTy = TD.getIntPtrType(GEP->getContext());
> + Value *Result = Constant::getNullValue(IntPtrTy);
> +
> + // If the GEP is inbounds, we know that none of the addressing operations will
> + // overflow in an unsigned sense.
> + bool isInBounds = cast<GEPOperator>(GEP)->isInBounds();
> +
> + // Build a mask for high order bits.
> + unsigned IntPtrWidth = TD.getPointerSizeInBits();
> + uint64_t PtrSizeMask = ~0ULL>> (64-IntPtrWidth);
> +
> + for (User::op_iterator i = GEP->op_begin() + 1, e = GEP->op_end(); i != e;
> + ++i, ++GTI) {
> + Value *Op = *i;
> + uint64_t Size = TD.getTypeAllocSize(GTI.getIndexedType())& PtrSizeMask;
> + if (ConstantInt *OpC = dyn_cast<ConstantInt>(Op)) {
> + if (OpC->isZero()) continue;
> +
> + // Handle a struct index, which adds its field offset to the pointer.
> + if (StructType *STy = dyn_cast<StructType>(*GTI)) {
> + Size = TD.getStructLayout(STy)->getElementOffset(OpC->getZExtValue());
> +
> + if (Size)
> + Result = Builder->CreateAdd(Result, ConstantInt::get(IntPtrTy, Size),
> + GEP->getName()+".offs");
> + continue;
> + }
> +
> + Constant *Scale = ConstantInt::get(IntPtrTy, Size);
> + Constant *OC = ConstantExpr::getIntegerCast(OpC, IntPtrTy, true /*SExt*/);
> + Scale = ConstantExpr::getMul(OC, Scale, isInBounds/*NUW*/);
> + // Emit an add instruction.
> + Result = Builder->CreateAdd(Result, Scale, GEP->getName()+".offs");
> + continue;
> + }
> + // Convert to correct type.
> + if (Op->getType() != IntPtrTy)
> + Op = Builder->CreateIntCast(Op, IntPtrTy, true, Op->getName()+".c");
> + if (Size != 1) {
> + // We'll let instcombine(mul) convert this to a shl if possible.
> + Op = Builder->CreateMul(Op, ConstantInt::get(IntPtrTy, Size),
> + GEP->getName()+".idx", isInBounds /*NUW*/);
> + }
> +
> + // Emit an add instruction.
> + Result = Builder->CreateAdd(Op, Result, GEP->getName()+".offs");
> + }
> + return Result;
> +}
> +
> ///===---------------------------------------------------------------------===//
> /// Dbg Intrinsic utilities
> ///
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombine.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombine.h?rev=157261&r1=157260&r2=157261&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombine.h (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombine.h Tue May 22 12:19:09 2012
> @@ -226,7 +226,7 @@
> bool DoXform = true);
> Instruction *transformSExtICmp(ICmpInst *ICI, Instruction&CI);
> bool WillNotOverflowSignedAdd(Value *LHS, Value *RHS);
> - Value *EmitGEPOffset(User *GEP, bool NoNUW = false);
> + Value *EmitGEPOffset(User *GEP);
>
> public:
> // InsertNewInstBefore - insert an instruction New before instruction Old
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp?rev=157261&r1=157260&r2=157261&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp Tue May 22 12:19:09 2012
> @@ -420,67 +420,6 @@
> }
> [snip]
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp?rev=157261&r1=157260&r2=157261&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp Tue May 22 12:19:09 2012
> @@ -87,6 +87,10 @@
> }
>
>
> +Value *InstCombiner::EmitGEPOffset(User *GEP) {
> + return llvm::EmitGEPOffset(Builder, *getTargetData(), GEP);
> +}
> +
> /// ShouldChangeType - Return true if it is desirable to convert a computation
> /// from 'From' to 'To'. We don't want to convert from a legal to an illegal
> /// type for example, or from a smaller to a larger illegal type.
>
> Added: llvm/trunk/lib/Transforms/Scalar/BoundsChecking.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/BoundsChecking.cpp?rev=157261&view=auto
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/BoundsChecking.cpp (added)
> +++ llvm/trunk/lib/Transforms/Scalar/BoundsChecking.cpp Tue May 22 12:19:09 2012
> @@ -0,0 +1,280 @@
> +//===- BoundsChecking.cpp - Instrumentation for run-time bounds checking --===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This file implements a pass that instruments the code to perform run-time
> +// bounds checking on loads, stores, and other memory intrinsics.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#define DEBUG_TYPE "bounds-checking"
> +#include "llvm/Transforms/Scalar.h"
> +#include "llvm/ADT/Statistic.h"
> +#include "llvm/Analysis/MemoryBuiltins.h"
> +#include "llvm/Support/Debug.h"
> +#include "llvm/Support/InstIterator.h"
> +#include "llvm/Support/IRBuilder.h"
> +#include "llvm/Support/TargetFolder.h"
> +#include "llvm/Target/TargetData.h"
> +#include "llvm/Transforms/Utils/Local.h"
> +#include "llvm/GlobalVariable.h"
> +#include "llvm/Instructions.h"
> +#include "llvm/Intrinsics.h"
> +#include "llvm/Operator.h"
> +#include "llvm/Pass.h"
> +using namespace llvm;
> +
> +STATISTIC(ChecksAdded, "Bounds checks added");
> +STATISTIC(ChecksSkipped, "Bounds checks skipped");
> +STATISTIC(ChecksUnable, "Bounds checks unable to add");
For evaluation purposes, it may be helpful to know how many of each kind
of instruction (loads, stores, atomics) were instrumented.
> +
> +typedef IRBuilder<true, TargetFolder> BuilderTy;
> +
> +namespace {
> + enum ConstTriState {
> + NotConst, Const, Dunno
> + };
> +
> + struct BoundsChecking : public FunctionPass {
> + const TargetData *TD;
> + BuilderTy *Builder;
> + Function *Fn;
> + BasicBlock *TrapBB;
> + unsigned Penalty;
> + static char ID;
I don't recall the rules for the default C++ visibility, but these
members should either be private or protected. They look like they're
public here.
> +
> + BoundsChecking(unsigned _Penalty = 5) : FunctionPass(ID), Penalty(_Penalty){
> + initializeBoundsCheckingPass(*PassRegistry::getPassRegistry());
> + }
You're passing a penalty parameter as a default constructor argument to
a pass. In my experience, that approach creates headaches. The problem
is that the default value is what opt and bugpoint will use, so if a bug
crops up when a non-default value is used, you need to go change the
default value to get bugpoint to reproduce the bug. It's easy to forget
to do that.
If you can find a better way of configuring that parameter, it would be
better.
> +
> + BasicBlock *getTrapBB();
> + ConstTriState computeAllocSize(Value *Alloc, uint64_t&Size, Value*&SizeValue);
> + bool instrument(Value *Ptr, Value *Val);
> +
> + virtual bool runOnFunction(Function&F);
> +
> + virtual void getAnalysisUsage(AnalysisUsage&AU) const {
> + AU.addRequired<TargetData>();
> + }
> + };
> +}
> +
> +char BoundsChecking::ID = 0;
> +INITIALIZE_PASS(BoundsChecking, "bounds-checking", "Run-time bounds checking",
> + false, false)
> +
> +
> +/// getTrapBB - create a basic block that traps. All overflowing conditions
> +/// branch to this block. There's only one trap block per function.
> +BasicBlock *BoundsChecking::getTrapBB() {
> + if (TrapBB)
> + return TrapBB;
> +
> + BasicBlock::iterator PrevInsertPoint = Builder->GetInsertPoint();
> + TrapBB = BasicBlock::Create(Fn->getContext(), "trap", Fn);
> + Builder->SetInsertPoint(TrapBB);
> +
> + llvm::Value *F = Intrinsic::getDeclaration(Fn->getParent(), Intrinsic::trap);
> + CallInst *TrapCall = Builder->CreateCall(F);
> + TrapCall->setDoesNotReturn();
> + TrapCall->setDoesNotThrow();
> + Builder->CreateUnreachable();
> +
> + Builder->SetInsertPoint(PrevInsertPoint);
> + return TrapBB;
> +}
> +
> +
> +/// computeAllocSize - compute the object size allocated by an allocation
> +/// site. Returns NotConst if the size is not constant (in SizeValue), Const if
> +/// the size is constant (in Size), and Dunno if the size could not be
> +/// determined within the given maximum Penalty that the computation would
> +/// incurr at run-time.
> +ConstTriState BoundsChecking::computeAllocSize(Value *Alloc, uint64_t&Size,
> + Value*&SizeValue) {
> + if (GlobalVariable *GV = dyn_cast<GlobalVariable>(Alloc)) {
> + if (GV->hasDefinitiveInitializer()) {
> + Constant *C = GV->getInitializer();
> + Size = TD->getTypeAllocSize(C->getType());
> + return Const;
> + }
> + return Dunno;
You can find the size of a global even if it doesn't have an
initializer; the trick is to make sure that the global is not declared
in another compilation unit.
> +
> + } else if (AllocaInst *AI = dyn_cast<AllocaInst>(Alloc)) {
> + if (!AI->getAllocatedType()->isSized())
> + return Dunno;
> +
> + Size = TD->getTypeAllocSize(AI->getAllocatedType());
> + if (!AI->isArrayAllocation())
> + return Const; // we are done
> +
> + Value *ArraySize = AI->getArraySize();
> + if (const ConstantInt *C = dyn_cast<ConstantInt>(ArraySize)) {
> + Size *= C->getZExtValue();
> + return Const;
> + }
> +
> + if (Penalty< 2)
> + return Dunno;
> +
> + SizeValue = ConstantInt::get(ArraySize->getType(), Size);
> + SizeValue = Builder->CreateMul(SizeValue, ArraySize);
> + return NotConst;
> +
> + } else if (CallInst *MI = extractMallocCall(Alloc)) {
> + SizeValue = MI->getArgOperand(0);
> + if (ConstantInt *CI = dyn_cast<ConstantInt>(SizeValue)) {
> + Size = CI->getZExtValue();
> + return Const;
> + }
> + return Penalty>= 2 ? NotConst : Dunno;
> +
> + } else if (CallInst *MI = extractCallocCall(Alloc)) {
> + Value *Arg1 = MI->getArgOperand(0);
> + Value *Arg2 = MI->getArgOperand(1);
> + if (ConstantInt *CI1 = dyn_cast<ConstantInt>(Arg1)) {
> + if (ConstantInt *CI2 = dyn_cast<ConstantInt>(Arg2)) {
> + Size = (CI1->getValue() * CI2->getValue()).getZExtValue();
> + return Const;
> + }
> + }
What about realloc() and strdup()?
Don't forget byval arguments.
> +
> + if (Penalty< 2)
> + return Dunno;
> +
> + SizeValue = Builder->CreateMul(Arg1, Arg2);
> + return NotConst;
> + }
> +
> + DEBUG(dbgs()<< "computeAllocSize failed:\n"<< *Alloc);
> + return Dunno;
> +}
> +
> +
> +bool BoundsChecking::instrument(Value *Ptr, Value *InstVal) {
There should be a comment indicating what the two parameters mean (as
well as the meaning of the return value).
> + uint64_t NeededSize = TD->getTypeStoreSize(InstVal->getType());
> + DEBUG(dbgs()<< "Instrument "<< *Ptr<< " for "<< Twine(NeededSize)
> +<< " bytes\n");
> +
> + Type *SizeTy = Type::getInt64Ty(Fn->getContext());
> +
> + // Get to the real allocated thing and offset as fast as possible.
> + Ptr = Ptr->stripPointerCasts();
> + GEPOperator *GEP;
> +
> + if ((GEP = dyn_cast<GEPOperator>(Ptr))) {
> + // check if we will be able to get the offset
> + if (!GEP->hasAllConstantIndices()&& Penalty< 2) {
> + ++ChecksUnable;
> + return false;
> + }
> + Ptr = GEP->getPointerOperand()->stripPointerCasts();
> + }
If I understand this code correctly, you're just checking to see if the
dereferenced pointer (Ptr) is a GEP and, if so, generating code to
calculate it's offset. What if Ptr is a PHI node derived from a GEP? I
imagine you'd see that in loops a lot. Does it handle non-pointer casts
(like inttoptr and ptrtoint, which I think some of the loop transforms
add when modifying GEPs)? Unless I'm not seeing something, your code
will miss a lot of trivial cases. Is this intentional or an oversight?
> +
> + uint64_t Size = 0;
> + Value *SizeValue = 0;
> + ConstTriState ConstAlloc = computeAllocSize(Ptr, Size, SizeValue);
> + if (ConstAlloc == Dunno) {
> + ++ChecksUnable;
> + return false;
> + }
> + assert(ConstAlloc == Const || SizeValue);
> +
> + uint64_t Offset = 0;
> + Value *OffsetValue = 0;
> +
> + if (GEP) {
> + if (GEP->hasAllConstantIndices()) {
> + SmallVector<Value*, 8> Ops(GEP->idx_begin(), GEP->idx_end());
> + assert(GEP->getPointerOperandType()->isPointerTy());
> + Offset = TD->getIndexedOffset(GEP->getPointerOperandType(), Ops);
> + } else {
> + OffsetValue = EmitGEPOffset(Builder, *TD, GEP);
> + }
> + }
> +
> + if (!OffsetValue&& ConstAlloc == Const) {
> + if (Size< Offset || (Size - Offset)< NeededSize) {
> + // Out of bounds
> + Builder->CreateBr(getTrapBB());
> + ++ChecksAdded;
> + return true;
> + }
> + // in bounds
> + ++ChecksSkipped;
> + return false;
> + }
> +
> + if (OffsetValue)
> + OffsetValue = Builder->CreateZExt(OffsetValue, SizeTy);
> + else
> + OffsetValue = ConstantInt::get(SizeTy, Offset);
> +
> + if (SizeValue)
> + SizeValue = Builder->CreateZExt(SizeValue, SizeTy);
> + else
> + SizeValue = ConstantInt::get(SizeTy, Size);
> +
> + Value *NeededSizeVal = ConstantInt::get(SizeTy, NeededSize);
> + Value *ObjSize = Builder->CreateSub(SizeValue, OffsetValue);
> + Value *Cmp1 = Builder->CreateICmpULT(SizeValue, OffsetValue);
> + Value *Cmp2 = Builder->CreateICmpULT(ObjSize, NeededSizeVal);
> + Value *Or = Builder->CreateOr(Cmp1, Cmp2);
> +
> + // FIXME: add unlikely branch taken metadata?
> + Instruction *Inst = Builder->GetInsertPoint();
> + BasicBlock *OldBB = Inst->getParent();
> + BasicBlock *Cont = OldBB->splitBasicBlock(Inst);
> + OldBB->getTerminator()->eraseFromParent();
> + BranchInst::Create(getTrapBB(), Cont, Or, OldBB);
> + ++ChecksAdded;
> + return true;
> +}
> +
> +bool BoundsChecking::runOnFunction(Function&F) {
> + TD =&getAnalysis<TargetData>();
> +
> + TrapBB = 0;
> + Fn =&F;
> + BuilderTy TheBuilder(F.getContext(), TargetFolder(TD));
> + Builder =&TheBuilder;
> +
> + // check HANDLE_MEMORY_INST in include/llvm/Instruction.def for memory
> + // touching instructions
I think you should add a TODO: or FIXME: here if this is a comment about
future work that you want to do.
> + std::vector<Instruction*> WorkList;
> + for (inst_iterator i = inst_begin(F), e = inst_end(F); i != e; ++i) {
> + Instruction *I =&*i;
> + if (isa<LoadInst>(I) || isa<StoreInst>(I) || isa<AtomicCmpXchgInst>(I) ||
> + isa<AtomicRMWInst>(I))
> + WorkList.push_back(I);
> + }
You may want to consider using an InstVisitor class instead of iterating
through all the instructions doing isa<> and dyn_cast<>. The
instruction visitor is supposedly faster as it acts like switch
statement. In some cases, it may also make the code easier to read.
Glad to see you thought of the atomic ops; those can be easy to overlook.
:)
> +
> + bool MadeChange = false;
> + while (!WorkList.empty()) {
> + Instruction *I = WorkList.back();
> + WorkList.pop_back();
> +
> + Builder->SetInsertPoint(I);
> + if (LoadInst *LI = dyn_cast<LoadInst>(I)) {
> + MadeChange |= instrument(LI->getPointerOperand(), LI);
> + } else if (StoreInst *SI = dyn_cast<StoreInst>(I)) {
> + MadeChange |= instrument(SI->getPointerOperand(), SI->getValueOperand());
> + } else if (AtomicCmpXchgInst *AI = dyn_cast<AtomicCmpXchgInst>(I)) {
> + MadeChange |= instrument(AI->getPointerOperand(),AI->getCompareOperand());
> + } else if (AtomicRMWInst *AI = dyn_cast<AtomicRMWInst>(I)) {
> + MadeChange |= instrument(AI->getPointerOperand(), AI->getValOperand());
> + } else {
> + llvm_unreachable("unknown Instruction type");
> + }
> + }
Something else you may consider doing is to have separate worklists for
each type of instruction instead of one worklist for all the
instructions. That way, you don't need to use dyn_cast.
Also, there's no need to remove items from the worklist with pop_back();
just iterate through the worklist with an iterator and process each item.
-- John T.
More information about the llvm-commits
mailing list