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

[Duncan P. N. Exon Smith via llvm-commits]

Yes, most of the iplist instantiations use a `ilist_half_node`, which
somehow magically re-attaches the cut-off head of the list, spoiling the
is-a-sentinel detection.  I'm not surprised it's hard to demonstrate
this bug.

This patch LGTM once Philip is happy.

> On 2015-Nov-04, at 15:04, David Blaikie <dblaikie at gmail.com> wrote:
> 
> +Duncan, who's been mucking about with llvm's ilist issues recently
> 
> On Wed, Nov 4, 2015 at 2:37 PM, Sanjoy Das via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> sanjoy created this revision.
> sanjoy added reviewers: chandlerc, reames.
> sanjoy added a subscriber: llvm-commits.
> 
> This change fixes an iterator wraparound bug in
> `determinePointerReadAttrs`.
> 
> Ideally, ++'ing off the `end()` of an iplist should result in a failed
> assert, but currently iplist seems to silently wrap to the head of the
> list on `end()++`.  This is why the bad behavior is difficult to
> demonstrate.
> 
> http://reviews.llvm.org/D14350
> 
> Files:
>   lib/Transforms/IPO/FunctionAttrs.cpp
>   test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll
> 
> Index: test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll
> ===================================================================
> --- /dev/null
> +++ test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll
> @@ -0,0 +1,30 @@
> +; RUN: opt -functionattrs -S < %s | FileCheck %s
> +
> +; This checks for an iterator wraparound bug in FunctionAttrs.  The previous
> +; "incorrect" behavior was inferring readonly for the %x argument in @caller.
> +; Inferring readonly for %x *is* actually correct, since @va_func is marked
> +; readonly, but FunctionAttrs was inferring readonly for the wrong reasons (and
> +; we _need_ the readonly on @va_func to trigger the problematic code path).  It
> +; is possible that in the future FunctionAttrs becomes smart enough to infer
> +; readonly for %x for the right reasons, and at that point this test will have
> +; to be marked invalid.
> +
> +declare void @llvm.va_start(i8*)
> +declare void @llvm.va_end(i8*)
> +
> +define void @va_func(i32* readonly %b, ...) readonly nounwind {
> +; CHECK-LABEL: define void @va_func(i32* nocapture readonly %b, ...)
> + entry:
> +  %valist = alloca i8
> +  call void @llvm.va_start(i8* %valist)
> +  call void @llvm.va_end(i8* %valist)
> +  %x = call i32 @caller(i32* %b)
> +  ret void
> +}
> +
> +define i32 @caller(i32* %x) {
> +; CHECK-LABEL: define i32 @caller(i32* nocapture %x)
> + entry:
> +  call void(i32*,...) @va_func(i32* null, i32 0, i32 0, i32 0, i32* %x)
> +  ret i32 42
> +}
> Index: lib/Transforms/IPO/FunctionAttrs.cpp
> ===================================================================
> --- lib/Transforms/IPO/FunctionAttrs.cpp
> +++ lib/Transforms/IPO/FunctionAttrs.cpp
> @@ -442,9 +442,11 @@
> 
>        Function::arg_iterator AI = F->arg_begin(), AE = F->arg_end();
>        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) {
> +        InVarArgSection |= (AI == AE);
>          if (A->get() == V) {
> -          if (AI == AE) {
> +          if (InVarArgSection) {
>              assert(F->isVarArg() &&
>                     "More params than args in non-varargs call.");
>              return Attribute::None;
> @@ -457,6 +459,8 @@
>            if (!CS.doesNotAccessMemory(A - B))
>              IsRead = true;
>          }
> +        if (!InVarArgSection)
> +          ++AI;
>        }
>        AddUsersToWorklistIfCapturing();
>        break;
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 
>