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

[Nick Lewycky]

================
Comment at: include/llvm/Analysis/ExternalFunctionAnalysis.h:38-39
@@ +37,4 @@
+///      that is not defined or declared in the current Module?
+///   2. Does this Function contain indirect calls that maybe target functions
+///      that are not defined or declared in the current Module?
+///
----------------
This means "does this Function contain any Instruction matching case #1" right?

================
Comment at: include/llvm/Analysis/ExternalFunctionAnalysis.h:47-50
@@ +46,6 @@
+///
+/// EFA looks for function pointers returned by functions external to the module
+/// it is analyzing, and it traces the dataflow of these incoming external
+/// function pointers to find places where they are stored or called in indirect
+/// function calls.
+///
----------------
There's three possible states, certainly not calling an external function, certainly calling an external function, and who knows.

The previous paragraph:
///   1. Is this Instruction an indirect call that is maybe calling a function
///      that is not defined or declared in the current Module?
suggests that you answer "false" only when certainly not calling into an external function, and "true" when calling into an external function or you're not sure. This paragraph suggests that you're going to track uses of external function pointers and try to track them, and that's only going to help you solve cases which are certainly calling an external function.

Which way is it? I can't review the dataflow part of this patch without understanding this.

================
Comment at: include/llvm/Analysis/ExternalFunctionAnalysis.h:89-92
@@ +88,6 @@
+private:
+  typedef struct {
+    const Function *Source;
+    const Function *Caller;
+  } SourcePair;
+
----------------
In C++ "struct SourcePair { ... };" will suffice.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:1
@@ +1,2 @@
+//=- ExternalFunctionAnalysis.cpp: Find external function pointers -*- C++ -*-//
+//
----------------
No need for the emacs major mode marker on a .cpp file, just on the .h files.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:71-82
@@ +70,14 @@
+
+static const Value *getParameterForArgument(unsigned ArgNo, const Function *F) {
+  unsigned count = 0;
+  Function::const_arg_iterator FAI, FAE;
+  for (FAI = F->arg_begin(), FAE = F->arg_end(); FAI != FAE; ++FAI, ++count) {
+    if (count == ArgNo) {
+      const Value *Arg = FAI;
+      return Arg;
+    }
+  }
+
+  return NULL;
+}
+
----------------
This always returns an Argument*, any reason not to make that clear in the type? Why not use std::advance? I think this becomes:

  static const Argument *getParameterForArgument(unsigned ArgNo, const Function *F) {
    if (ArgNo < F->arg_size()) return nullptr;
    return *std::advance(F->arg_begin(), ArgNo);
  }

but I haven't tried to compile that.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:132-134
@@ +131,5 @@
+  // argument we want is in the "..." part.
+  const Value *Arg = getParameterForArgument(ArgNo, CalledFun);
+  if (Arg)
+    followValue(Arg, FoundValues, SeenValues);
+}
----------------
The form:

  if (const Value *Arg = getParameterForArgument(ArgNo, CalledFun))
    followValue(Arg, FoundValues, SeenValues);

is very common in LLVM.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:299-300
@@ +298,4 @@
+
+    // We can't use a vector type here because this algorithm depends on being
+    // able to continue iterating through FoundValues as new items are added.
+    std::list<const Value *> FoundValues;
----------------
It's possible to do that with a vector too, you just need to keep an index number (I assume you only add to the back).

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:372-373
@@ +371,4 @@
+  }
+}
+bool ExternalFunctionAnalysis::runOnModule(Module &M) {
+  // Build up the sets of maybe-external functions, variables, pointers, and
----------------
Missing blank line.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:392
@@ +391,3 @@
+
+  TargetLibraryInfo TLI;
+
----------------
Don't create a new one here, use the one that was created for the given target and module being compiled:
  const TargetLibraryInfo *TLI = P->getAnalysisIfAvailable<TargetLibraryInfo>();


================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:424
@@ +423,3 @@
+      // of false positives.
+      if (isAllocationFn(I, &TLI, true /* LookThroughBitCast */))
+        break;
----------------
TargetLibraryInfo supports a whole ton of different functions. Do you just care about allocation functions or any recognized library function (strlen, etc.)?

================
Comment at: test/Analysis/ExternalFunctionAnalysis/external_function_analysis.ll:1
@@ +1,2 @@
+; RUN: llvm-as < %s >%t1
+; RUN: opt -efa -o %t2 %t1 -stats -debug-only=efa 2>&1 | FileCheck %s
----------------
That shouldn't be necessary, opt works on .ll files as well as .bc files.

================
Comment at: test/Analysis/ExternalFunctionAnalysis/external_function_analysis.ll:2
@@ +1,3 @@
+; RUN: llvm-as < %s >%t1
+; RUN: opt -efa -o %t2 %t1 -stats -debug-only=efa 2>&1 | FileCheck %s
+
----------------
If you plan to discard %t2, use -disable-output instead?

================
Comment at: test/Analysis/ExternalFunctionAnalysis/external_function_analysis.ll:2
@@ +1,3 @@
+; RUN: llvm-as < %s >%t1
+; RUN: opt -efa -o %t2 %t1 -stats -debug-only=efa 2>&1 | FileCheck %s
+
----------------
Nick Lewycky wrote:
> If you plan to discard %t2, use -disable-output instead?
Please don't use -debug-only=efa here, it makes your test only work on debug builds of llvm and not optimized builds. Instead, you could implement Pass::print() in your pass and then use that via "opt -analyze". This is the same thing that the "opt -analyze -scalar-evolution" tests do with the ScalarEvolution analysis.

================
Comment at: test/Analysis/ExternalFunctionAnalysis/external_function_analysis.ll:150
@@ +149,3 @@
+
+; XFAIL: win32
+; CHECK: External function 'f' returns a pointer
----------------
Why?

http://reviews.llvm.org/D2873