This CL supersedes the previous 2 outstanding CLs.

1) Remove the ABIVerifyErrors class since it was just a wrapper with no real functionality. Replace with a simple string ostream for now. If in the future we want to have more intelligent error handling, we can come up with something that does what we actually decide we want.
2) When printing type errors in initializers, just print the Type that was invalid, rather than the whole initializer. This is implemented by having checkTypesInValue return the Type that was invalid, rather than just a boolean.
3) Check the type of each instruction (this is just to make sure it works from the function pass). Do not, however, make the passes dependent as in the previous CL (function passes cannot depend on module passes, but there are no checks for this, and opt does not enforce the dependence, and when llc tries to enforce it, very bad things happen)
2) Style cleanup to match LLVM style.

Sorry for the multiple changes in one CL, hopefully it's still small enough to be reviewable.
BUG=

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

3 files changed, 79 insertions, 57 deletions

diff --git a/lib/Analysis/NaCl/CheckTypes.h b/lib/Analysis/NaCl/CheckTypes.h
index 2f2af53eff..64a634cd25 100644
--- a/lib/Analysis/NaCl/CheckTypes.h
+++ b/lib/Analysis/NaCl/CheckTypes.h
@@ -21,7 +21,7 @@
 
 class TypeChecker {
  public:
-  bool IsValidType(const llvm::Type* Ty) {
+  bool isValidType(const llvm::Type *Ty) {
     if (VisitedTypes.count(Ty))
       return VisitedTypes[Ty];
 
@@ -64,7 +64,7 @@ class TypeChecker {
         VisitedTypes[Ty] = true;
         for (llvm::Type::subtype_iterator I = Ty->subtype_begin(),
                  E = Ty->subtype_end(); I != E; ++I)
-          Valid &= IsValidType(*I);
+          Valid &= isValidType(*I);
         break;
         // Handle NumTypeIDs, and no default case,
         // so we get a warning if new types are added
@@ -77,7 +77,9 @@ class TypeChecker {
     return Valid;
   }
 
-  bool CheckTypesInValue(const llvm::Value* V) {
+  // If the value contains an invalid type, return a pointer to the type.
+  // Return null if there are no invalid types.
+  llvm::Type *checkTypesInValue(const llvm::Value *V) {
     // TODO: Checking types in values probably belongs in its
     // own value checker which also handles the various types of
     // constexpr (in particular, blockaddr constexprs cause this code
@@ -87,22 +89,41 @@ class TypeChecker {
     if (VisitedConstants.count(V))
       return VisitedConstants[V];
 
-    bool Valid = IsValidType(V->getType());
-    VisitedConstants[V] = Valid;
+    if (!isValidType(V->getType())) {
+      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;
     const llvm::User *U = llvm::cast<llvm::User>(V);
     for (llvm::Constant::const_op_iterator I = U->op_begin(),
-             E = U->op_end(); I != E; ++I)
-      Valid &= CheckTypesInValue(*I);
-    VisitedConstants[V] = Valid;
-    return Valid;
+             E = U->op_end(); I != E; ++I) {
+      llvm::Type *Invalid = checkTypesInValue(*I);
+      if (Invalid) {
+        VisitedConstants[V] = Invalid;
+        return Invalid;
+      }
+    }
+    VisitedConstants[V] = NULL;
+    return NULL;
+  }
+
+  // There's no built-in way to get the name of a type, so use a
+  // string ostream to print it.
+  static std::string getTypeName(const llvm::Type *T) {
+    std::string TypeName;
+    llvm::raw_string_ostream N(TypeName);
+    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.
-  llvm::DenseMap<const llvm::Value*, bool> VisitedConstants;
+  llvm::DenseMap<const llvm::Value*, llvm::Type*> VisitedConstants;
   llvm::DenseMap<const llvm::Type*, bool> VisitedTypes;
 };
 
diff --git a/lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp b/lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp
index a46e95b0b3..df57cfbbce 100644
--- a/lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp
+++ b/lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp
@@ -18,30 +18,32 @@
 #include "llvm/IR/Instruction.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Analysis/NaCl.h"
+
+#include "CheckTypes.h"
 using namespace llvm;
 
 namespace {
 
 // Checks that examine anything in the function body should be in
 // FunctionPasses to make them streaming-friendly
-struct PNaClABIVerifyFunctions : public FunctionPass {
+class PNaClABIVerifyFunctions : public FunctionPass {
+ public:
   static char ID;
-  PNaClABIVerifyFunctions() : FunctionPass(ID) {}
+  PNaClABIVerifyFunctions() : FunctionPass(ID), Errors(ErrorsString) {}
   bool runOnFunction(Function &F);
-  // For now, this print method exists to allow us to run the pass with
-  // opt -analyze to avoid dumping the result to stdout, to make testing
-  // simpler. In the future we will probably want to make it do something
-  // useful.
   virtual void print(llvm::raw_ostream &O, const Module *M) const;
  private:
-  ABIVerifyErrors Errors;
+  TypeChecker TC;
+  std::string ErrorsString;
+  raw_string_ostream Errors;
 };
+
 } // and anonymous namespace
 
 bool PNaClABIVerifyFunctions::runOnFunction(Function &F) {
   // For now just start with new errors on each function; this may change
   // once we want to do something with them other than just calling print()
-  Errors.clear();
+  ErrorsString.clear();
   for (Function::const_iterator FI = F.begin(), FE = F.end();
            FI != FE; ++FI) {
     for (BasicBlock::const_iterator BBI = FI->begin(), BBE = FI->end();
@@ -55,9 +57,9 @@ bool PNaClABIVerifyFunctions::runOnFunction(Function &F) {
         case Instruction::ExtractElement:
         case Instruction::InsertElement:
         case Instruction::ShuffleVector:
-          Errors.addError(Twine("Function ") + F.getName() +
-                          " has disallowed instruction: " +
-                          BBI->getOpcodeName() + "\n");
+          Errors << "Function " + F.getName() +
+              " has disallowed instruction: " +
+              BBI->getOpcodeName() + "\n";
           break;
 
         // Terminator instructions
@@ -120,16 +122,22 @@ bool PNaClABIVerifyFunctions::runOnFunction(Function &F) {
         case Instruction::LandingPad:
           break;
       }
+      // Check the types. First check the type of the instruction.
+      if (!TC.isValidType(BBI->getType())) {
+        Errors << "Function " + F.getName() +
+            " has instruction with disallowed type: " +
+            TypeChecker::getTypeName(BBI->getType()) + "\n";
+      }
     }
   }
+
+  Errors.flush();
   return false;
 }
 
 void PNaClABIVerifyFunctions::print(llvm::raw_ostream &O, const Module *M)
     const {
-  for (ABIVerifyErrors::const_iterator I = Errors.begin(), E = Errors.end();
-      I != E; ++I)
-    O << *I;
+  O << ErrorsString;
 }
 
 char PNaClABIVerifyFunctions::ID = 0;
diff --git a/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp b/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
index ab1ea663e5..afe42434e7 100644
--- a/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
+++ b/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
@@ -24,25 +24,20 @@
 using namespace llvm;
 
 namespace {
-
 // This pass should not touch function bodies, to stay streaming-friendly
-struct PNaClABIVerifyModule : public ModulePass {
+class PNaClABIVerifyModule : public ModulePass {
+ public:
   static char ID;
-  PNaClABIVerifyModule() : ModulePass(ID) {}
+  PNaClABIVerifyModule();
   bool runOnModule(Module &M);
-  // For now, this print method exists to allow us to run the pass with
-  // opt -analyze to avoid dumping the result to stdout, to make testing
-  // simpler. In the future we will probably want to make it do something
-  // useful.
   virtual void print(llvm::raw_ostream &O, const Module *M) const;
  private:
-  // Ideally this would share an instance with the Function pass.
-  // TODO: see if that's feasible when we add checking in bodies
   TypeChecker TC;
-  ABIVerifyErrors Errors;
+  std::string ErrorsString;
+  llvm::raw_string_ostream Errors;
 };
 
-static const char* LinkageName(GlobalValue::LinkageTypes LT) {
+static const char *linkageName(GlobalValue::LinkageTypes LT) {
   // This logic is taken from PrintLinkage in lib/VMCore/AsmWriter.cpp
   switch (LT) {
     case GlobalValue::ExternalLinkage: return "external";
@@ -70,25 +65,25 @@ static const char* LinkageName(GlobalValue::LinkageTypes LT) {
 
 } // end anonymous namespace
 
-bool PNaClABIVerifyModule::runOnModule(Module &M) {
+PNaClABIVerifyModule::PNaClABIVerifyModule() : ModulePass(ID),
+                                               Errors(ErrorsString) {}
 
+bool PNaClABIVerifyModule::runOnModule(Module &M) {
   for (Module::const_global_iterator MI = M.global_begin(), ME = M.global_end();
        MI != ME; ++MI) {
     // Check types of global variables and their initializers
-    if (!TC.IsValidType(MI->getType())) {
-      std::string TypeName;
-      raw_string_ostream N(TypeName);
+    if (!TC.isValidType(MI->getType())) {
       // GVs are pointers, so print the pointed-to type for clarity
-      MI->getType()->getContainedType(0)->print(N);
-      Errors.addError(Twine("Variable ") + MI->getName() +
-                      " has disallowed type: " + N.str() + "\n");
-    } else if (MI->hasInitializer() &&
-               !TC.CheckTypesInValue(MI->getInitializer())) {
-      std::string TypeName;
-      raw_string_ostream N(TypeName);
-      MI->getInitializer()->print(N);
-      Errors.addError(Twine("Initializer for ") + MI->getName() +
-                      " has disallowed type: " + N.str() + "\n");
+      Errors << "Variable " + MI->getName() +
+          " has disallowed type: " +
+          TypeChecker::getTypeName(MI->getType()->getContainedType(0)) + "\n";
+    } else if (MI->hasInitializer()) {
+      // If the type of the global is bad, no point in checking its initializer
+      Type *T = TC.checkTypesInValue(MI->getInitializer());
+      if (T)
+        Errors << "Initializer for " + MI->getName() +
+            " has disallowed type: " +
+            TypeChecker::getTypeName(T) + "\n";
     }
 
     // Check GV linkage types
@@ -99,24 +94,22 @@ bool PNaClABIVerifyModule::runOnModule(Module &M) {
       case GlobalValue::PrivateLinkage:
         break;
       default:
-        Errors.addError(Twine("Variable ") + MI->getName() +
-                        " has disallowed linkage type: " +
-                        LinkageName(MI->getLinkage()) + "\n");
+        Errors << "Variable " + MI->getName() +
+            " has disallowed linkage type: " +
+            linkageName(MI->getLinkage()) + "\n";
     }
   }
   // No aliases allowed for now.
   for (Module::alias_iterator MI = M.alias_begin(),
            E = M.alias_end(); MI != E; ++MI)
-    Errors.addError(Twine("Variable ") + MI->getName() +
-                    " is an alias (disallowed)\n");
+    Errors << "Variable " + MI->getName() + " is an alias (disallowed)\n";
 
+  Errors.flush();
   return false;
 }
 
 void PNaClABIVerifyModule::print(llvm::raw_ostream &O, const Module *M) const {
-  for (ABIVerifyErrors::const_iterator I = Errors.begin(), E = Errors.end();
-       I != E; ++I)
-    O << *I;
+  O << ErrorsString;
 }
 
 char PNaClABIVerifyModule::ID = 0;