[funcattrs] Fix incorrect readnone/readonly inference on captured arguments

[reames]

Summary

This fixes a bug where we would infer readnone/readonly for a function which passed a value to a function which could capture it. With the value captured in memory, the function could reload the value from memory after the call, and write to it. Inferring the argument as readnone or readonly is unsound.

@jdoerfert apparently noticed this about two years ago, and tests were checked in with 76467c4, but the issue appears to have never gotten fixed.

Since this seems like this issue should break everything, let me explain why the case is actually fairly narrow. The main inference loop over the argument SCCs only analyzes nocapture arguments. As such, we can only hit this when construction the partial SCCs. Due to that restriction, we can only hit this when we have either a) a function declaration with a manually annotated argument, or b) an immediately self recursive call.

It's also worth highlighting that we do have cases we can infer readonly/readnone on a capturing argument validly. The easiest example is a function which simply returns its argument without ever accessing it.

[reames]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

999

Note: This bit is arguably not part of the bug fix, but it's unreachable without it, and without it the bugfix causes massive test diffs.

[jdoerfert]

Drive-by, I'm on vacation.

Glad someone fixes this, even if I was hoping we replace all this by invoking the Attributor only for the IR attributes we are looking for here.

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

702

I don't know if it is checked but unwinding is the same issue as writes. The Attributor code might have even more corner cases covered.

[reames]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

702

I don't believe we can have a well defined readonly throwing function.

[reames]

ping

[nikic]

LG

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

case Instruction::AddrSpaceCast:
       // The original value is not read/written via this if the new value isn't.
       for (Use &UU : I->uses())
         if (Visited.insert(&UU).second)
           Worklist.push_back(&UU);
       break;
 
     case Instruction::Call:
     case Instruction::Invoke: {
-      bool Captures = true;
+      CallBase &CB = cast<CallBase>(*I);
+      if (CB.isCallee(U)) {
+        IsRead = true;
+        // Note that indirect calls do not capture, see comment in
+        // CaptureTracking for context
+        continue;
+      }
 
-      if (I->getType()->isVoidTy())
-        Captures = false;
+      // Given we've explictily handled the callee operand above, what's left
+      // must be a data operand (e.g. argument or operand bundle)
+      const unsigned UseIndex = CB.getDataOperandNo(U);
 
-      auto AddUsersToWorklistIfCapturing = [&] {
-        if (Captures)
+      if (!CB.doesNotCapture(UseIndex)) {
+        if (!CB.onlyReadsMemory())
+          // If the callee can save a copy into other memory, then simply
+          // scanning uses of the call is insufficient.  We have no way
+          // of tracking copies of the pointer through memory to see
+          // if a reloaded copy is written to, thus we must give up.
+          return Attribute::None;
+        // Push users for processing once we finish this one
+        if (!I->getType()->isVoidTy())
           for (Use &UU : I->uses())
             if (Visited.insert(&UU).second)
               Worklist.push_back(&UU);
-      };
-
-      CallBase &CB = cast<CallBase>(*I);
-      if (CB.isCallee(U)) {
-        IsRead = true;
-        Captures = false; // See comment in CaptureTracking for context
-        continue;
       }
-      if (CB.doesNotAccessMemory()) {
-        AddUsersToWorklistIfCapturing();
+      
+      if (CB.doesNotAccessMemory())
         continue;
-      }
 
       Function *F = CB.getCalledFunction();
       if (!F) {
         if (CB.onlyReadsMemory()) {
           IsRead = true;
-          AddUsersToWorklistIfCapturing();
           continue;
         }
         return Attribute::None;
       }
 
       // Given we've explictily handled the callee operand above, what's left
       // must be a data operand (e.g. argument or operand bundle)
-      const unsigned UseIndex = CB.getDataOperandNo(U);
       const bool IsOperandBundleUse = UseIndex >= CB.arg_size();
-
       if (UseIndex >= F->arg_size() && !IsOperandBundleUse) {
         assert(F->isVarArg() && "More params than args in non-varargs call");
         return Attribute::None;
       }
 
-      Captures &= !CB.doesNotCapture(UseIndex);
 
       if (CB.isArgOperand(U) && SCCNodes.count(F->getArg(UseIndex))) {
         // This is an argument which is part of the speculative SCC.  Note that
         // only operands corresponding to formal arguments of the callee can
         // participate in the speculation.
-        AddUsersToWorklistIfCapturing();
         break;
       }
 
       // The accessors used on call site here do the right thing for calls and
       // invokes with operand bundles.
       if (!CB.onlyReadsMemory() && !CB.onlyReadsMemory(UseIndex))
         return Attribute::None;
       if (!CB.doesNotAccessMemory(UseIndex))
         IsRead = true;
-      AddUsersToWorklistIfCapturing();
       break;
     }
 
     case Instruction::Load:
       // A volatile load has side effects beyond what readonly can be relied
       // upon.
       if (cast<LoadInst>(I)->isVolatile())
         return Attribute::None;
if (ArgumentSCC.size() == 1) {
 
       // eg. "void f(int* x) { if (...) f(x); }"
       if (ArgumentSCC[0]->Uses.size() == 1 &&
           ArgumentSCC[0]->Uses[0] == ArgumentSCC[0]) {
         Argument *A = ArgumentSCC[0]->Definition;
         A->addAttr(Attribute::NoCapture);
         ++NumNoCapture;
         Changed.insert(A->getParent());
+
+        // Infer the access attributes given the new nocapture one
+        SmallPtrSet<Argument *, 8> Self;
+        Self.insert(&*A);
+        Attribute::AttrKind R = determinePointerAccessAttrs(&*A, Self);
+        if (R != Attribute::None)
+          addAccessAttr(A, R);
       }
       continue;
     }
 
     bool SCCCaptured = false;
     for (auto I = ArgumentSCC.begin(), E = ArgumentSCC.end();
          I != E && !SCCCaptured; ++I) {
       ArgumentGraphNode *Node = *I;

llvm/test/Transforms/FunctionAttrs/readattrs.ll

 ; RUN: opt < %s -function-attrs -S | FileCheck %s
 ; RUN: opt < %s -aa-pipeline=basic-aa -passes='cgscc(function-attrs)' -S | FileCheck %s
 
 @x = global i32 0
 
-declare void @test1_1(i8* %x1_1, i8* readonly %y1_1, ...)
+declare void @test1_1(i8* %x1_1, i8* nocapture readonly %y1_1, ...)
 
-; NOTE: readonly for %y1_2 would be OK here but not for the similar situation in test13.
-;
-; CHECK: define void @test1_2(i8* %x1_2, i8* readonly %y1_2, i8* %z1_2)
+; CHECK: define void @test1_2(i8* %x1_2, i8* nocapture readonly %y1_2, i8* %z1_2)
 define void @test1_2(i8* %x1_2, i8* %y1_2, i8* %z1_2) {
   call void (i8*, i8*, ...) @test1_1(i8* %x1_2, i8* %y1_2, i8* %z1_2)
   store i32 0, i32* @x
   ret void
 }
 
 ; CHECK: define i8* @test2(i8* readnone returned %p)
 define i8* @test2(i8* %p) {

 declare void @escape_readnone_ptr(i8** %addr, i8* readnone %ptr)
 declare void @escape_readonly_ptr(i8** %addr, i8* readonly %ptr)
 
 ; The argument pointer %escaped_then_written cannot be marked readnone/only even
 ; though the only direct use, in @escape_readnone_ptr/@escape_readonly_ptr,
 ; is marked as readnone/only. However, the functions can write the pointer into
 ; %addr, causing the store to write to %escaped_then_written.
 ;
-; FIXME: This test currently exposes a bug in function-attrs!
-;
-; CHECK: define void @unsound_readnone(i8* nocapture readnone %ignored, i8* readnone %escaped_then_written)
-; CHECK: define void @unsound_readonly(i8* nocapture readnone %ignored, i8* readonly %escaped_then_written)
+; CHECK: define void @unsound_readnone(i8* nocapture readnone %ignored, i8* %escaped_then_written)
+; CHECK: define void @unsound_readonly(i8* nocapture readnone %ignored, i8* %escaped_then_written)
 ;
 define void @unsound_readnone(i8* %ignored, i8* %escaped_then_written) {
   %addr = alloca i8*
   call void @escape_readnone_ptr(i8** %addr, i8* %escaped_then_written)
   %addr.ld = load i8*, i8** %addr
   store i8 0, i8* %addr.ld
   ret void
 }
 
 define void @unsound_readonly(i8* %ignored, i8* %escaped_then_written) {
   %addr = alloca i8*
   call void @escape_readonly_ptr(i8** %addr, i8* %escaped_then_written)
   %addr.ld = load i8*, i8** %addr
   store i8 0, i8* %addr.ld
   ret void
 }