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

[Chris Lattner]

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.

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

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


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

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

> +
> +  switch (Ty->getTypeID()) {
> +  case Type::FunctionTyID: {
> +    if (!CheckedTypes.insert(Ty)) return;

Why don't you do the insert before the switch?


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

Thanks for working on this!

-Chris