[PATCH] D14350: [FunctionAttrs] Fix an iterator wraparound bug

[Nick Lewycky via llvm-commits]

On 4 November 2015 at 14:50, Sanjoy Das <sanjoy at playingwithpointers.com>
wrote:

> sanjoy added a subscriber: nlewycky.
>
> ================
> Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:445
> @@ -444,2 +444,3 @@
>        CallSite::arg_iterator B = CS.arg_begin(), E = CS.arg_end();
> -      for (CallSite::arg_iterator A = B; A != E; ++A, ++AI) {
> +      bool InVarArgSection = false;
> +      for (CallSite::arg_iterator A = B; A != E; ++A) {
> ----------------
> reames wrote:
> > I would rather we just gave up on VarArg callsites unless you have a
> compelling reason we need to handle them here.  Return a conservative
> answer, and let someone else refine it later.
> >
> > p.s. Does the other patch have the same issue?
> I didn't add the code, so I'm not in a position to judge whether there is
> a compelling reason for this optimization or not. :)
>
> Moreover, bailing out unconditionally on varargs functions breaks
> `test/Transforms/FunctionAttrs/readattrs.ll` (added by @nlewycky in Jul
> 2013), and I'd rather not pessimize transforms that have in-tree tests.
>

It's long ago enough that I'm not 100% sure, but IIRC the varargs case got
the first test because it happened to me late in testing the original
patch, before I started writing a proper testcase file. I don't think
varargs are really important to optimize, but it would match the string
literals in printf-like functions, and it would be nice to get that right.

> p.s. Does the other patch have the same issue?
>
> If you mean D14306, then I don't think so -- that change is fine as far as
> I can tell.
>
>
>
> http://reviews.llvm.org/D14350
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151104/0373f37a/attachment.html>