[PATCH] An analysis to find external function pointers and trace their data flow

[Nick Lewycky]

Just a few comments from looking at it once, this isn't a complete review yet.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:163-167
@@ +162,7 @@
+
+// Compares a function name against a list of known memory allocation
+// functions, as provided by TargetLibraryInfo. This is used to skip
+// memory-allocation functions in the analysis, since they are a frequent
+// source of false positives.
+bool isMemoryAllocation(const TargetLibraryInfo &TLI, StringRef Name) {
+  return
----------------
llvm::isAllocationFn? It's in include/llvm/Analysis/MemoryBuiltins.h and there are other similar functions in case that one isn't quite right.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:223-224
@@ +222,4 @@
+        // subexpressions of other instructions.
+        if (const Instruction *I = dyn_cast<Instruction>(Us)) {
+          if (isa<BasicBlock>(I->getParent())) {
+            followValue(cast<Value>(Us), Vals, SeenValues);
----------------
If I is not null then I->getParent() is guaranteed* to be a BasicBlock.

* The only way it isn't guaranteed is if your pass is creating instructions and not inserting them into BasicBlocks (or taking instructions out of BBs and not deleting them). At the start and finish of every pass, this property holds.

I think what you're referring to as instructions that appear as subexpressions are actually ConstantExpr's.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:282
@@ +281,3 @@
+    Value *OpVal = Us->getOperand(1);
+    if (!OpVal)
+      continue;
----------------
I claim this is always false. The only way an operand can be null is if the object whose operands we're looking at is metadata, and metadata never shows up as a user of another Value.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:447-448
@@ +446,4 @@
+    // We only follow calls to external pointers.
+    const GlobalValue *GV = cast<GlobalValue>(F);
+    if (!GV->isDeclaration())
+      continue;
----------------
Why not
  if (!F->isDeclaration())
without the explicit cast?

http://reviews.llvm.org/D2873