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

[JF Bastien]

  Looks good overall, but it would be great to have someone more seasoned with LLVM reviewing this.


================
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");
+
----------------
Tom Roeder wrote:
> JF Bastien wrote:
> > The assert is redundant when using cast<>.
> The assert is for getOperand rather than the cast. But maybe getOperand can't return null. It will certainly assert if its argument is out of range.
Oh right, I was over-zealous on cast<>.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:251
@@ +250,3 @@
+
+  size_t annotation_len = array_lengthof(efa_annotation);
+  for (const auto &Us : PtrAnnotation->users()) {
----------------
Tom Roeder wrote:
> JF Bastien wrote:
> > This should be array_lengthof(efa_annotation) - 1? The array contains the null terminator. The same applies a few places below.
> It turns out that the initializer length in the IR also contains the null terminator, so both strings end up with the same length this way.
Hmm, weird. It still seems wrong to initialized the StringRef below counting the null terminator, instead of excluding it. Isn't the StringRef str below initialized with just the const char * constructor for StringRef? That would then set Length with strlen on Data, and the length would mismatch that of StringRef efa (because StringRef::equal first compare length, and then compares memory).

Anyhow, I may be confused, but I'm surprised when string comparison counts the null terminator too, it seems like it could break in odd ways if one of the string's length starts not containing the null terminator.

================
Comment at: lib/Analysis/ExternalFunctionAnalysis.cpp:277
@@ +276,3 @@
+  size_t annotation_len = array_lengthof(efa_annotation);
+  StringRef efa(efa_annotation, annotation_len);
+  for (const auto &Us : VarAnnotation->users()) {
----------------
Unrelated to your change, but I was sad that StringRef can't have the following constructor:
```
template<size_t N> StringRef(const char (&Data)[N]) : StringRef(Data, N) { }
```
Templates don't participate in overload resolution if there is a best match nontemplate overload.


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