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..8dff805c347fa84e5259a08a19e2e07425721146 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,13 @@ cl::opt<bool>
 PNaClABIAllowDebugMetadata("pnaclabi-allow-debug-metadata",
   cl::desc("Allow debug metadata during PNaCl ABI verification."),
   cl::init(false));
+
 }
 
+static cl::opt<bool> PNaClABIAllowIntrinsics("pnaclabi-allow-intrinsics",

[eliben, 2013/05/07 15:27:40]

Can we divide intrinsics into 3 groups:

1. Those that we allow (and will allow in the stable ABI)
2. Those we don't allow even now (i.e. won't fail any existing tests)
3. Those that we plan to disallow bu have to adjust tests first

Which of these is the flag meant to override? 2, or or both? In any case the
current flag name is not very descriptive as it's not clear that some intrinsics
are allowed even if it's not set to 1.

[jvoung (off chromium), 2013/05/07 17:39:46]

That sounds like a good breakdown.


I'll rename the flag to "PNaClABIAllowDevIntrinsics"?  Other suggestions?  The
flag is intended for (3), but patch set 1 didn't enumerate stuff for (2).  Just
added a couple of things for (2).


For (3) we could also consider some intrinsics for debugging, or it might be
intrinsics that we can support, but need more thought or testing (e.g.,
__builtin_popcount) which is why I considered calling the flag "Dev".


For (2) I partly ended up having to glob based on function name, instead of
using the switch-case facilities.  There are probably over a thousand intrinsics
in llvm (llvm.arm.*, llvm.x86.*, llvm.ppc.*, llvm.nvvm.* ).  The implementation
of the table-gen'ed llvm::Function::getIntrinsicID() takes up 170KB of .text for
the x86-64 build of the LLC.nexe.

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

At the Tues meeting (an hour ago), there was some desire to not have extra flags
for the verifier.

This can be done two ways:

(a) disable tests

(b) just use the existing "--pnacl-disable-abi-check"

(c) gain enough confidence to start accepting stuff in list (3) -- possibly we
could duplicate the test coverage via some llvm lit tests that show, for each
architecture, what architectural support we have, or what compiler_rt support we
have.


=====

How about I keep this flag here for now, and remove it once there is enough of a
combination of (a), (b), and (c).

This flag won't actually propagate out into scons, gcc, or llvm test suites,
only (a) or (b) will be used to deal with them.

I'd like to start testing and promoting some of the stuff in (3) so that we
won't need to do too much of (a) and (b).  We could also get rid of this flag
now, and just "return true;" for stuff in category (3) for now.

+  cl::desc("Allow all LLVM intrinsics duing PNaCl ABI verification."),
+  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 +62,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 +121,38 @@ void PNaClABIVerifyModule::CheckGlobalValueCommon(const GlobalValue *GV) {
   }
 }
 
+bool PNaClABIVerifyModule::IsWhitelistedIntrinsic(const Function* F,
+                                                  unsigned ID) {
+  switch(ID) {
+    default:
+      DEBUG(dbgs() << "Unknown intrinsic: " << F->getName() << "\n");
+      return PNaClABIAllowIntrinsics;
+    case Intrinsic::not_intrinsic: return false;
+    case Intrinsic::dbg_declare:
+    case Intrinsic::dbg_value:
+    case Intrinsic::frameaddress:   // Likely only used for debugging?
+    case Intrinsic::returnaddress:  // Likely only used for debugging?
+      return PNaClABIAllowDebugMetadata;
+    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;
+    case Intrinsic::vacopy:
+    case Intrinsic::vaend:
+    case Intrinsic::vastart:
+      // Var-args code is expanded out, so we shouldn't need va_arg intrinsics.
+      // TODO(jvoung): Disallow this too.  However, we need to strip
+      // dead prototypes before this will work.
+      return PNaClABIAllowDebugMetadata;
+  }
+}
+
 bool PNaClABIVerifyModule::IsWhitelistedMetadata(const NamedMDNode* MD) {
   return MD->getName().startswith("llvm.dbg.") && PNaClABIAllowDebugMetadata;
 }
@@ -153,6 +193,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 15:27:40]

Can an intrinsic be called without a declaration? What if we don't run the IR
verifier in the translator?

[jvoung (off chromium), 2013/05/07 17:39:46]

I don't think you can call intrinsics without a declaration:

"error: use of undefined value '@llvm.readcyclecounter'"
Can you elaborate?  Does the usage of intrinsics differ between code that the
translator sees and code that "opt" or "pnacl-abicheck" sees?

[jvoung (off chromium), 2013/05/07 22:32:18]

Ah yes, you mean we'd do something like "llc -disable-verify".  The error I
showed above is actually from the LLParser.  We don't support textual llvm IR,
so we won't see that particular error.  

I don't think you can generate a binary representation of the bitcode that calls
a function that hasn't been declared.

According to "llvm-bcanalyzer -dump ..." you get:

<FUNCTION_BLOCK ...>
  <DECLAREBLOCKS ...>
  <INST_CALL op0=... op2=[[FUNCTION ID]] />
  ...
</FUNCTION_BLOCK>

where [[FUNCTION ID]] should be something defined earlier.

[jvoung (off chromium), 2013/05/07 22:33:08]

Err... not that you can't generate bad binary bitcode, but maybe the bitcode
reader would blow up if you did.  I'm not sure =)

+      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())) {