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

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.

Differential Revision: https://reviews.llvm.org/D115961

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

@@ -681,34 +681,39 @@ determinePointerAccessAttrs(Argument *A,
 
     case Instruction::Call:
     case Instruction::Invoke: {
-      bool Captures = true;
-
-      if (I->getType()->isVoidTy())
-        Captures = false;
-
-      auto AddUsersToWorklistIfCapturing = [&] {
-        if (Captures)
-          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
+        // Note that indirect calls do not capture, see comment in
+        // CaptureTracking for context
         continue;
       }
-      if (CB.doesNotAccessMemory()) {
-        AddUsersToWorklistIfCapturing();
-        continue;
+
+      // 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);
+
+      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);
       }
+      
+      if (CB.doesNotAccessMemory())
+        continue;
 
       Function *F = CB.getCalledFunction();
       if (!F) {
         if (CB.onlyReadsMemory()) {
           IsRead = true;
-          AddUsersToWorklistIfCapturing();
           continue;
         }
         return Attribute::None;
@@ -716,21 +721,17 @@ determinePointerAccessAttrs(Argument *A,
 
       // 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;
       }
 
@@ -740,7 +741,6 @@ determinePointerAccessAttrs(Argument *A,
         return Attribute::None;
       if (!CB.doesNotAccessMemory(UseIndex))
         IsRead = true;
-      AddUsersToWorklistIfCapturing();
       break;
     }
 
@@ -995,6 +995,13 @@ static void addArgumentAttrs(const SCCNodeSet &SCCNodes,
         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;
     }

llvm/test/Transforms/FunctionAttrs/readattrs.ll

@@ -3,11 +3,9 @@
 
 @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
@@ -130,10 +128,8 @@ declare void @escape_readonly_ptr(i8** %addr, i8* 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*