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

[Philip Reames via cfe-commits]

Author: Philip Reames
Date: 2021-12-17T09:02:03-08:00
New Revision: 33cbaab1416b234d5a08b41e3110d64a00b0651c

URL: https://github.com/llvm/llvm-project/commit/33cbaab1416b234d5a08b41e3110d64a00b0651c
DIFF: https://github.com/llvm/llvm-project/commit/33cbaab1416b234d5a08b41e3110d64a00b0651c.diff

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

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.

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

Added: 
    

Modified: 
    clang/test/CodeGen/arm-cmse-attr.c
    llvm/lib/Transforms/IPO/FunctionAttrs.cpp
    llvm/test/Transforms/FunctionAttrs/nocapture.ll
    llvm/test/Transforms/FunctionAttrs/writeonly.ll

Removed: 
    


################################################################################
diff  --git a/clang/test/CodeGen/arm-cmse-attr.c b/clang/test/CodeGen/arm-cmse-attr.c
index 5cfadfd3828a1..ae0b606a0bb21 100644
--- a/clang/test/CodeGen/arm-cmse-attr.c
+++ b/clang/test/CodeGen/arm-cmse-attr.c
@@ -29,9 +29,9 @@ 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 {

diff  --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
index 2cee9c0b4766a..8c8aea465c490 100644
--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -702,6 +702,11 @@ determinePointerAccessAttrs(Argument *A,
       };
 
       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;

diff  --git a/llvm/test/Transforms/FunctionAttrs/nocapture.ll b/llvm/test/Transforms/FunctionAttrs/nocapture.ll
index 3c699de9df6c9..ab84a0cece52e 100644
--- a/llvm/test/Transforms/FunctionAttrs/nocapture.ll
+++ b/llvm/test/Transforms/FunctionAttrs/nocapture.ll
@@ -128,7 +128,7 @@ define void @nc2(i32* %p, i32* %q) {
 }
 
 
-; 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
@@ -141,7 +141,7 @@ define void @nc4(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)
@@ -319,21 +319,21 @@ define i1 @captureDereferenceableOrNullICmp(i32* dereferenceable_or_null(4) %x)
 
 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

diff  --git a/llvm/test/Transforms/FunctionAttrs/writeonly.ll b/llvm/test/Transforms/FunctionAttrs/writeonly.ll
index fb391111b301b..54d00d355f7af 100644
--- a/llvm/test/Transforms/FunctionAttrs/writeonly.ll
+++ b/llvm/test/Transforms/FunctionAttrs/writeonly.ll
@@ -92,19 +92,19 @@ define void @direct3(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