PNaCl ABI checker: Check for normal form introduced by ReplacePtrsWithInts

Move most of the per-instruction checking in
PNaClABIVerifyFunctions::runOnFunction() to a top-level function so
that it can do an early exit to reject the instruction.  Reporting
just a single error per instruction makes the test expectations easier
to understand and update.

Remove the check for metadata types.  Metadata won't be part of
PNaCl's stable ABI, so if the metadata-rejecting check is disabled for
debugging purpose, we don't really need to recursively check the
metadata's types.

This lets us remove the recursive checks of Constants and types.  We
now only accept a very restricted, non-recursive set of Constants
inside functions and global variable initialisers.  This means
PNaClABITypeChecker doesn't need to be stateful any more, and its
methods can become static.

This change also fixes a hole where the operands of "switch"
instructions weren't fully checked.

Add a test to replace-ptrs-with-ints.ll to ensure that "undef" doesn't
appear directly in loads and stores.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3343
TEST=PNaCl toolchain trybots + GCC torture tests + LLVM test suite

Review URL: https://codereview.chromium.org/15780014

14 files changed, 574 insertions, 346 deletions

diff --git a/lib/Analysis/NaCl/PNaClABITypeChecker.cpp b/lib/Analysis/NaCl/PNaClABITypeChecker.cpp
index 7e8c269717..8749abcaa6 100644
--- a/lib/Analysis/NaCl/PNaClABITypeChecker.cpp
+++ b/lib/Analysis/NaCl/PNaClABITypeChecker.cpp
@@ -1,4 +1,4 @@
-//===- PNaClABICheckTypes.h - Verify PNaCl ABI rules --------===//
+//===- PNaClABITypeChecker.cpp - Verify PNaCl ABI rules -------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -21,7 +21,7 @@
 using namespace llvm;
 
 bool PNaClABITypeChecker::isValidParamType(const Type *Ty) {
-  if (!isValidType(Ty))
+  if (!isValidScalarType(Ty))
     return false;
   if (const IntegerType *IntTy = dyn_cast<IntegerType>(Ty)) {
     // PNaCl requires function arguments and return values to be 32
@@ -47,113 +47,18 @@ bool PNaClABITypeChecker::isValidFunctionType(const FunctionType *FTy) {
   return true;
 }
 
-bool PNaClABITypeChecker::isValidType(const Type *Ty) {
-  if (VisitedTypes.count(Ty))
-    return VisitedTypes[Ty];
-
-  unsigned Width;
-  bool Valid = false;
+bool PNaClABITypeChecker::isValidScalarType(const Type *Ty) {
   switch (Ty->getTypeID()) {
-    // Allowed primitive types
+    case Type::IntegerTyID: {
+      unsigned Width = cast<const IntegerType>(Ty)->getBitWidth();
+      return Width == 1 || Width == 8 || Width == 16 ||
+             Width == 32 || Width == 64;
+    }
     case Type::VoidTyID:
     case Type::FloatTyID:
     case Type::DoubleTyID:
-    case Type::LabelTyID:
-    case Type::MetadataTyID:
-      Valid = true;
-      break;
-      // Disallowed primitive types
-    case Type::HalfTyID:
-    case Type::X86_FP80TyID:
-    case Type::FP128TyID:
-    case Type::PPC_FP128TyID:
-    case Type::X86_MMXTyID:
-      Valid = false;
-      break;
-      // Derived types
-    case Type::VectorTyID:
-      Valid = false;
-      break;
-    case Type::IntegerTyID:
-      Width = cast<const IntegerType>(Ty)->getBitWidth();
-      Valid = (Width == 1 || Width == 8 || Width == 16 ||
-               Width == 32 || Width == 64);
-      break;
-    case Type::FunctionTyID:
-      Valid = isValidFunctionType(cast<FunctionType>(Ty));
-      break;
-    case Type::StructTyID:
-    case Type::ArrayTyID:
-    case Type::PointerTyID:
-      // These types are valid if their contained or pointed-to types are
-      // valid. Since struct/pointer subtype relationships may be circular,
-      // mark the current type as valid to avoid infinite recursion
-      Valid = true;
-      VisitedTypes[Ty] = true;
-      for (Type::subtype_iterator I = Ty->subtype_begin(),
-               E = Ty->subtype_end(); I != E; ++I)
-        Valid &= isValidType(*I);
-      break;
-      // Handle NumTypeIDs, and no default case,
-      // so we get a warning if new types are added
-    case Type::NumTypeIDs:
-      Valid = false;
-      break;
-  }
-
-  VisitedTypes[Ty] = Valid;
-  return Valid;
-}
-
-Type *PNaClABITypeChecker::checkTypesInConstant(const Constant *V) {
-  if (!V) return NULL;
-  if (VisitedConstants.count(V))
-    return VisitedConstants[V];
-
-  if (!isValidType(V->getType())) {
-    VisitedConstants[V] = V->getType();
-    return V->getType();
-  }
-
-  // Check for BlockAddress because it contains a non-Constant
-  // BasicBlock operand.
-  // TODO(mseaborn): This produces an error which is misleading
-  // because it complains about the type being "i8*".  It should
-  // instead produce an error saying that BlockAddress and computed
-  // gotos are not allowed.
-  if (isa<BlockAddress>(V)) {
-    VisitedConstants[V] = V->getType();
-    return V->getType();
-  }
-
-  // Operand values must also be valid. Values may be circular, so
-  // mark the current value as valid to avoid infinite recursion.
-  VisitedConstants[V] = NULL;
-  for (Constant::const_op_iterator I = V->op_begin(),
-           E = V->op_end(); I != E; ++I) {
-    Type *Invalid = checkTypesInConstant(cast<Constant>(*I));
-    if (Invalid) {
-      VisitedConstants[V] = Invalid;
-      return Invalid;
-    }
-  }
-  VisitedConstants[V] = NULL;
-  return NULL;
-}
-
-
-// MDNodes don't support the same way of iterating over operands that Users do
-Type *PNaClABITypeChecker::checkTypesInMDNode(const MDNode *N) {
-  if (VisitedConstants.count(N))
-    return VisitedConstants[N];
-
-  for (unsigned i = 0, e = N->getNumOperands(); i != e; i++) {
-    if (Value *Op = N->getOperand(i)) {
-      if (Type *Invalid = checkTypesInConstant(dyn_cast<Constant>(Op))) {
-        VisitedConstants[N] = Invalid;
-        return Invalid;
-      }
-    }
+      return true;
+    default:
+      return false;
   }
-  return NULL;
 }
diff --git a/lib/Analysis/NaCl/PNaClABITypeChecker.h b/lib/Analysis/NaCl/PNaClABITypeChecker.h
index cd898e2328..ac3cf850e5 100644
--- a/lib/Analysis/NaCl/PNaClABITypeChecker.h
+++ b/lib/Analysis/NaCl/PNaClABITypeChecker.h
@@ -1,4 +1,4 @@
-//===- CheckTypes.h - Verify PNaCl ABI rules --------===//
+//===- PNaClABITypeChecker.h - Verify PNaCl ABI rules ---------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -20,29 +20,18 @@
 #include "llvm/Support/raw_ostream.h"
 
 namespace llvm {
-class Constant;
 class FunctionType;
-class MDNode;
-class Value;
 
 class PNaClABITypeChecker {
   // Returns true if Ty is a valid argument or return value type for PNaCl.
-  bool isValidParamType(const Type *Ty);
-
-  // Returns true if Ty is a valid function type for PNaCl.
-  bool isValidFunctionType(const FunctionType *FTy);
+  static bool isValidParamType(const Type *Ty);
 
  public:
-  // Returns true if Ty is a valid type for PNaCl.
-  bool isValidType(const Type *Ty);
-
-  // If the value contains an invalid type, return a pointer to the type.
-  // Return null if there are no invalid types.
-  Type *checkTypesInConstant(const Constant *V);
+  // Returns true if Ty is a valid function type for PNaCl.
+  static bool isValidFunctionType(const FunctionType *FTy);
 
-  // If the Metadata node contains an invalid type, return a pointer to the
-  // type. Return null if there are no invalid types.
-  Type *checkTypesInMDNode(const MDNode *V);
+  // Returns true if Ty is a valid non-derived type for PNaCl.
+  static bool isValidScalarType(const Type *Ty);
 
   // There's no built-in way to get the name of a type, so use a
   // string ostream to print it.
@@ -52,13 +41,6 @@ class PNaClABITypeChecker {
     T->print(N);
     return N.str();
   }
-
- private:
-  // To avoid walking constexprs and types multiple times, keep a cache of
-  // what we have seen. This is also used to prevent infinite recursion e.g.
-  // in case of structures like linked lists with pointers to themselves.
-  DenseMap<const Value*, Type*> VisitedConstants;
-  DenseMap<const Type*, bool> VisitedTypes;
 };
 } // namespace llvm
 
diff --git a/lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp b/lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp
index ad01ae58a2..edb0e5fad2 100644
--- a/lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp
+++ b/lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp
@@ -1,4 +1,4 @@
-//===- PNaClABIVerifyFunctions.cpp - Verify PNaCl ABI rules --------===//
+//===- PNaClABIVerifyFunctions.cpp - Verify PNaCl ABI rules ---------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -12,13 +12,14 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "llvm/Pass.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Analysis/NaCl.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Instructions.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Metadata.h"
+#include "llvm/Pass.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include "PNaClABITypeChecker.h"
@@ -51,15 +52,17 @@ class PNaClABIVerifyFunctions : public FunctionPass {
   virtual void print(raw_ostream &O, const Module *M) const;
  private:
   bool IsWhitelistedMetadata(unsigned MDKind);
-  PNaClABITypeChecker TC;
+  const char *checkInstruction(const Instruction *Inst);
   PNaClABIErrorReporter *Reporter;
   bool ReporterIsOwned;
 };
 
+} // and anonymous namespace
+
 // There's no built-in way to get the name of an MDNode, so use a
 // string ostream to print it.
-std::string getMDNodeString(unsigned Kind,
-                            const SmallVectorImpl<StringRef>& MDNames) {
+static std::string getMDNodeString(unsigned Kind,
+                                   const SmallVectorImpl<StringRef> &MDNames) {
   std::string MDName;
   raw_string_ostream N(MDName);
   if (Kind < MDNames.size()) {
@@ -70,135 +73,274 @@ std::string getMDNodeString(unsigned Kind,
   return N.str();
 }
 
-} // and anonymous namespace
-
 bool PNaClABIVerifyFunctions::IsWhitelistedMetadata(unsigned MDKind) {
   return MDKind == LLVMContext::MD_dbg && PNaClABIAllowDebugMetadata;
 }
 
+// A valid pointer type is either:
+//  * a pointer to a valid PNaCl scalar type, or
+//  * a function pointer (with valid argument and return types).
+static bool isValidPointerType(Type *Ty) {
+  if (PointerType *PtrTy = dyn_cast<PointerType>(Ty)) {
+    if (PNaClABITypeChecker::isValidScalarType(PtrTy->getElementType()))
+      return true;
+    if (FunctionType *FTy = dyn_cast<FunctionType>(PtrTy->getElementType()))
+      return PNaClABITypeChecker::isValidFunctionType(FTy);
+  }
+  return false;
+}
+
+static bool isIntrinsicFunc(const Value *Val) {
+  if (const Function *F = dyn_cast<Function>(Val))
+    return F->isIntrinsic();
+  return false;
+}
+
+// InherentPtrs may be referenced by casts -- PtrToIntInst and
+// BitCastInst -- that produce NormalizedPtrs.
+//
+// InherentPtrs exclude intrinsic functions in order to prevent taking
+// the address of an intrinsic function.  InherentPtrs include
+// intrinsic calls because some intrinsics return pointer types
+// (e.g. nacl.read.tp returns i8*).
+static bool isInherentPtr(const Value *Val) {
+  return isa<AllocaInst>(Val) ||
+         (isa<GlobalValue>(Val) && !isIntrinsicFunc(Val)) ||
+         isa<IntrinsicInst>(Val);
+}
+
+// NormalizedPtrs may be used where pointer types are required -- for
+// loads, stores, etc.  Note that this excludes ConstantExprs,
+// ConstantPointerNull and UndefValue.
+static bool isNormalizedPtr(const Value *Val) {
+  if (!isValidPointerType(Val->getType()))
+    return false;
+  // The bitcast must also be a bitcast of an InherentPtr, but we
+  // check that when visiting the bitcast instruction.
+  return isa<IntToPtrInst>(Val) || isa<BitCastInst>(Val) || isInherentPtr(Val);
+}
+
+static bool isValidScalarOperand(const Value *Val) {
+  // The types of Instructions and Arguments are checked elsewhere
+  // (when visiting the Instruction or the Function).  BasicBlocks are
+  // included here because branch instructions have BasicBlock
+  // operands.
+  if (isa<Instruction>(Val) || isa<Argument>(Val) || isa<BasicBlock>(Val))
+    return true;
+
+  // Allow some Constants.  Note that this excludes ConstantExprs.
+  return PNaClABITypeChecker::isValidScalarType(Val->getType()) &&
+         (isa<ConstantInt>(Val) ||
+          isa<ConstantFP>(Val) ||
+          isa<UndefValue>(Val));
+}
+
+// Check the instruction's opcode and its operands.  The operands may
+// require opcode-specific checking.
+//
+// This returns an error string if the instruction is rejected, or
+// NULL if the instruction is allowed.
+const char *PNaClABIVerifyFunctions::checkInstruction(const Instruction *Inst) {
+  // If the instruction has a single pointer operand, PtrOperandIndex is
+  // set to its operand index.
+  unsigned PtrOperandIndex = -1;
+
+  switch (Inst->getOpcode()) {
+    // Disallowed instructions. Default is to disallow.
+    // We expand GetElementPtr out into arithmetic.
+    case Instruction::GetElementPtr:
+    // VAArg is expanded out by ExpandVarArgs.
+    case Instruction::VAArg:
+    // Zero-cost C++ exception handling is not supported yet.
+    case Instruction::Invoke:
+    case Instruction::LandingPad:
+    case Instruction::Resume:
+    // indirectbr may interfere with streaming
+    case Instruction::IndirectBr:
+    // No vector instructions yet
+    case Instruction::ExtractElement:
+    case Instruction::InsertElement:
+    case Instruction::ShuffleVector:
+    // ExtractValue and InsertValue operate on struct values.
+    case Instruction::ExtractValue:
+    case Instruction::InsertValue:
+      return "bad instruction opcode";
+    default:
+      return "unknown instruction opcode";
+
+    // Terminator instructions
+    case Instruction::Ret:
+    case Instruction::Br:
+    case Instruction::Unreachable:
+    // Binary operations
+    case Instruction::Add:
+    case Instruction::FAdd:
+    case Instruction::Sub:
+    case Instruction::FSub:
+    case Instruction::Mul:
+    case Instruction::FMul:
+    case Instruction::UDiv:
+    case Instruction::SDiv:
+    case Instruction::FDiv:
+    case Instruction::URem:
+    case Instruction::SRem:
+    case Instruction::FRem:
+    // Bitwise binary operations
+    case Instruction::Shl:
+    case Instruction::LShr:
+    case Instruction::AShr:
+    case Instruction::And:
+    case Instruction::Or:
+    case Instruction::Xor:
+    // Memory instructions
+    case Instruction::Fence:
+    // Conversion operations
+    case Instruction::Trunc:
+    case Instruction::ZExt:
+    case Instruction::SExt:
+    case Instruction::FPTrunc:
+    case Instruction::FPExt:
+    case Instruction::FPToUI:
+    case Instruction::FPToSI:
+    case Instruction::UIToFP:
+    case Instruction::SIToFP:
+    // Other operations
+    case Instruction::ICmp:
+    case Instruction::FCmp:
+    case Instruction::PHI:
+    case Instruction::Select:
+      break;
+
+    // Memory accesses.
+    case Instruction::Load:
+    case Instruction::AtomicCmpXchg:
+    case Instruction::AtomicRMW:
+      PtrOperandIndex = 0;
+      if (!isNormalizedPtr(Inst->getOperand(PtrOperandIndex)))
+        return "bad pointer";
+      break;
+    case Instruction::Store:
+      PtrOperandIndex = 1;
+      if (!isNormalizedPtr(Inst->getOperand(PtrOperandIndex)))
+        return "bad pointer";
+      break;
+
+    // Casts.
+    case Instruction::BitCast:
+      if (Inst->getType()->isPointerTy()) {
+        PtrOperandIndex = 0;
+        if (!isInherentPtr(Inst->getOperand(PtrOperandIndex)))
+          return "operand not InherentPtr";
+      }
+      break;
+    case Instruction::IntToPtr:
+      if (!cast<IntToPtrInst>(Inst)->getSrcTy()->isIntegerTy(32))
+        return "non-i32 inttoptr";
+      break;
+    case Instruction::PtrToInt:
+      PtrOperandIndex = 0;
+      if (!isInherentPtr(Inst->getOperand(PtrOperandIndex)))
+        return "operand not InherentPtr";
+      if (!Inst->getType()->isIntegerTy(32))
+        return "non-i32 ptrtoint";
+      break;
+
+    case Instruction::Alloca: {
+      ArrayType *Ty = dyn_cast<ArrayType>(cast<AllocaInst>(Inst)
+                                          ->getType()->getElementType());
+      if (!Ty || !Ty->getElementType()->isIntegerTy(8))
+        return "non-i8-array alloca";
+      break;
+    }
+
+    case Instruction::Call:
+      if (cast<CallInst>(Inst)->isInlineAsm())
+        return "inline assembly";
+
+      // Intrinsic calls can have multiple pointer arguments and
+      // metadata arguments, so handle them specially.
+      if (const IntrinsicInst *Call = dyn_cast<IntrinsicInst>(Inst)) {
+        for (unsigned ArgNum = 0, E = Call->getNumArgOperands();
+             ArgNum < E; ++ArgNum) {
+          const Value *Arg = Call->getArgOperand(ArgNum);
+          if (!(isValidScalarOperand(Arg) ||
+                isNormalizedPtr(Arg) ||
+                isa<MDNode>(Arg)))
+            return "bad intrinsic operand";
+        }
+        // Allow the instruction and skip the later checks.
+        return NULL;
+      }
+
+      // The callee is the last operand.
+      PtrOperandIndex = Inst->getNumOperands() - 1;
+      if (!isNormalizedPtr(Inst->getOperand(PtrOperandIndex)))
+        return "bad function callee operand";
+      break;
+
+    case Instruction::Switch: {
+      // SwitchInst represents switch cases using array and vector
+      // constants, which we normally reject, so we must check
+      // SwitchInst specially here.
+      const SwitchInst *Switch = cast<SwitchInst>(Inst);
+      if (!isValidScalarOperand(Switch->getCondition()))
+        return "bad switch condition";
+
+      // SwitchInst requires the cases to be ConstantInts, but it
+      // doesn't require their types to be the same as the condition
+      // value, so check all the cases too.
+      for (SwitchInst::ConstCaseIt Case = Switch->case_begin(),
+             E = Switch->case_end(); Case != E; ++Case) {
+        IntegersSubset CaseRanges = Case.getCaseValueEx();
+        for (unsigned I = 0, E = CaseRanges.getNumItems(); I < E ; ++I) {
+          if (!isValidScalarOperand(
+                  CaseRanges.getItem(I).getLow().toConstantInt()) ||
+              !isValidScalarOperand(
+                  CaseRanges.getItem(I).getHigh().toConstantInt())) {
+            return "bad switch case";
+          }
+        }
+      }
+
+      // Allow the instruction and skip the later checks.
+      return NULL;
+    }
+  }
+
+  // Check the instruction's operands.  We have already checked any
+  // pointer operands.  Any remaining operands must be scalars.
+  for (unsigned OpNum = 0, E = Inst->getNumOperands(); OpNum < E; ++OpNum) {
+    if (OpNum != PtrOperandIndex &&
+        !isValidScalarOperand(Inst->getOperand(OpNum)))
+      return "bad operand";
+  }
+  // Allow the instruction.
+  return NULL;
+}
+
 bool PNaClABIVerifyFunctions::runOnFunction(Function &F) {
   SmallVector<StringRef, 8> MDNames;
   F.getContext().getMDKindNames(MDNames);
 
-  // TODO: only report one error per instruction?
   for (Function::const_iterator FI = F.begin(), FE = F.end();
            FI != FE; ++FI) {
     for (BasicBlock::const_iterator BBI = FI->begin(), BBE = FI->end();
              BBI != BBE; ++BBI) {
-      switch (BBI->getOpcode()) {
-        // Disallowed instructions. Default is to disallow.
-        default:
-        // We expand GetElementPtr out into arithmetic.
-        case Instruction::GetElementPtr:
-        // VAArg is expanded out by ExpandVarArgs.
-        case Instruction::VAArg:
-        // Zero-cost C++ exception handling is not supported yet.
-        case Instruction::Invoke:
-        case Instruction::LandingPad:
-        case Instruction::Resume:
-        // indirectbr may interfere with streaming
-        case Instruction::IndirectBr:
-        // No vector instructions yet
-        case Instruction::ExtractElement:
-        case Instruction::InsertElement:
-        case Instruction::ShuffleVector:
-        // ExtractValue and InsertValue operate on struct values.
-        case Instruction::ExtractValue:
-        case Instruction::InsertValue:
-          Reporter->addError() << "Function " << F.getName() <<
-              " has disallowed instruction: " <<
-              BBI->getOpcodeName() << "\n";
-          break;
-
-        // Terminator instructions
-        case Instruction::Ret:
-        case Instruction::Br:
-        case Instruction::Switch:
-        case Instruction::Unreachable:
-        // Binary operations
-        case Instruction::Add:
-        case Instruction::FAdd:
-        case Instruction::Sub:
-        case Instruction::FSub:
-        case Instruction::Mul:
-        case Instruction::FMul:
-        case Instruction::UDiv:
-        case Instruction::SDiv:
-        case Instruction::FDiv:
-        case Instruction::URem:
-        case Instruction::SRem:
-        case Instruction::FRem:
-        // Bitwise binary operations
-        case Instruction::Shl:
-        case Instruction::LShr:
-        case Instruction::AShr:
-        case Instruction::And:
-        case Instruction::Or:
-        case Instruction::Xor:
-        // Memory instructions
-        case Instruction::Alloca:
-        case Instruction::Load:
-        case Instruction::Store:
-        case Instruction::Fence:
-        case Instruction::AtomicCmpXchg:
-        case Instruction::AtomicRMW:
-        // Conversion operations
-        case Instruction::Trunc:
-        case Instruction::ZExt:
-        case Instruction::SExt:
-        case Instruction::FPTrunc:
-        case Instruction::FPExt:
-        case Instruction::FPToUI:
-        case Instruction::FPToSI:
-        case Instruction::UIToFP:
-        case Instruction::SIToFP:
-        case Instruction::PtrToInt:
-        case Instruction::IntToPtr:
-        case Instruction::BitCast:
-        // Other operations
-        case Instruction::ICmp:
-        case Instruction::FCmp:
-        case Instruction::PHI:
-        case Instruction::Select:
-          break;
-        case Instruction::Call:
-          if (cast<CallInst>(BBI)->isInlineAsm()) {
-            Reporter->addError() << "Function " << F.getName() <<
-                " contains disallowed inline assembly\n";
-          }
-          break;
+      const Instruction *Inst = BBI;
+      // Check the instruction opcode first.  This simplifies testing,
+      // because some instruction opcodes must be rejected out of hand
+      // (regardless of the instruction's result type) and the tests
+      // check the reason for rejection.
+      const char *Error = checkInstruction(BBI);
+      // Check the instruction's result type.
+      if (!Error && !(PNaClABITypeChecker::isValidScalarType(Inst->getType()) ||
+                      isNormalizedPtr(Inst) ||
+                      isa<AllocaInst>(Inst))) {
+        Error = "bad result type";
       }
-      // Check the types. First check the type of the instruction.
-      if (!TC.isValidType(BBI->getType())) {
+      if (Error) {
         Reporter->addError() << "Function " << F.getName() <<
-            " has instruction with disallowed type: " <<
-            PNaClABITypeChecker::getTypeName(BBI->getType()) << "\n";
-      }
-
-      // Check the instruction operands. Operands which are Instructions will
-      // be checked on their own here, and GlobalValues will be checked by the
-      // Module verifier. That leaves Constants.
-      // Switches are implemented in the in-memory IR with vectors, so don't
-      // check them.
-      if (!isa<SwitchInst>(*BBI))
-        for (User::const_op_iterator OI = BBI->op_begin(), OE = BBI->op_end();
-             OI != OE; OI++) {
-          if (isa<Constant>(OI) && !isa<GlobalValue>(OI)) {
-            Type *T = TC.checkTypesInConstant(cast<Constant>(*OI));
-            if (T) {
-              Reporter->addError() << "Function " << F.getName() <<
-                  " has instruction operand with disallowed type: " <<
-                  PNaClABITypeChecker::getTypeName(T) << "\n";
-            }
-          }
-        }
-
-      for (User::const_op_iterator OI = BBI->op_begin(), OE = BBI->op_end();
-           OI != OE; OI++) {
-        if (isa<ConstantExpr>(OI)) {
-          Reporter->addError() << "Function " << F.getName() <<
-              " contains disallowed ConstantExpr\n";
-        }
+          " disallowed: " << Error << ": " << *BBI << "\n";
       }
 
       // Check instruction attachment metadata.
@@ -211,15 +353,6 @@ bool PNaClABIVerifyFunctions::runOnFunction(Function &F) {
               << "Function " << F.getName()
               << " has disallowed instruction metadata: "
               << getMDNodeString(MDForInst[i].first, MDNames) << "\n";
-        } else {
-          // If allowed, check the types hiding in the metadata.
-          Type *T = TC.checkTypesInMDNode(MDForInst[i].second);
-          if (T) {
-            Reporter->addError()
-                << "Function " << F.getName()
-                << " has instruction metadata containing disallowed type: "
-                << PNaClABITypeChecker::getTypeName(T) << "\n";
-          }
         }
       }
     }
diff --git a/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp b/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
index 1e878a1fe8..cbe7eb833f 100644
--- a/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
+++ b/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
@@ -1,4 +1,4 @@
-//===- PNaClABIVerifyModule.cpp - Verify PNaCl ABI rules --------===//
+//===- PNaClABIVerifyModule.cpp - Verify PNaCl ABI rules ------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -67,7 +67,6 @@ class PNaClABIVerifyModule : public ModulePass {
   bool isWhitelistedIntrinsic(const Function *F, unsigned ID);
   bool isWhitelistedMetadata(const NamedMDNode *MD);
   void checkGlobalIsFlattened(const GlobalVariable *GV);
-  PNaClABITypeChecker TC;
   PNaClABIErrorReporter *Reporter;
   bool ReporterIsOwned;
 };
@@ -353,7 +352,8 @@ bool PNaClABIVerifyModule::runOnModule(Module &M) {
     // Check types of functions and their arguments.  Not necessary
     // for intrinsics, whose types are fixed anyway, and which have
     // argument types that we disallow such as i8.
-    if (!MI->isIntrinsic() && !TC.isValidType(MI->getType())) {
+    if (!MI->isIntrinsic() &&
+        !PNaClABITypeChecker::isValidFunctionType(MI->getFunctionType())) {
       Reporter->addError() << "Function " << MI->getName()
           << " has disallowed type: "
           << PNaClABITypeChecker::getTypeName(MI->getFunctionType())
@@ -374,15 +374,6 @@ bool PNaClABIVerifyModule::runOnModule(Module &M) {
     if (!isWhitelistedMetadata(I)) {
       Reporter->addError() << "Named metadata node " << I->getName()
                            << " is disallowed\n";
-    } else {
-      // Check the types in the metadata.
-      for (unsigned i = 0, e = I->getNumOperands(); i != e; i++) {
-        if (Type *T = TC.checkTypesInMDNode(I->getOperand(i))) {
-          Reporter->addError() << "Named metadata node " << I->getName()
-                               << " refers to disallowed type: "
-                               << PNaClABITypeChecker::getTypeName(T) << "\n";
-        }
-      }
     }
   }
 
diff --git a/test/NaCl/PNaClABI/abi-debug-info.ll b/test/NaCl/PNaClABI/abi-debug-info.ll
new file mode 100644
index 0000000000..95efa0ced7
--- /dev/null
+++ b/test/NaCl/PNaClABI/abi-debug-info.ll
@@ -0,0 +1,37 @@
+; RUN: pnacl-abicheck -pnaclabi-allow-dev-intrinsics=0 < %s | FileCheck %s
+; RUN: pnacl-abicheck -pnaclabi-allow-dev-intrinsics=0 \
+; RUN:   -pnaclabi-allow-debug-metadata < %s | FileCheck %s --check-prefix=DBG
+; RUN: pnacl-abicheck -pnaclabi-allow-dev-intrinsics=1 < %s | \
+; RUN:   FileCheck %s --check-prefix=DBG
+
+
+; DBG-NOT: disallowed
+
+
+declare void @llvm.dbg.declare(metadata, metadata)
+declare void @llvm.dbg.value(metadata, i64, metadata)
+
+; CHECK: Function llvm.dbg.declare is a disallowed LLVM intrinsic
+; CHECK: Function llvm.dbg.value is a disallowed LLVM intrinsic
+
+
+define void @debug_declare(i32 %val) {
+  ; We normally expect llvm.dbg.declare to be used on an alloca.
+  %var = alloca [4 x i8]
+  tail call void @llvm.dbg.declare(metadata !{[4 x i8]* %var}, metadata !{})
+  tail call void @llvm.dbg.declare(metadata !{i32 %val}, metadata !{})
+  ret void
+}
+
+define void @debug_value(i32 %ptr_as_int, i32 %val) {
+  %ptr = inttoptr i32 %ptr_as_int to i8*
+  tail call void @llvm.dbg.value(metadata !{i8* %ptr}, i64 2, metadata !{})
+  tail call void @llvm.dbg.value(metadata !{i32 %val}, i64 1, metadata !{})
+  ret void
+}
+
+; FileCheck gives an error if its input file is empty, so ensure that
+; the output of pnacl-abicheck is non-empty by generating at least one
+; error.
+declare void @bad_func(ppc_fp128 %bad_arg)
+; DBG: Function bad_func has disallowed type: void (ppc_fp128)
diff --git a/test/NaCl/PNaClABI/abi-metadata.ll b/test/NaCl/PNaClABI/abi-metadata.ll
index 4734c25973..751a3d3673 100644
--- a/test/NaCl/PNaClABI/abi-metadata.ll
+++ b/test/NaCl/PNaClABI/abi-metadata.ll
@@ -2,12 +2,13 @@
 ; RUN: pnacl-abicheck -pnaclabi-allow-debug-metadata < %s | FileCheck %s --check-prefix=DEBUG
 
 
-; If the metadata is allowed we want to check for types.
-; We have a hacky way to test this. The -allow-debug-metadata whitelists debug
-; metadata.  That allows us to check types within debug metadata, even though
-; debug metadata normally does not have illegal types.
+; Metadata is not part of the PNaCl's stable ABI, so normally the ABI
+; checker rejects metadata entirely.  However, for debugging support,
+; pre-finalized pexes may contain metadata.  When checking a
+; pre-finalized pexe, the ABI checker does not check the types in the
+; metadata.
+
 ; DEBUG-NOT: Named metadata node llvm.dbg.cu is disallowed
-; DEBUG: Named metadata node llvm.dbg.cu refers to disallowed type: half
 ; CHECK: Named metadata node llvm.dbg.cu is disallowed
 !llvm.dbg.cu = !{!0}
 !0 = metadata !{ half 0.0}
diff --git a/test/NaCl/PNaClABI/abi-small-arguments.ll b/test/NaCl/PNaClABI/abi-small-arguments.ll
index 890b84c42a..ce698e7d47 100644
--- a/test/NaCl/PNaClABI/abi-small-arguments.ll
+++ b/test/NaCl/PNaClABI/abi-small-arguments.ll
@@ -21,25 +21,32 @@ define i8 @return_i8() {
 ; CHECK: Function return_i8 has disallowed type:
 
 
-; Direct calls currently do not produce errors because the functions
-; are deemed to have already been flagged.
-; CHECK-NOT: disallowed
 define void @bad_direct_calls() {
   call void @arg_i1(i1 0)
+; CHECK: bad function callee operand: call void @arg_i1
+
   call void @arg_i16(i32 0, i16 0)
+; CHECK-NEXT: bad function callee operand: call void @arg_i16
+
   %result1 = call i1 @return_i1()
+; CHECK-NEXT: bad function callee operand: {{.*}} call i1 @return_i1
+
   %result2 = call i8 @return_i8()
+; CHECK-NEXT: bad function callee operand: {{.*}} call i8 @return_i8
+
   ret void
 }
 
 define void @bad_indirect_calls(i32 %ptr) {
   %func1 = inttoptr i32 %ptr to void (i8)*
+; CHECK: bad result type: %func1
   call void %func1(i8 0)
-; CHECK: Function bad_indirect_calls has instruction with disallowed type: void (i8)*
+; CHECK: bad function callee operand: {{.*}} %func1
 
   %func2 = inttoptr i32 %ptr to i16 ()*
+; CHECK: bad result type: %func2
   %result3 = call i16 %func2()
-; CHECK: Function bad_indirect_calls has instruction with disallowed type: i16 ()*
+; CHECK: bad function callee operand: {{.*}} %func2
 
   ret void
 }
diff --git a/test/NaCl/PNaClABI/abi-stripped-pointers.ll b/test/NaCl/PNaClABI/abi-stripped-pointers.ll
new file mode 100644
index 0000000000..c994b7d009
--- /dev/null
+++ b/test/NaCl/PNaClABI/abi-stripped-pointers.ll
@@ -0,0 +1,131 @@
+; RUN: pnacl-abicheck < %s | FileCheck %s
+
+; This test checks that the PNaCl ABI verifier enforces the normal
+; form introduced by the ReplacePtrsWithInts pass.
+
+
+@var = global [4 x i8] c"xxxx"
+@ptr = global i32 ptrtoint ([4 x i8]* @var to i32)
+
+declare i8* @llvm.nacl.read.tp()
+
+
+define void @pointer_arg(i8* %arg) {
+  ret void
+}
+; CHECK: Function pointer_arg has disallowed type
+
+define i8* @pointer_return() {
+  unreachable
+}
+; CHECK-NEXT: Function pointer_return has disallowed type
+
+define void @func() {
+  ret void
+}
+
+define void @func_with_arg(i32 %arg) {
+  ret void
+}
+
+
+define void @allowed_cases(i32 %arg) {
+  inttoptr i32 123 to i8*
+
+  ptrtoint [4 x i8]* @var to i32
+
+  %alloc = alloca [1 x i8]
+  ptrtoint [1 x i8]* %alloc to i32
+
+  ; These instructions may use a NormalizedPtr, which may be a global.
+  load i32* @ptr
+  store i32 123, i32* @ptr
+  cmpxchg i32* @ptr, i32 1, i32 2 seq_cst
+  atomicrmw add i32* @ptr, i32 3 seq_cst
+
+  ; A NormalizedPtr may be a bitcast.
+  %ptr_bitcast = bitcast [4 x i8]* @var to i32*
+  load i32* %ptr_bitcast
+
+  ; A NormalizedPtr may be an inttoptr.
+  %ptr_from_int = inttoptr i32 123 to i32*
+  load i32* %ptr_from_int
+
+  ; Check direct and indirect function calls.
+  %func_as_int = ptrtoint void ()* @func to i32
+  %func_ptr = inttoptr i32 %func_as_int to void ()*
+  call void %func_ptr()
+  call void @func()
+  call void @func_with_arg(i32 123)
+
+  ; Intrinsic calls may return pointers.
+  %thread_ptr = call i8* @llvm.nacl.read.tp()
+  ptrtoint i8* %thread_ptr to i32
+
+  ; Bitcasts between non-pointers are not restricted
+  bitcast i64 0 to double
+  bitcast i32 0 to float
+
+  ; ConstantInts and Arguments are allowed as operands.
+  add i32 %arg, 123
+
+  ret void
+}
+; CHECK-NOT: disallowed
+
+
+define void @bad_cases() {
+entry:
+  ptrtoint [4 x i8]* @var to i16
+; CHECK: Function bad_cases disallowed: non-i32 ptrtoint
+
+  inttoptr i16 123 to i8*
+; CHECK-NEXT: non-i32 inttoptr
+
+  %a = alloca i32
+; CHECK-NEXT: non-i8-array alloca
+
+  store i32 0, i32* null
+; CHECK-NEXT: bad pointer
+
+  store i32 0, i32* undef
+; CHECK-NEXT: bad pointer
+
+  %bc = bitcast i32* @ptr to i31*
+; CHECK-NEXT: bad result type
+  store i31 0, i31* %bc
+; CHECK-NEXT: bad pointer
+
+  ; Only one level of bitcasts is allowed.
+  %b = bitcast i32* %a to i8*
+  %c = bitcast i8* %b to i16*
+; CHECK-NEXT: operand not InherentPtr
+
+  br label %block
+block:
+  %phi1 = phi i8* [ undef, %entry ]
+; CHECK-NEXT: bad operand: %phi1
+  %phi2 = phi i32* [ undef, %entry ]
+; CHECK-NEXT: bad operand: %phi2
+
+  icmp eq i32* @ptr, @ptr
+; CHECK-NEXT: bad operand: {{.*}} icmp
+  icmp eq void ()* @func, @func
+; CHECK-NEXT: bad operand: {{.*}} icmp
+  icmp eq i31 0, 0
+; CHECK-NEXT: bad operand: {{.*}} icmp
+
+  call void null()
+; CHECK-NEXT: bad function callee operand
+
+  call void @func_with_arg(i32 ptrtoint (i32* @ptr to i32))
+; CHECK-NEXT: bad operand
+
+  ; Taking the address of an intrinsic is not allowed.
+  ptrtoint i8* ()* @llvm.nacl.read.tp to i32
+; CHECK-NEXT: operand not InherentPtr
+
+  ret void
+}
+
+; CHECK-NOT: disallowed
diff --git a/test/NaCl/PNaClABI/abi-switch.ll b/test/NaCl/PNaClABI/abi-switch.ll
new file mode 100644
index 0000000000..a3b5e631fc
--- /dev/null
+++ b/test/NaCl/PNaClABI/abi-switch.ll
@@ -0,0 +1,31 @@
+; RUN: pnacl-abicheck < %s | FileCheck %s
+
+@var = global [4 x i8] c"xxxx"
+
+
+; CHECK-NOT: disallowed
+
+define void @bad_cases() {
+  ; ConstantExprs should be rejected here.
+  switch i32 ptrtoint ([4 x i8]* @var to i32), label %next [i32 0, label %next]
+; CHECK: disallowed: bad switch condition
+next:
+
+  ; Bad integer type.
+  switch i32 0, label %next [i99 0, label %next]
+; CHECK: bad switch case
+
+  ; Bad integer type.
+  switch i32 0, label %next [i32 0, label %next
+                             i99 1, label %next]
+; CHECK: bad switch case
+
+  ; Note that the reader only allows ConstantInts in the label list.
+  ; We don't need to check the following, because the reader rejects
+  ; it:
+  ; switch i32 0, label %next [i32 ptrtoint (i32* @ptr to i32), label %next]
+
+  ret void
+}
+
+; CHECK-NOT: disallowed
diff --git a/test/NaCl/PNaClABI/abi-varargs.ll b/test/NaCl/PNaClABI/abi-varargs.ll
index 6eda0b9b53..dac94e00e0 100644
--- a/test/NaCl/PNaClABI/abi-varargs.ll
+++ b/test/NaCl/PNaClABI/abi-varargs.ll
@@ -10,4 +10,4 @@ define void @call_varargs_func(i32 %ptr) {
   call void (i32, ...)* %ptr2(i32 123)
   ret void
 }
-; CHECK: Function call_varargs_func has instruction with disallowed type: void (i32, ...)*
+; CHECK: Function call_varargs_func disallowed: bad function callee operand: call void (i32, ...)*
diff --git a/test/NaCl/PNaClABI/instructions.ll b/test/NaCl/PNaClABI/instructions.ll
index 0076c3d07c..57d1ec856b 100644
--- a/test/NaCl/PNaClABI/instructions.ll
+++ b/test/NaCl/PNaClABI/instructions.ll
@@ -1,6 +1,5 @@
 ; RUN: pnacl-abicheck < %s | FileCheck %s
 ; Test instruction opcodes allowed by PNaCl ABI
-; No testing yet of operands, types, attributes, etc
 
 target datalayout = "e-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-p:32:32:32-v128:32:32"
 target triple = "le32-unknown-nacl"
@@ -15,7 +14,7 @@ next:
 next2:
   unreachable
 ; CHECK-NOT: disallowed
-; CHECK: Function terminators has disallowed instruction: indirectbr
+; CHECK: Function terminators disallowed: bad instruction opcode: indirectbr
   indirectbr i8* undef, [label %next, label %next2]
 }
 
@@ -44,37 +43,43 @@ define void @binops() {
 
 define void @vectors() {
 ; CHECK-NOT: disallowed
-; CHECK: Function vectors has disallowed instruction: extractelement
+
+; CHECK: disallowed: bad instruction opcode: {{.*}} extractelement
   %a1 = extractelement <2 x i32> <i32 0, i32 0>, i32 0
-; CHECK: Function vectors has disallowed instruction: shufflevector
+
+; CHECK: disallowed: bad instruction opcode: {{.*}} shufflevector
   %a2 = shufflevector <2 x i32> undef, <2 x i32> undef, <2 x i32> undef
-; CHECK: Function vectors has disallowed instruction: insertelement
-; CHECK: Function vectors has instruction with disallowed type
-; CHECK: Function vectors has instruction operand with disallowed type
+
+; CHECK: disallowed: bad instruction opcode: {{.*}} insertelement
   %a3 = insertelement <2 x i32> undef, i32 1, i32 0
+
   ret void
 }
 
 define void @aggregates() {
+; CHECK-NOT: disallowed
+
 ; Aggregate operations
   %a1 = extractvalue { i32, i32 } { i32 0, i32 0 }, 0
-; CHECK-NOT: disallowed
-; CHECK: Function aggregates has disallowed instruction: extractvalue
+; CHECK: disallowed: bad instruction opcode: {{.*}} extractvalue
+
   %a2 = insertvalue {i32, float} undef, i32 1, 0
-; CHECK-NEXT: Function aggregates has disallowed instruction: insertvalue
+; CHECK-NEXT: disallowed: bad instruction opcode: {{.*}} insertvalue
+
   ret void
 }
 
 define void @memory() {
 ; Memory operations
-  %a1 = alloca i32
-  %a2 = load i32* undef
-  store i32 undef, i32* undef
+  %a1 = alloca [4 x i8]
+  %ptr = inttoptr i32 0 to i32*
+  %a2 = load i32* %ptr
+  store i32 undef, i32* %ptr
   fence acq_rel
-  %a3 = cmpxchg i32* undef, i32 undef, i32 undef acq_rel
-  %a4 = atomicrmw add i32* undef, i32 1 acquire
+  %a3 = cmpxchg i32* %ptr, i32 undef, i32 undef acq_rel
+  %a4 = atomicrmw add i32* %ptr, i32 1 acquire
 ; CHECK-NOT: disallowed
-; CHECK: Function memory has disallowed instruction: getelementptr
+; CHECK: disallowed: bad instruction opcode: {{.*}} getelementptr
   %a5 = getelementptr { i32, i32}* undef
   ret void
 }
@@ -90,9 +95,6 @@ define void @conversion() {
   %a7 = fptosi double undef to i64
   %a8 = uitofp i64 undef to double
   %a9 = sitofp i64 undef to double
-  %a10 = ptrtoint i8* undef to i32
-  %a11 = inttoptr i32 undef to i8*
-  %a12 = bitcast i8* undef to i32*
   ret void
 }
 
@@ -117,40 +119,41 @@ declare void @personality_func()
 define void @invoke_func() {
   invoke void @external_func() to label %ok unwind label %onerror
 ; CHECK-NOT: disallowed
-; CHECK: Function invoke_func has disallowed instruction: invoke
+; CHECK: disallowed: bad instruction opcode: invoke
 ok:
   ret void
 onerror:
   %lp = landingpad i32
       personality i8* bitcast (void ()* @personality_func to i8*)
       catch i32* null
-; CHECK-NEXT: Function invoke_func has disallowed instruction: landingpad
-; CHECK-NEXT: Function invoke_func contains disallowed ConstantExpr
+; CHECK: disallowed: bad instruction opcode: {{.*}} landingpad
   resume i32 %lp
-; CHECK-NEXT: Function invoke_func has disallowed instruction: resume
+; CHECK: disallowed: bad instruction opcode: resume
 }
 
-define i32 @va_arg(i8* %va_list) {
+define i32 @va_arg(i32 %va_list_as_int) {
+  %va_list = inttoptr i32 %va_list_as_int to i8*
   %val = va_arg i8* %va_list, i32
   ret i32 %val
 }
 ; CHECK-NOT: disallowed
-; CHECK: Function va_arg has disallowed instruction: va_arg
+; CHECK: disallowed: bad instruction opcode: {{.*}} va_arg
 
 @global_var = global [4 x i8] zeroinitializer
 
-define i8* @constantexpr() {
-  ret i8* getelementptr ([4 x i8]* @global_var, i32 1, i32 0)
+define void @constantexpr() {
+  ptrtoint i8* getelementptr ([4 x i8]* @global_var, i32 1, i32 0) to i32
+  ret void
 }
 ; CHECK-NOT: disallowed
-; CHECK: Function constantexpr contains disallowed ConstantExpr
+; CHECK: disallowed: operand not InherentPtr: %1 = ptrtoint i8* getelementptr
 
 define void @inline_asm() {
   call void asm "foo", ""()
   ret void
 }
 ; CHECK-NOT: disallowed
-; CHECK: Function inline_asm contains disallowed inline assembly
+; CHECK: disallowed: inline assembly: call void asm "foo", ""()
 
 ; CHECK-NOT: disallowed
 ; If another check is added, there should be a check-not in between each check
diff --git a/test/NaCl/PNaClABI/types-function.ll b/test/NaCl/PNaClABI/types-function.ll
index 32004d39ad..1482c727f0 100644
--- a/test/NaCl/PNaClABI/types-function.ll
+++ b/test/NaCl/PNaClABI/types-function.ll
@@ -4,22 +4,27 @@
 ; stashed in various places in function bodies are caught.
 
 @a2 = private global i17 zeroinitializer
+
+; CHECK: Function func has disallowed type: void (i15)
 declare void @func(i15 %arg)
 
 !llvm.foo = !{!0}
 !0 = metadata !{ half 0.0}
 
 define void @types() {
-; CHECK: Function types has instruction with disallowed type: half
+; CHECK: bad result type: {{.*}} fptrunc
   %h1 = fptrunc double undef to half
-; CHECK: Function types has instruction operand with disallowed type: half
+
+; CHECK: bad operand: {{.*}} bitcast half
   %h2 = bitcast half 0.0 to i16
+
 ; see below...
   %h3 = fadd double 0.0, fpext (half 0.0 to double)
 
-; CHECK: Function types has instruction operand with disallowed type: i17*
+; CHECK: bad pointer: store
   store i32 0, i32* bitcast (i17* @a2 to i32*), align 4
-; CHECK: Function types has instruction operand with disallowed type: i15
+
+; CHECK: bad function callee operand: call void @func(i15 1)
   call void @func(i15 1)
 
 ; CHECK: Function types has disallowed instruction metadata: !foo
diff --git a/test/NaCl/PNaClABI/types.ll b/test/NaCl/PNaClABI/types.ll
index 7587609d11..b1527ec111 100644
--- a/test/NaCl/PNaClABI/types.ll
+++ b/test/NaCl/PNaClABI/types.ll
@@ -34,66 +34,59 @@ block:
   ; Disallowed integer types
 
   phi i4 [ undef, %entry ]
-; CHECK: Function func has instruction with disallowed type: i4
+; CHECK: Function func disallowed: bad operand: {{.*}} i4
 
   phi i33 [ undef, %entry ]
-; CHECK: instruction with disallowed type: i33
+; CHECK-NEXT: disallowed: bad operand: {{.*}} i33
 
   phi i128 [ undef, %entry ]
-; CHECK: instruction with disallowed type: i128
+; CHECK-NEXT: disallowed: bad operand: {{.*}} i128
 
 
   ; Disallowed floating point types
 
   phi half [ undef, %entry ]
-; CHECK: instruction with disallowed type: half
+; CHECK-NEXT: disallowed: bad operand: {{.*}} half
 
   phi x86_fp80 [ undef, %entry ]
-; CHECK: instruction with disallowed type: x86_fp80
+; CHECK-NEXT: disallowed: bad operand: {{.*}} x86_fp80
 
   phi fp128 [ undef, %entry ]
-; CHECK: instruction with disallowed type: fp128
+; CHECK-NEXT: disallowed: bad operand: {{.*}} fp128
 
   phi ppc_fp128 [ undef, %entry ]
-; CHECK: instruction with disallowed type: ppc_fp128
+; CHECK-NEXT: disallowed: bad operand: {{.*}} ppc_fp128
 
   phi x86_mmx [ undef, %entry ]
-; CHECK: instruction with disallowed type: x86_mmx
-; CHECK: instruction operand with disallowed type: x86_mmx
+; CHECK-NEXT: disallowed: bad operand: {{.*}} x86_mmx
 
 
-  ; Derived types
+  ; Derived types are disallowed too
 
-  ; TODO(mseaborn): These are currently allowed but should be disallowed.
   phi i32* [ undef, %entry ]
+; CHECK-NEXT: disallowed: bad operand: {{.*}} i32*
+
   phi [1 x i32] [ undef, %entry ]
+; CHECK-NEXT: disallowed: bad operand: {{.*}} [1 x i32]
+
   phi { i32, float } [ undef, %entry ]
+; CHECK-NEXT: disallowed: bad operand: {{.*}} { i32, float }
+
   phi void (i32)* [ undef, %entry ]
-  phi <{ i8, i32 }> [ undef, %entry ]
-  phi { i32, { i32, double }, float } [ undef, %entry ]
-; CHECK-NOT: disallowed
+; CHECK-NEXT: disallowed: bad operand: {{.*}} void (i32)*
 
-  ; Derived types containing disallowed types
-  phi half* [ undef, %entry ]
-; CHECK: instruction with disallowed type: half*
-  phi [2 x i33] [ undef, %entry ]
-; CHECK: instruction with disallowed type: [2 x i33]
-  phi { half, i32 } [ undef, %entry ]
-; CHECK: instruction with disallowed type: { half, i32 }
-  phi { float, i33 } [ undef, %entry ]
-; CHECK: instruction with disallowed type: { float, i33 }
-  phi { i32, { i32, half }, float } [ undef, %entry ]
-; CHECK: instruction with disallowed type: { i32, { i32, half }, float }
+  phi <{ i8, i32 }> [ undef, %entry ]
+; CHECK-NEXT: disallowed: bad operand: {{.*}} <{ i8, i32 }>
 
   ; Vector types are disallowed
   phi <2 x i32> [ undef, %entry ]
-; CHECK: instruction with disallowed type: <2 x i32>
+; CHECK-NEXT: disallowed: bad operand: {{.*}} <2 x i32>
 
   ret void
 }
 
 
-; named types. with the current implementation, bogus named types are legal
+; Named types. With the current implementation, named types are legal
 ; until they are actually attempted to be used. Might want to fix that.
 %struct.s1 = type { half, float}
 %struct.s2 = type { i32, i32}
@@ -104,11 +97,10 @@ entry:
 block:
 
   phi %struct.s1 [ undef, %entry ]
-; CHECK: instruction with disallowed type: %struct.s1 = type { half, float }
-; CHECK: instruction operand with disallowed type: %struct.s1 = type { half, float }
+; CHECK: disallowed: bad operand: {{.*}} %struct.s1
 
   phi %struct.s2 [ undef, %entry ]
-; CHECK-NOT: disallowed
+; CHECK-NEXT: disallowed: bad operand: {{.*}} %struct.s2
 
   ret void
 }
@@ -129,8 +121,10 @@ entry:
 block:
 
   phi %struct.snake [ undef, %entry ]
+; CHECK: disallowed: bad operand: {{.*}} %struct.snake
+
   phi %struct.linked [ undef, %entry ]
-; CHECK-NOT: disallowed
+; CHECK-NEXT: disallowed: bad operand: {{.*}} %struct.linked
 
   ret void
 }
diff --git a/test/Transforms/NaCl/replace-ptrs-with-ints.ll b/test/Transforms/NaCl/replace-ptrs-with-ints.ll
index 5a8f58dbaf..823697d738 100644
--- a/test/Transforms/NaCl/replace-ptrs-with-ints.ll
+++ b/test/Transforms/NaCl/replace-ptrs-with-ints.ll
@@ -202,6 +202,14 @@ define i16* @constant_pointer_null_load() {
 ; CHECK-NEXT: %.asptr = inttoptr i32 0 to i32*
 ; CHECK-NEXT: %val = load i32* %.asptr
 
+define i16* @constant_pointer_undef_load() {
+  %val = load i16** undef
+  ret i16* %val
+}
+; CHECK: define i32 @constant_pointer_undef_load() {
+; CHECK-NEXT: %.asptr = inttoptr i32 undef to i32*
+; CHECK-NEXT: %val = load i32* %.asptr
+
 
 define i8 @load(i8* %ptr) {
   %x = load i8* %ptr