[llvm-commits] [llvm] r58673 - in /llvm/trunk: include/llvm/CodeGen/Passes.h lib/CodeGen/LLVMTargetMachine.cpp lib/CodeGen/StackProtector.cpp
Chris Lattner
clattner at apple.com
Tue Nov 4 05:28:13 PST 2008
On Nov 3, 2008, at 6:10 PM, Bill Wendling wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=58673&view=rev
> Log:
> Initial checkin for stack protectors. Here's what it does:
Cool! Some questions/comments below:
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- llvm/trunk/lib/CodeGen/LLVMTargetMachine.cpp (original)
> +++ llvm/trunk/lib/CodeGen/LLVMTargetMachine.cpp Mon Nov 3 20:10:20
> 2008
> @@ -60,6 +60,11 @@
> EnableFastISelOption("fast-isel", cl::Hidden,
> cl::desc("Enable the experimental \"fast\" instruction selector"));
>
> +// Enable stack protectors.
> +static cl::opt<int>
> +EnableStackProtector("enable-stack-protector", cl::init(0),
> + cl::desc("Use ProPolice as a stack protection
> method."));
I don't know what "ProPolice" is, but it sounds like it may be a
trademark or something. Please just use "stack canary" or something
generic like that.
Also, you should use an enum for this, not magic integer numbers. I'd
like to see -enable-stack-protector=all | some | none.
> @@ -165,6 +170,8 @@
> if (!Fast)
> PM.add(createCodeGenPreparePass(getTargetLowering()));
>
> + PM.add(createStackProtectorPass(EnableStackProtector));
Why do you add the pass even when it is not enabled? Have you looked
at whether adding this pass ends up breaking analysis chains, and
requiring them to be run multiple times? For example, does dominators
end up being run more frequently with this? If so, the pass should
update dominators instead of invalidating it.
> @@ -0,0 +1,228 @@
> +//===-- StackProtector.cpp - Stack Protector Insertion
> --------------------===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open
> Source
> +// License. See LICENSE.TXT for details.
> +//
> +//
> =
> =
> =
> ----------------------------------------------------------------------=
> ==//
> +//
> +// This pass inserts stack protectors into functions which need
> them. The stack
> +// protectors this uses are the type that ProPolice used. A
> variable with a
Please remove references to ProPolice and make the documentation self
contained instead.
>
> +// random value in it is stored onto the stack before the local
> variables are
> +// allocated. Upon exitting the block, the stored value is checked.
> If it's
typo exiting.
> +// Enable stack protectors.
> +static cl::opt<unsigned>
> +SSPBufferSize("ssp-buffer-size", cl::init(8),
> + cl::desc("The lower bound for a buffer to be
> considered for "
> + "stack smashing protection."));
why the contraction ssp? Please use something like "stack-protector-
buffer-size" or something like that, for similarity with the enable-
stack-protector option.
>
> +
> +namespace {
> + class VISIBILITY_HIDDEN StackProtector : public FunctionPass {
> + // Level == 0 -- Stack protectors are off.
> + // Level == 1 -- Stack protectors are on only for some
> functions.
> + // Level == 2 -- Stack protectors are on for all functions.
> + int Level;
I think that this should be an enum in Passes.h, which can also be
used above for the command line option.
>
> ++ /// RequiresStackProtector - Check whether or not this
> function needs a
> + /// stack protector based upon the stack protector level.
> + bool RequiresStackProtector();
This should be const.
> +/// InsertStackProtectorPrologue - Insert code into the entry block
> that stores
> +/// the __stack_chk_guard variable onto the stack.
> +void StackProtector::InsertStackProtectorPrologue() {
> + BasicBlock &Entry = F->getEntryBlock();
> + Instruction &InsertPt = Entry.front();
> +
> + const char *StackGuardStr = "__stack_chk_guard";
> + StackGuardVar = M->getNamedGlobal(StackGuardStr)
If the module already has a '__stack_chk_guard' global with the wrong
type, I think this code will crash, see below:
>
> + if (!StackGuardVar)
> + StackGuardVar = new
> GlobalVariable(PointerType::getUnqual(Type::Int8Ty),
> + false,
> GlobalValue::ExternalLinkage,
> + 0, StackGuardStr, M);
We really want a Module::getOrInsertGlobal() method that works like
getOrInsertFunction. It would do the bitcast to solve the problem
above and factor away this code. Can you please add this?
> + StackProtFrameSlot = new
> AllocaInst(PointerType::getUnqual(Type::Int8Ty),
> + "StackProt_Frame", &InsertPt);
What ensures that this alloca ends up in the right place in the stack
frame at codegen time?
> + LoadInst *LI = new LoadInst(StackGuardVar, "StackGuard", true,
> &InsertPt);
> + new StoreInst(LI, StackProtFrameSlot, true, &InsertPt);
Please add a comment indicating why these are volatile. Why is the
load volatile?
> +/// InsertStackProtectorEpilogue - Insert code before the return
> instructions
> +/// checking the stack value that was stored in the prologue. If it
> isn't the
> +/// same as the original value, then call a "failure" function.
> +void StackProtector::InsertStackProtectorEpilogue() {
> + // Create the basic block to jump to when the guard check fails.
> + CreateFailBB();
> +
> + Function::iterator I = F->begin(), E = F->end();
> + std::vector<BasicBlock*> ReturnBBs;
> + ReturnBBs.reserve(F->size());
> +
> + for (; I != E; ++I)
> + if (isa<ReturnInst>((*I).getTerminator()))
> + ReturnBBs.push_back(I);
Two things: first, use I->getTerminator. Second, why do you make a
vector of returns and iterate over the vector? You should be able to
handle all of these with one pass over the function without the
intermediate vector. Also, F->size() is linear time, so that itself
does a pass over the function.
> + for (std::vector<BasicBlock*>::iterator
> + II = ReturnBBs.begin(), IE = ReturnBBs.end(); II != IE; +
> +II) {
> + BasicBlock *BB = *II;
> + ReturnInst *RI = cast<ReturnInst>(BB->getTerminator());
> + Function::iterator InsPt = BB; ++InsPt; // Insertion point for
> new BB.
> +
> + BasicBlock *NewBB = BasicBlock::Create("SPRet", F, InsPt);
> +
> + // Move the return instruction into the new basic block.
> + RI->removeFromParent();
> + NewBB->getInstList().insert(NewBB->begin(), RI);
Please use BasicBlock::splitBasicBlock to simplify this code.
> +/// CreateFailBB - Create a basic block to jump to when the stack
> protector
> +/// check fails.
> +void StackProtector::CreateFailBB() {
> + assert(!FailBB && "Failure basic block already created?!");
> + FailBB = BasicBlock::Create("CallStackCheckFailBlk", F);
> + std::vector<const Type*> Params;
> + Constant *StackChkFail =
> + M->getOrInsertFunction("__stack_chk_fail",
> + FunctionType::get(Type::VoidTy, Params,
> false));
You can eliminate the temporary vector by using the varargs form of
getOrInsertFunction:
Constant *StackChkFail =
M->getOrInsertFunction("__stack_chk_fail", Type::VoidTy, NULL);
> +/// RequiresStackProtector - Check whether or not this function
> needs a stack
> +/// protector based upon the stack protector level.
> +bool StackProtector::RequiresStackProtector() {
> + switch (Level) {
> + default: return false;
> + case 2: return true;
> + case 1: {
Please use enums instead of magic numbers :)
> + // If the size of the local variables allocated on the stack is
> greater than
> + // SSPBufferSize, then we require a stack protector.
> + uint64_t StackSize = 0;
> +
> + for (Function::iterator I = F->begin(), E = F->end(); I != E; +
> +I) {
> + BasicBlock *BB = I;
> +
> + for (BasicBlock::iterator
> + II = BB->begin(), IE = BB->end(); II != IE; ++II)
> + if (AllocaInst *AI = dyn_cast<AllocaInst>(II))
> + if (ConstantInt *CI = dyn_cast<ConstantInt>(AI-
> >getArraySize())) {
> + const APInt &Size = CI->getValue();
> + StackSize += Size.getZExtValue() * 8;
> + }
This isn't sufficient. If you have "alloca i32", the array size is
1. You need to use TargetData to multiply the array size by the size
of the type. Also, is the protection size in bits or bytes?
Does GCC ignore variable sized stack allocations?
> + if (SSPBufferSize <= StackSize)
> + return true;
This check should be moved inside the loop so that you stop scanning a
function as soon as you see at least enough to qualify for stack
protection.
>
> +
> +// [EOF] StackProtector.cpp
Urr?
-Chris
More information about the llvm-commits
mailing list