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

Bill Wendling wendling at apple.com
Mon Nov 17 21:33:03 PST 2008


On Nov 15, 2008, at 10:13 PM, Chris Lattner wrote:

> On Nov 4, 2008, at 3:59 PM, Bill Wendling wrote:
>
>> On Tue, Nov 4, 2008 at 5:28 AM, Chris Lattner <clattner at apple.com>  
>> wrote:
>>>> +  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.
>>>
>> I looked at --debug-pass=Details with stack protectors and without.
>> There doesn't seem to be a difference in the two analyses. I'll test
>> more on other programs to make sure. I'm trying to do this late in  
>> the
>> game -- right before DAG conversion -- so that LLVM IR passes won't  
>> be
>> affected too much.
>
> Ok, thanks.
>
> Some other comments:
>
> 1) are there any .ll testcases for the new intrinsics?  It seems  
> that there should be something in test/CodeGen/X86.  Is there any  
> target-specific code for the lowering?  If not, putting a test in  
> test/CodeGen/generic would be even better.
>
There's no target-specific code for lowering. (YAY!) As far as  
testcases, the only ones we have are in the llvm-gcc testsuite. But I  
can craft together some kind of .ll files for our local tests.

> 2) does stackprotector_check need to be an intrinsic?  Can't it just  
> be written in LLVM IR directly?
>
Hmm, perhaps. If the load is marked "volatile", will the optimizer  
passes refrain from moving it around? That's the only question. Right  
now the intrinsic is marked as "IntrReadMem", so I'm hoping that  
passes will leave it alone.

>>> 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 simplified the code. But it still looks like modifying the function
>> while iterating over it is doing badness (it went into an infinite
>> loop on me when I tried it out). What did you have in mind?
>
> Since you've made many changes to the pass, I'll just review the  
> whole thing again.  I really like the approach and where it  
> converged to, thanks for working on this!
>
Sure! :-)

> static cl::opt<unsigned>
> SSPBufferSize("stack-protector-buffer-size", cl::init(8),
>              cl::desc("The lower bound for a buffer to be considered  
> for "
>                       "stack smashing protection."));
>
> This is the longest option description in llc --help, please shorten  
> it a bit.
>
Okay.

> /// RequiresStackProtector - Check whether or not this function  
> needs a stack
> /// protector based upon the stack protector level. The heuristic we  
> use is to
> /// add a guard variable to functions that call alloca, and  
> functions with
> /// buffers larger than 8 bytes.
> bool StackProtector::RequiresStackProtector() const {
>
> It's a minor thing, but I'd prefer this method to be lexically right  
> after runOnFunction to aid in reading the file from top to bottom.

Heh. I prefer it the opposite way (auxiliary methods are placed out of  
the way), but no big deal. I'll move it.

>  I don't see "8 bytes" hard coded here, please update the comment.

Done.

>
>            // If an array has more than 8 bytes of allocated space,  
> then we
>            // emit stack protectors.
>            if (SSPBufferSize <= TD->getABITypeSize(AT))
>              return true;
>
> Same out of date comment.
>
Okay.

>  if (F->hasFnAttr(Attribute::StackProtectReq))
>      return true;
>
> funky indentation.
>
Okay.

>  if (F->hasFnAttr(Attribute::StackProtect)) {
> ...
>  }
>
>  return false;
>
> Please use:
>
>  if (!F->hasFnAttr(Attribute::StackProtect))
>    return false;
>
> to reduce nesting.
>
Okay.

> bool StackProtector::InsertStackProtectors() {
>  // Loop through the basic blocks that have return instructions.  
> Convert this:
>
> This big comment is out of date, please update it.
>
Alright.

>  for (Function::iterator I = F->begin(), E = F->end(); I != E; ) {
>    BasicBlock *BB = I;
>
>    if (ReturnInst *RI = dyn_cast<ReturnInst>(BB->getTerminator())) {
> ...
>      ++I; // Skip to the next block so that we don't resplit the  
> return block.
> ...
>    } else {
>      ++I;
>    }
>  }
>
> I think this loop would be simpler if you wrote it as:
>
>  for (Function::iterator I = F->begin(), E = F->end(); I != E; ) {
>    BasicBlock *BB = I++;
>
>   ReturnInst *RI = dyn_cast<ReturnInst>(BB->getTerminator());
>   if (!RI) continue;
>   ...
> }
>
> It reduces nesting and simplifies manipulation of I.
>
Yeah. Good call. :-)

>
>      // Move the newly created basic block to the point right after  
> the old
>      // basic block so that it's in the "fall through" position.
>      NewBB->removeFromParent();
>      F->getBasicBlockList().insert(I, NewBB);
>
> This is correct, but less efficient than using BB->moveBefore or BB- 
> >moveAfter.  Unlinking and relinking a block has to remove all the  
> instructions from the function symbol table then reinsert them.
>
Oh yeah. That's fairly gross. Done!
>
> Thanks again for working on this Bill!
>
No probs. :-)

-bw




More information about the llvm-commits mailing list