[funcattrs] Consistently treat calling a function pointer as a non-capturing read

[reames]

Summary

We were being wildly inconsistent about what memory access was implied by an indirect function call. Depending on the call site attributes, you could get anything from a read, to unknown, to none at all. (The last was a miscompile.)

We were also always traversing the uses of a readonly indirect call. This is entirely unneeded as the indirect call does not capture. The callee might capture itself internally, but that has no implications for this caller. (See the nice explanation in the CaptureTracking comments if that case is confusing.)

Note that elsewhere in the same file, we were correctly computing the nocapture attribute for indirect calls. The changed case only resulted in conservatism when computing memory attributes if say the return value was written to.

[nikic]

LGTM

clang/test/CodeGen/arm-cmse-attr.c

 void f3()
 {
 }
 
 void f4() __attribute__((cmse_nonsecure_entry))
 {
 }
 
-// CHECK: define{{.*}} void @f1(void ()* nocapture %fptr) {{[^#]*}}#0 {
+// CHECK: define{{.*}} void @f1(void ()* nocapture readonly %fptr) {{[^#]*}}#0 {
 // CHECK: call void %fptr() #2
-// CHECK: define{{.*}} void @f2(void ()* nocapture %fptr) {{[^#]*}}#0 {
+// CHECK: define{{.*}} void @f2(void ()* nocapture readonly %fptr) {{[^#]*}}#0 {
 // CHECK: call void %fptr() #2
 // CHECK: define{{.*}} void @f3() {{[^#]*}}#1 {
 // CHECK: define{{.*}} void @f4() {{[^#]*}}#1 {
 
 // CHECK-NOSE-NOT: cmse_nonsecure_entry
 // CHECK-NOSE-NOT: cmse_nonsecure_call
 // CHECK-SE: attributes #0 = { nounwind
 // CHECK-SE: attributes #1 = { {{.*}} "cmse_nonsecure_entry"
 // CHECK-SE: attributes #2 = { {{.*}} "cmse_nonsecure_call"

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

case Instruction::Invoke: {
       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
+        continue;
+      }
       if (CB.doesNotAccessMemory()) {
         AddUsersToWorklistIfCapturing();
         continue;
       }
 
       Function *F = CB.getCalledFunction();
       if (!F) {
         if (CB.onlyReadsMemory()) {

llvm/test/Transforms/FunctionAttrs/nocapture.ll

 
 ; FNATTR: define void @nc2(i32* nocapture %p, i32* %q)
 define void @nc2(i32* %p, i32* %q) {
   %1 = call i32 @nc1(i32* %q, i32* %p, i1 0)    ; <i32> [#uses=0]
   ret void
 }
 
 
-; FNATTR: define void @nc3(void ()* nocapture %p)
+; FNATTR: define void @nc3(void ()* nocapture readonly %p)
 define void @nc3(void ()* %p) {
   call void %p()
   ret void
 }
 
 declare void @external(i8*) readonly nounwind
 ; FNATTR: define void @nc4(i8* nocapture readonly %p)
 define void @nc4(i8* %p) {
   call void @external(i8* %p)
   ret void
 }
 
-; FNATTR: define void @nc5(void (i8*)* nocapture %f, i8* nocapture %p)
+; FNATTR: define void @nc5(void (i8*)* nocapture readonly %f, i8* nocapture %p)
 define void @nc5(void (i8*)* %f, i8* %p) {
   call void %f(i8* %p) readonly nounwind
   call void %f(i8* nocapture %p)
   ret void
 }
 
 ; FNATTR:     define void @test1_1(i8* nocapture readnone %x1_1, i8* %y1_1, i1 %c)
 ; It would be acceptable to add readnone to %y1_1 and %y1_2.

 define i1 @captureDereferenceableOrNullICmp(i32* dereferenceable_or_null(4) %x) null_pointer_is_valid {
   %1 = bitcast i32* %x to i8*
   %2 = icmp eq i8* %1, null
   ret i1 %2
 }
 
 declare void @capture(i8*)
 
-; FNATTR: define void @nocapture_fptr(i8* (i8*)* nocapture %f, i8* %p)
+; FNATTR: define void @nocapture_fptr(i8* (i8*)* nocapture readonly %f, i8* %p)
 define void @nocapture_fptr(i8* (i8*)* %f, i8* %p) {
   %res = call i8* %f(i8* %p)
   call void @capture(i8* %res)
   ret void
 }
 
-; FNATTR: define void @recurse_fptr(i8* (i8*)* nocapture %f, i8* %p)
+; FNATTR: define void @recurse_fptr(i8* (i8*)* nocapture readonly %f, i8* %p)
 define void @recurse_fptr(i8* (i8*)* %f, i8* %p) {
   %res = call i8* %f(i8* %p)
   store i8 0, i8* %res
   ret void
 }
 
-; FNATTR: define void @readnone_indirec(void (i8*)* nocapture readnone %f, i8* readnone %p)
+; FNATTR: define void @readnone_indirec(void (i8*)* nocapture readonly %f, i8* readnone %p)
 define void @readnone_indirec(void (i8*)* %f, i8* %p) {
   call void %f(i8* %p) readnone
   ret void
 }
 
 
 declare i8* @llvm.launder.invariant.group.p0i8(i8*)
 declare i8* @llvm.strip.invariant.group.p0i8(i8*)

llvm/test/Transforms/FunctionAttrs/writeonly.ll

 declare void @direct3_callee(i8* writeonly %p)
 
 ; CHECK: define void @direct3(i8* %p)
 define void @direct3(i8* %p) {
   call void @direct3_callee(i8* %p)
   ret void
 }
 
-; CHECK: define void @fptr_test1(i8* %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test1(i8* %p, void (i8*)* nocapture readonly %f)
 define void @fptr_test1(i8* %p, void (i8*)* %f) {
   call void %f(i8* %p)
   ret void
 }
 
-; CHECK: define void @fptr_test2(i8* %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test2(i8* %p, void (i8*)* nocapture readonly %f)
 define void @fptr_test2(i8* %p, void (i8*)* %f) {
   call void %f(i8* writeonly %p)
   ret void
 }
 
-; CHECK: define void @fptr_test3(i8* %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test3(i8* %p, void (i8*)* nocapture readonly %f)
 define void @fptr_test3(i8* %p, void (i8*)* %f) {
   call void %f(i8* %p) writeonly
   ret void
 }
 
 ; CHECK: attributes #0 = { {{.*}}readnone{{.*}} }
 ; CHECK: attributes #1 = { {{.*}}readonly{{.*}} }
 ; CHECK: attributes #2 = { {{.*}}writeonly{{.*}} }