[llvm-commits] [llvm] r58673 - in /llvm/trunk: include/llvm/CodeGen/Passes.h lib/CodeGen/LLVMTargetMachine.cpp lib/CodeGen/StackProtector.cpp

[Chris Lattner]

On Nov 4, 2008, at 11:40 AM, Bill Wendling wrote:
>>> @@ -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.
>>
> Okay. No. Don't know.

Can you check?  Just pass -debug-pass=Structure to llc with and  
without the pass.

>>> +// 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.
>>
> It's the same name that's used in GCC. I'm not tied to the name, and
> I'm not expecting this to be used much at all.

I understand, but gcc and llc don't agree on many options.  I'd prefer  
to keep the llc options regular where possible, even if gcc is strange.

>>> +/// 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:
>>
> Hrh?

Nevermind, getOrInsertGlobal fixes this.

>>> +  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?
>>
> Sure. That would be nice.

Awesome, thanks for doing 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?
>>
> We looked at the code in code gen -- the lowering of allocas. It's
> doing the "right thing" with regard to this. There might be some
> special magic that needs to be done, though.

I'd prefer to have something a little bit less magic than this.  Does  
the canary go before spill slots for callee save regs, could the frame  
packing pass move the field?  If we had a pass that sorted the fields  
based on size, it would move it.  Instead of ending up a normal frame  
index, we really want the canary to be a fixed frame index pinned to  
be right next to the retaddr.

>>> +  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.
>>
> I'm modifying the Function (inserting new blocks). Won't that
> invalidate the iterators?

No, linked list iterators don't get invalided like that.  The problem  
you'll have is that you'll rewrite the return block infinitely  
(because you split the block, and it might point to the block before  
the return, so you rediscover the return again), just increment the  
iterator once extra to solve that problem.

Thanks for working on this Bill, I really like this implementation  
approach!

-Chris