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

7 files changed, 95 insertions, 80 deletions

diff --git a/include/llvm/Analysis/NaCl.h b/include/llvm/Analysis/NaCl.h
index f1591075d0..8e599594b2 100644
--- a/include/llvm/Analysis/NaCl.h
+++ b/include/llvm/Analysis/NaCl.h
@@ -10,10 +10,6 @@
 #ifndef LLVM_ANALYSIS_NACL_H
 #define LLVM_ANALYSIS_NACL_H
 
-#include "llvm/ADT/Twine.h"
-#include <string>
-#include <vector>
-
 namespace llvm {
 
 class FunctionPass;
@@ -24,21 +20,5 @@ ModulePass *createPNaClABIVerifyModulePass();
 
 }
 
-// A simple class to store verification errors. This allows them to be saved
-// and printed by the analysis passes' print() methods, while still allowing
-// the messages to be easily constructed with Twine.
-class ABIVerifyErrors {
- public:
-  void addError(const llvm::Twine &Error) {
-    Messages.push_back(Error.str());
-  }
-  typedef std::vector<std::string>::const_iterator const_iterator;
-  const_iterator begin() const { return Messages.begin(); }
-  const_iterator end() const   { return Messages.end(); }
-  bool empty() const           { return Messages.empty(); }
-  void clear()                 { Messages.clear(); }
- private:
-  std::vector<std::string> Messages;
-};
 
 #endif
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;
diff --git a/test/NaCl/PNaClABI/instructions.ll b/test/NaCl/PNaClABI/instructions.ll
index d07fa78c5b..c7615529c9 100644
--- a/test/NaCl/PNaClABI/instructions.ll
+++ b/test/NaCl/PNaClABI/instructions.ll
@@ -47,10 +47,11 @@ define void @vectors() {
 ; CHECK-NOT: disallowed
   %a1 = extractelement <2 x i32> <i32 0, i32 0>, i32 0
 ; CHECK: Function vectors has disallowed instruction: extractelement
-  %a2 = shufflevector <2 x i32> undef , <2 x i32> undef, <2 x i32> undef
+  %a2 = shufflevector <2 x i32> undef, <2 x i32> undef, <2 x i32> undef
 ; CHECK: Function vectors has disallowed instruction: shufflevector
   %a3 = insertelement <2 x i32> undef, i32 1, i32 0
 ; CHECK: Function vectors has disallowed instruction: insertelement
+; CHECK: Function vectors has instruction with disallowed type
   ret void
 }
 
diff --git a/test/NaCl/PNaClABI/types-function.ll b/test/NaCl/PNaClABI/types-function.ll
new file mode 100644
index 0000000000..0302d3715d
--- /dev/null
+++ b/test/NaCl/PNaClABI/types-function.ll
@@ -0,0 +1,10 @@
+; RUN: opt -verify-pnaclabi-functions -analyze < %s | FileCheck %s
+; Test type-checking in function bodies. This test is not intended to verify
+; all the rules about the various types, but instead to make sure that types
+; stashed in various places in function bodies are caught.
+
+define void @types() {
+; CHECK: Function types has instruction with disallowed type: half
+  %h1 = fptrunc double undef to half
+  ret void
+}
\ No newline at end of file
diff --git a/test/NaCl/PNaClABI/types.ll b/test/NaCl/PNaClABI/types.ll
index 48630ad9b1..0c68036a73 100644
--- a/test/NaCl/PNaClABI/types.ll
+++ b/test/NaCl/PNaClABI/types.ll
@@ -67,10 +67,10 @@
 ; Initializers with constexprs
 ; CHECK: Variable cc1 has disallowed type: half
 @cc1 = private global half 0.0
-; CHECK: Initializer for ce1 has disallowed type: i8* bitcast (half* @cc1 to i8*)
+; CHECK: Initializer for ce1 has disallowed type: half*
 @ce1 = private global i8 * bitcast (half* @cc1 to i8*)
 @cc2 = private global { i32, half } undef
-; CHECK: Initializer for ce2 has disallowed type: i32* getelementptr inbounds ({ i32, half }* @cc2, i32 0, i32 0)
+; CHECK: Initializer for ce2 has disallowed type: { i32, half }*
 @ce2 = private global i32 * getelementptr ({ i32, half } * @cc2, i32 0, i32 0)
 
 ; Circularities:  here to make sure the verifier doesn't crash or assert.
@@ -83,6 +83,8 @@
 
 %struct.linked = type { i32, %struct.linked * }
 @list1 = private global %struct.linked { i32 0, %struct.linked* null }
+
+@list2 = private global i32* bitcast (i32** @list2 to i32*)
 ; CHECK-NOT: disallowed
 
 ; CHECK: Variable alias1 is an alias (disallowed)