[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 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.

2) does stackprotector_check need to be an intrinsic?  Can't it just  
be written in LLVM IR directly?

>> 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!



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.



/// 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.  I  
don't see "8 bytes" hard coded here, please update the comment.


             // 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.



   if (F->hasFnAttr(Attribute::StackProtectReq))
       return true;

funky indentation.


   if (F->hasFnAttr(Attribute::StackProtect)) {
...
   }

   return false;

Please use:

   if (!F->hasFnAttr(Attribute::StackProtect))
     return false;

to reduce nesting.



bool StackProtector::InsertStackProtectors() {
   // Loop through the basic blocks that have return instructions.  
Convert this:

This big comment is out of date, please update it.


   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.


       // 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.


Thanks again for working on this Bill!

-Chris