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 int