Start a whitelist of intrinsics for the PNaCl ABI checker.

lib/Analysis/NaCl/PNaClABIVerifyModule.cpp

Index: lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
diff --git a/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp b/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
index efb40cf43a7d90747ae4ae081e47a468e8318c9b..80a706a00192236314e4fd4162e42dc57fdd7477 100644
--- a/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
+++ b/lib/Analysis/NaCl/PNaClABIVerifyModule.cpp
@@ -17,7 +17,9 @@
 #include "llvm/ADT/Twine.h"
 #include "llvm/Analysis/NaCl.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Module.h"
+#include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include "PNaClABITypeChecker.h"
@@ -28,8 +30,14 @@ cl::opt<bool>
 PNaClABIAllowDebugMetadata("pnaclabi-allow-debug-metadata",
   cl::desc("Allow debug metadata during PNaCl ABI verification."),
   cl::init(false));
+
 }
 
+static cl::opt<bool>
+PNaClABIAllowDevIntrinsics("pnaclabi-allow-dev-intrinsics",
+  cl::desc("Allow all LLVM intrinsics duing PNaCl ABI verification."),

[eliben, 2013/05/07 17:49:36]

duing

[jvoung (off chromium), 2013/05/07 21:24:41]

Done.

+  cl::init(true));  // TODO(jvoung): Make this false by default.
+
 namespace {
 // This pass should not touch function bodies, to stay streaming-friendly
 class PNaClABIVerifyModule : public ModulePass {
@@ -55,6 +63,7 @@ class PNaClABIVerifyModule : public ModulePass {
   virtual void print(raw_ostream &O, const Module *M) const;
  private:
   void CheckGlobalValueCommon(const GlobalValue *GV);
+  bool IsWhitelistedIntrinsic(const Function* F, unsigned ID);
   bool IsWhitelistedMetadata(const NamedMDNode *MD);
   PNaClABITypeChecker TC;
   PNaClABIErrorReporter *Reporter;
@@ -113,6 +122,67 @@ void PNaClABIVerifyModule::CheckGlobalValueCommon(const GlobalValue *GV) {
   }
 }
 
+bool PNaClABIVerifyModule::IsWhitelistedIntrinsic(const Function* F,
+                                                  unsigned ID) {
+  // Keep 3 categories of intrinsics for now.
+  // (1) Allowed always
+  // (2) Never allowed
+  // (3) "Dev" intrinsics, which may or may not be allowed.
+  // "Dev" intrinsics are controlled by the PNaClABIAllowDevIntrinsics flag.
+  // Please keep these sorted within each category.
+  switch(ID) {
+    default: {
+      StringRef name = F->getName();
+      // Disallow target-specific intrinsics
+      // (see list of target TD includes in Intrinsics.td).
+      if (name.startswith("llvm.arm.")
+          || name.startswith("llvm.hexagon.")
+          || name.startswith("llvm.mips.")
+          || name.startswith("llvm.nvvm.")
+          || name.startswith("llvm.ppc.")
+          || name.startswith("llvm.r600.")
+          || name.startswith("llvm.x86.")
+          || name.startswith("llvm.xcore.")) {
+        return false;
+      } else {
+        DEBUG(dbgs() << "Unknown intrinsic: " << F->getName() << "\n");

[eliben, 2013/05/07 17:49:36]

Again, I'm not sure why we can't:

Detect (1) and (3) in the switch, leaving the rest to 'default'. Are there very
many intrinsics in (3) ?

[jvoung (off chromium), 2013/05/07 21:24:41]

Added the list of stuff triggered by our scons + gcc + llvm nightly + spec
tests.

+        // TODO(jvoung): Dev intrinsics should be listed out, instead
+        // of being caught by the default case.
+        return PNaClABIAllowDevIntrinsics;
+      }
+    }
+    // (1) Always allowed.
+    case Intrinsic::invariant_end:
+    case Intrinsic::invariant_start:
+    case Intrinsic::lifetime_end:
+    case Intrinsic::lifetime_start:
+    case Intrinsic::memcpy:
+    case Intrinsic::memmove:
+    case Intrinsic::memset:
+    case Intrinsic::nacl_read_tp:
+    case Intrinsic::trap:
+      return true;
+    // (2) Never allowed.
+    case Intrinsic::not_intrinsic:
+    // Trampoline intrinsics depend on target-specific-sized buffer.
+    // Perhaps if the allocator was target-independent, then this could
+    // be accepted.
+    case Intrinsic::adjust_trampoline:
+    case Intrinsic::init_trampoline:
+    // Var-args code is expanded out, so we shouldn't need va_arg intrinsics.
+    case Intrinsic::vacopy:
+    case Intrinsic::vaend:
+    case Intrinsic::vastart:
+      return false;
+    // (3) Dev intrinsics.
+    case Intrinsic::dbg_declare:
+    case Intrinsic::dbg_value:
+    case Intrinsic::frameaddress: // Support for 0-level or not?
+    case Intrinsic::returnaddress: // Support for 0-level or not?
+      return PNaClABIAllowDevIntrinsics || PNaClABIAllowDebugMetadata;
+  }
+}
+
 bool PNaClABIVerifyModule::IsWhitelistedMetadata(const NamedMDNode* MD) {
   return MD->getName().startswith("llvm.dbg.") && PNaClABIAllowDebugMetadata;
 }
@@ -153,6 +223,14 @@ bool PNaClABIVerifyModule::runOnModule(Module &M) {
   }
 
   for (Module::const_iterator MI = M.begin(), ME = M.end(); MI != ME; ++MI) {
+    // Check intrinsics.
+    if (MI->isIntrinsic()) {

[eliben, 2013/05/07 17:49:36]

Any reason not to combine into a single condition?

[jvoung (off chromium), 2013/05/07 21:24:41]

Done.

+      if (!IsWhitelistedIntrinsic(MI, MI->getIntrinsicID())) {
+        Reporter->addError() << "Function " << MI->getName()
+                             << " is a disallowed LLVM intrinsic\n";
+      }
+    }
+
     // Check types of functions and their arguments
     FunctionType *FT = MI->getFunctionType();
     if (!TC.isValidType(FT->getReturnType())) {