[llvm-commits] [llvm] r81179 - /llvm/trunk/lib/VMCore/Verifier.cpp

[Nick Lewycky]

Chris Lattner wrote:
> 
> On Sep 7, 2009, at 6:23 PM, Nick Lewycky wrote:
> 
>> Author: nicholas
>> Date: Mon Sep  7 20:23:52 2009
>> New Revision: 81179
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=81179&view=rev
>> Log:
>> Verify types. Invalid types can be constructed when assertions are off.
>>
>> Make the verifier more robust by avoiding unprotected cast<> calls. 
>> Notably,
>> Assert1(isa<>); cast<> is not safe as Assert1 does not terminate the 
>> program.
> 
> Please add "isValidElementType" to LangRef type classifications and 
> clarify the operation constraints as appropriate.

Okay, I'll do that later, but soon. This patch isn't changing the 
language, but you're right that the LangRef isn't being as clear as it 
should.

>> +    void CheckFailed(const Twine &Message, const Type* T1,
>> +                     const Type* T2 = 0, const Type* T3 = 0) {
> 
> Please don't put the *'s in the wrong place.

Fixed, and fixed where I copied it from.

>> -  // Check that all of the operands of the PHI node have the same 
>> type as the
>> -  // result.
>> -  for (unsigned i = 0, e = PN.getNumIncomingValues(); i != e; ++i)
>> +  // Check that all of the values of the PHI node have the same type 
>> as the
>> +  // result, and that the incoming blocks are really basic blocks.
>> +  for (unsigned i = 0, e = PN.getNumIncomingValues(); i != e; ++i) {
>>     Assert1(PN.getType() == PN.getIncomingValue(i)->getType(),
>>             "PHI node operands are not the same type as the result!", 
>> &PN);
>> +    Assert1(PN.getOperand(PHINode::getOperandNumForIncomingBlock(i)),
>> +            "PHI node incoming block is not a BasicBlock!", &PN);
> 
> How is this checking that the operand is a basic block?  At the least, 
> this should have a comment.

Whoa, missing isa<BasicBlock>!

>> void Verifier::visitLoadInst(LoadInst &LI) {
>> -  const Type *ElTy =
>> -    cast<PointerType>(LI.getOperand(0)->getType())->getElementType();
>> -  Assert2(ElTy == LI.getType(),
>> -          "Load result type does not match pointer operand type!", 
>> &LI, ElTy);
>> -  Assert1(ElTy != Type::getMetadataTy(LI.getContext()),
>> -          "Can't load metadata!", &LI);
>> +  const PointerType *PTy = 
>> dyn_cast<PointerType>(LI.getOperand(0)->getType());
>> +  Assert1(PTy, "Load operand must be a pointer.", &LI);
>> +  if (PTy) {
>> +    const Type *ElTy = PTy->getElementType();
>> +    Assert2(ElTy == LI.getType(),
>> +            "Load result type does not match pointer operand type!", 
>> &LI, ElTy);
>> +    Assert1(ElTy != Type::getMetadataTy(LI.getContext()),
>> +            "Can't load metadata!", &LI);
>> +  }
> 
> This isn't needed.  Assert1 does a "return" on error, many of the other 
> changes aren't needed either.

Thanks for pointing this out. I've reverted the needless changes.

>> +void Verifier::VerifyType(const Type *Ty) {
>> +  // We insert complex types into CheckedTypes even if they failed 
>> verification
>> +  // to prevent emitting messages about them multiple times if
> 
> Half formed thought.

Fixed.

>> +
>> +  switch (Ty->getTypeID()) {
>> +  case Type::FunctionTyID: {
>> +    if (!CheckedTypes.insert(Ty)) return;
> 
> Why don't you do the insert before the switch?

Because I don't want to insert the integers, double, labels, etc. They 
all go through the default case.

>> +    const FunctionType *FTy = cast<FunctionType>(Ty);
>> +
>> +    const Type *RetTy = FTy->getReturnType();
>> +    Assert2(FunctionType::isValidReturnType(RetTy),
>> +            "Function type with invalid return type", RetTy, FTy);
>> +    VerifyType(RetTy);
>> +
>> +    for (int i = 0, e = FTy->getNumParams(); i != e; ++i) {
> 
> Please use 'unsigned i' here and in other places.

Fixed.

Thanks for the review!

Nick