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

[JF Bastien]

  First pass comments.

  Overall LLVM has had a lot of churn w.r.t. C++11, so you may want to rebase and use the new C++11 features since it'll make some of the code easier to read (and it won't need updating).


================
Comment at: include/llvm/Analysis/ExternalFunctionAnalysis.h:48
@@ +47,3 @@
+
+  ~ExternalFunctionAnalysis() {}
+
----------------
virtual

================
Comment at: include/llvm/Analysis/ExternalFunctionAnalysis.h:75
@@ +74,3 @@
+  /// were annotated with __attribute__((annotate("efa-maybe-external"))).
+  void getMaybeExternalPtrInstrs(Module &M);
+
----------------
It seems weird to call this and the next two functions "get*" when they don't return anything. Compute instead?

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:42
@@ +41,3 @@
+
+const char *efa_annotation = "efa-maybe-external";
+
----------------
Put in anonymous namespace since it's local to the file.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:53
@@ +52,3 @@
+  std::string VarAnnotation("llvm.var.annotation");
+  std::string PtrAnnotation("llvm.ptr.annotation");
+  return ((FunName.compare(0, VarAnnotation.length(), VarAnnotation) == 0) ||
----------------
You should probably hoist the above strings with efa_annotation since it's use in other parts of the file.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:180
@@ +179,3 @@
+      } else if (StoreInst *S = dyn_cast<StoreInst>(Us)) {
+        if (Us->getOperand(0) == V) {
+          DEBUG(dbgs() << "Warning: in " << F->getName() << " an"
----------------
S->getValueOperand()

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:184
@@ +183,3 @@
+                       << " pointer.\n");
+        } else if (Us->getOperand(1) == V) {
+          MaybeExternalStores.insert(S);
----------------
S->getPointerOperand()

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:198
@@ +197,3 @@
+
+  size_t annotation_len = strlen(efa_annotation) + 1;
+  Value::use_iterator FI, FE;
----------------
This could be a constant if the declaration were an array, and you used array_lengthof.

================
Comment at: include/llvm/Analysis/ExternalFunctionAnalysis.h:21
@@ +20,3 @@
+#include <map>
+#include <set>
+
----------------
I'll let others comment here, but it seems like LLVM likes using its own datastructures for some use cases in the Programmer's manual. I don't think you need SetVector<Value*> since you AFAICT you don't traverse the set, but you may want DenseMap.

================
Comment at: include/llvm/Analysis/ExternalFunctionAnalysis.h:10
@@ +9,3 @@
+/// \brief A pass that finds incoming external function pointers and finds
+/// annotated storage locations and indirect calls based on these locations.
+///
----------------
It would be nice to have a better explanation of the algorithm and its use here, or on the class. You had some good information in your email.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:417
@@ +416,3 @@
+  // which the call to the first function took place.
+  std::string New("_Znwm");
+  StoreSources FPStores;
----------------
You should use include/llvm/Target/TargetLibraryInfo.h. You're also missing the nothrow variant.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:432
@@ +431,3 @@
+
+    // Don't chase values from operator new.
+    std::string Name(F->getName());
----------------
Why not malloc too? Is this just a performance thing, or does it impact correctness?


http://llvm-reviews.chandlerc.com/D2873