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

[JF Bastien]

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:131
@@ +130,3 @@
+  // This can return NULL if CalledFun is a VarArg function, and the
+  // argument we want is in the "..." part.
+  const Value *Arg = getParameterForArgument(ArgNo, CalledFun);
----------------
Doesn't the algorithm become imprecise if you don't follow vaarg arguments? AFAICT the call may just fail? Realistically it's probably just printf, but it would be good to document what happens with:
```
void ugly(size_t n, ...) {
  typedef void (*F)();
  va_list list;
  va_start(list, n);
  for (size_t i = 0; i != n; ++n) {
    F f = va_arg(list, F);
    (*f)();
  }
  va_end(list);
}
void callee() {
  printf("Callee\n");
}
int main() {
  ugly(1, &callee);
}
```
Maybe add a test if it's expected to work?

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:140
@@ +139,3 @@
+  const Instruction *I = dyn_cast<Instruction>(Val);
+  assert(I && "Couldn't get an instruction from a return value");
+
----------------
This could just be a cast<> and get the assert for free.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:236
@@ +235,3 @@
+                       << " efa-maybe-external pointer gets stored into another"
+                       << " pointer.\n");
+        } else if (S->getPointerOperand() == V) {
----------------
I'm not sure I understand what the bad implication is here, could you improve the warning to explain? Is it a kind of taint tracking thing, where it hard to know which maybe-external the pointer now refers to?

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:260
@@ +259,3 @@
+    const Value *V = cast<User>(OpVal)->getOperand(0);
+    assert(V && "Couldn't get operand 0 of operand 1 of a User");
+
----------------
The assert is redundant when using cast<>.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:267
@@ +266,3 @@
+    if ((count == annotation_len) &&
+        (memcmp(str.data(), efa_annotation, count) == 0)) {
+      // The llvm.ptr.annotation returns a value that replaces the first arg.
----------------
You could build a local StringRef(efa_annotation, annotation_len) and then just use operator== here. The same applies below.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:251
@@ +250,3 @@
+
+  size_t annotation_len = array_lengthof(efa_annotation);
+  for (const auto &Us : PtrAnnotation->users()) {
----------------
This should be array_lengthof(efa_annotation) - 1? The array contains the null terminator. The same applies a few places below.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:289
@@ +288,3 @@
+    const Value *V = cast<User>(OpVal)->getOperand(0);
+    assert(V && "Couldn't get operand 0 of operand 1 of a User");
+    const ConstantDataSequential *StrInit =
----------------
Redundant.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:299
@@ +298,3 @@
+      const User *OpUser = cast<User>(Us->getOperand(0));
+      assert(OpUser && "Couldn't get the User for operand 0");
+      const Value *VPtr = OpUser->getOperand(0);
----------------
Redundant.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:301
@@ +300,3 @@
+      const Value *VPtr = OpUser->getOperand(0);
+      assert(VPtr && "Couldn't get the second operand 0 for the annotation");
+      findRelatedInstrs(VPtr);
----------------
Redundant.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:327
@@ +326,3 @@
+          cast<GlobalVariable>(StrV)->getInitializer());
+      assert(StrInit && "Couldn't get the string initializer for annotation");
+      StringRef str = StrInit->getAsString();
----------------
3 redundant asserts above.


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