[PATCH] D21475: [CFLAA] Summarize interprocedural aliasing information instead of recomputing it at callsite

[George Burgess IV via llvm-commits]

george.burgess.iv added inline comments.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:394
@@ -421,1 +393,3 @@
         return false;
+      // Try to be defensive if the caller does not provide enough arguments
+      if (Fn->arg_size() > CS.arg_size())
----------------
grievejia wrote:
> george.burgess.iv wrote:
> > > I thought I asked you for justifications some time ago
> > 
> > You did :)
> > 
> > IIRC, the best I could offer you was "we somehow devirtualized the function pointer to N different functions, and their arities didn't match up." Given that that's the best explanation we have (and if we ever do anything fancy with indirect calls, it's probably going to be the resolver's job to ensure that the potential callee makes sense to call), I'm tempted to turn this into an assert, rather than guarding against this case without knowing why. You can blame me if it ends up breaking anything. :)
> > 
> > It also looks like this change would also make us do interprocedural analysis on variadic functions -- I think that should be fine, but can we have a test for that, please? If it helps, here's a link to vararg stuff: http://llvm.org/docs/LangRef.html#variable-argument-handling-intrinsics
> Regarding variadic functions: 
> 
> AFAIK, the LLVM va_arg instruction is mostly dead, at least on platforms I commonly use. The docs are outdated w.r.t. this issue. Clang handles C/C++ variadic functions in a highly platform-dependent manner, and it generally try to avoid emitting any va_arg instructions (you can verify this on your machine).
> 
> My point is that variadic function is a complex thing to support and I'm not sure this simple change will buy us that much.
> My point is that variadic function is a complex thing to support and I'm not sure this simple change will buy us that much

If we don't want to support interprocedural analysis on varargs functions, that's perfectly fine with me. My point is that relaxing this check to `if (param_size() > arg_size()) return false;` allows variadic functions (that were given more than zero args in their arg pack) to be analyzed interprocedurally. If we don't want to support that, we should either explicitly ask the callee if it takes an arg pack (and fail if it does), or we should un-loosen this check. :)


http://reviews.llvm.org/D21475