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

[Bill Wendling]

On Thu, Nov 6, 2008 at 10:35 AM, Chris Lattner <clattner at apple.com> wrote:
> 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.
>
I did a check and there didn't seem to be a difference between running
it and not running it (except for the addition of the stack protector
pass). That wasn't a massive function, though. I can check it out on a
more complex case.

>>> 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.
>
No prob. :-)

>>>> +  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!
>
NP :-)

>>>> +  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.
>
*nods*. I put code in the PEI module to do this, but I'll need to
check to make sure that there isn't additional places that can foul it
up.

>>>> +  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.
>
Hmm. Okay. I didn't know that the ilist stuff was a linked list. . .

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

-bw