[FunctionAttrs] try to extend nonnull-ness of arguments from a callsite back to its...

As discussed here:
http://lists.llvm.org/pipermail/llvm-dev/2016-December/108182.html
...we should be able to propagate 'nonnull' info from a callsite back to its parent.

The original motivation for this patch is our botched optimization of "dyn_cast" (PR28430),
but this won't solve that problem.

The transform is currently disabled by default while we wait for clang to work-around
potential security problems:
http://lists.llvm.org/pipermail/cfe-dev/2017-January/052066.html

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

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@294998 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Transforms/IPO/FunctionAttrs.cpp b/lib/Transforms/IPO/FunctionAttrs.cpp
index 02f2b303226c24605609531030e4f095e90c5bee..59b23699658fb88cf8c3a5a49195401e119704c2 100644 (file)
--- a/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -49,6 +49,14 @@ STATISTIC(NumNoAlias, "Number of function returns marked noalias");
 STATISTIC(NumNonNullReturn, "Number of function returns marked nonnull");
 STATISTIC(NumNoRecurse, "Number of functions marked as norecurse");
 
+// FIXME: This is disabled by default to avoid exposing security vulnerabilities
+// in C/C++ code compiled by clang:
+// http://lists.llvm.org/pipermail/cfe-dev/2017-January/052066.html
+static cl::opt<bool> EnableNonnullArgPropagation(
+    "enable-nonnull-arg-prop", cl::Hidden,
+    cl::desc("Try to propagate nonnull argument attributes from callsites to "
+             "caller functions."));
+
 namespace {
 typedef SmallSetVector<Function *, 8> SCCNodeSet;
 }
@@ -531,6 +539,49 @@ static bool addArgumentReturnedAttrs(const SCCNodeSet &SCCNodes) {
   return Changed;
 }
 
+/// If a callsite has arguments that are also arguments to the parent function,
+/// try to propagate attributes from the callsite's arguments to the parent's
+/// arguments. This may be important because inlining can cause information loss
+/// when attribute knowledge disappears with the inlined call.
+static bool addArgumentAttrsFromCallsites(Function &F) {
+  if (!EnableNonnullArgPropagation)
+    return false;
+
+  bool Changed = false;
+
+  // For an argument attribute to transfer from a callsite to the parent, the
+  // call must be guaranteed to execute every time the parent is called.
+  // Conservatively, just check for calls in the entry block that are guaranteed
+  // to execute.
+  // TODO: This could be enhanced by testing if the callsite post-dominates the
+  // entry block or by doing simple forward walks or backward walks to the
+  // callsite.
+  BasicBlock &Entry = F.getEntryBlock();
+  for (Instruction &I : Entry) {
+    if (auto CS = CallSite(&I)) {
+      if (auto *CalledFunc = CS.getCalledFunction()) {
+        for (auto &CSArg : CalledFunc->args()) {
+          if (!CSArg.hasNonNullAttr())
+            continue;
+
+          // If the non-null callsite argument operand is an argument to 'F'
+          // (the caller) and the call is guaranteed to execute, then the value
+          // must be non-null throughout 'F'.
+          auto *FArg = dyn_cast<Argument>(CS.getArgOperand(CSArg.getArgNo()));
+          if (FArg && !FArg->hasNonNullAttr()) {
+            FArg->addAttr(Attribute::NonNull);
+            Changed = true;
+          }
+        }
+      }
+    }
+    if (!isGuaranteedToTransferExecutionToSuccessor(&I))
+      break;
+  }
+  
+  return Changed;
+}
+
 /// Deduce nocapture attributes for the SCC.
 static bool addArgumentAttrs(const SCCNodeSet &SCCNodes) {
   bool Changed = false;
@@ -549,6 +600,8 @@ static bool addArgumentAttrs(const SCCNodeSet &SCCNodes) {
     if (!F->hasExactDefinition())
       continue;
 
+    Changed |= addArgumentAttrsFromCallsites(*F);
+
     // Functions that are readonly (or readnone) and nounwind and don't return
     // a value can't capture arguments. Don't analyze them.
     if (F->onlyReadsMemory() && F->doesNotThrow() &&

diff --git a/test/Transforms/FunctionAttrs/nonnull.ll b/test/Transforms/FunctionAttrs/nonnull.ll
index 1fb64b7434abac58eabf21f257648af4fac496de..4a1ff14b2041aa6d61b527ff9970dec7203cc97a 100644 (file)
--- a/test/Transforms/FunctionAttrs/nonnull.ll
+++ b/test/Transforms/FunctionAttrs/nonnull.ll
@@ -1,4 +1,4 @@
-; RUN: opt -S -functionattrs %s | FileCheck %s
+; RUN: opt -S -functionattrs -enable-nonnull-arg-prop %s | FileCheck %s
 declare nonnull i8* @ret_nonnull()
 
 ; Return a pointer trivially nonnull (call return attribute)
@@ -71,4 +71,148 @@ exit:
   ret i8* %phi
 }
 
+; Test propagation of nonnull callsite args back to caller.
+
+declare void @use1(i8* %x)
+declare void @use2(i8* %x, i8* %y);
+declare void @use3(i8* %x, i8* %y, i8* %z);
+
+declare void @use1nonnull(i8* nonnull %x);
+declare void @use2nonnull(i8* nonnull %x, i8* nonnull %y);
+declare void @use3nonnull(i8* nonnull %x, i8* nonnull %y, i8* nonnull %z);
+
+declare i8 @use1safecall(i8* %x) readonly nounwind ; readonly+nounwind guarantees that execution continues to successor
+
+; Can't extend non-null to parent for any argument because the 2nd call is not guaranteed to execute.
+
+define void @parent1(i8* %a, i8* %b, i8* %c) {
+; CHECK-LABEL: @parent1(i8* %a, i8* %b, i8* %c)
+; CHECK-NEXT:    call void @use3(i8* %c, i8* %a, i8* %b)
+; CHECK-NEXT:    call void @use3nonnull(i8* %b, i8* %c, i8* %a)
+; CHECK-NEXT:    ret void
+;
+  call void @use3(i8* %c, i8* %a, i8* %b)
+  call void @use3nonnull(i8* %b, i8* %c, i8* %a)
+  ret void
+}
+
+; Extend non-null to parent for all arguments.
+
+define void @parent2(i8* %a, i8* %b, i8* %c) {
+; CHECK-LABEL: @parent2(i8* nonnull %a, i8* nonnull %b, i8* nonnull %c)
+; CHECK-NEXT:    call void @use3nonnull(i8* %b, i8* %c, i8* %a)
+; CHECK-NEXT:    call void @use3(i8* %c, i8* %a, i8* %b)
+; CHECK-NEXT:    ret void
+;
+  call void @use3nonnull(i8* %b, i8* %c, i8* %a)
+  call void @use3(i8* %c, i8* %a, i8* %b)
+  ret void
+}
+
+; Extend non-null to parent for 1st argument.
+
+define void @parent3(i8* %a, i8* %b, i8* %c) {
+; CHECK-LABEL: @parent3(i8* nonnull %a, i8* %b, i8* %c)
+; CHECK-NEXT:    call void @use1nonnull(i8* %a)
+; CHECK-NEXT:    call void @use3(i8* %c, i8* %b, i8* %a)
+; CHECK-NEXT:    ret void
+;
+  call void @use1nonnull(i8* %a)
+  call void @use3(i8* %c, i8* %b, i8* %a)
+  ret void
+}
+
+; Extend non-null to parent for last 2 arguments.
+
+define void @parent4(i8* %a, i8* %b, i8* %c) {
+; CHECK-LABEL: @parent4(i8* %a, i8* nonnull %b, i8* nonnull %c)
+; CHECK-NEXT:    call void @use2nonnull(i8* %c, i8* %b)
+; CHECK-NEXT:    call void @use2(i8* %a, i8* %c)
+; CHECK-NEXT:    call void @use1(i8* %b)
+; CHECK-NEXT:    ret void
+;
+  call void @use2nonnull(i8* %c, i8* %b)
+  call void @use2(i8* %a, i8* %c)
+  call void @use1(i8* %b)
+  ret void
+}
+
+; The callsite must execute in order for the attribute to transfer to the parent.
+; It appears benign to extend non-null to the parent in this case, but we can't do that
+; because it would incorrectly propagate the wrong information to its callers.
+
+define void @parent5(i8* %a, i1 %a_is_notnull) {
+; CHECK-LABEL: @parent5(i8* %a, i1 %a_is_notnull)
+; CHECK-NEXT:    br i1 %a_is_notnull, label %t, label %f
+; CHECK:       t:
+; CHECK-NEXT:    call void @use1nonnull(i8* %a)
+; CHECK-NEXT:    ret void
+; CHECK:       f:
+; CHECK-NEXT:    ret void
+;
+  br i1 %a_is_notnull, label %t, label %f
+t:
+  call void @use1nonnull(i8* %a)
+  ret void
+f:
+  ret void
+}
+
+; The callsite must execute in order for the attribute to transfer to the parent.
+; The volatile load might trap, so there's no guarantee that we'll ever get to the call.
+
+define i8 @parent6(i8* %a, i8* %b) {
+; CHECK-LABEL: @parent6(i8* %a, i8* %b)
+; CHECK-NEXT:    [[C:%.*]] = load volatile i8, i8* %b
+; CHECK-NEXT:    call void @use1nonnull(i8* %a)
+; CHECK-NEXT:    ret i8 [[C]]
+;
+  %c = load volatile i8, i8* %b
+  call void @use1nonnull(i8* %a)
+  ret i8 %c
+}
+
+; The nonnull callsite is guaranteed to execute, so the argument must be nonnull throughout the parent.
+
+define i8 @parent7(i8* %a) {
+; CHECK-LABEL: @parent7(i8* nonnull %a)
+; CHECK-NEXT:    [[RET:%.*]] = call i8 @use1safecall(i8* %a)
+; CHECK-NEXT:    call void @use1nonnull(i8* %a)
+; CHECK-NEXT:    ret i8 [[RET]]
+;
+  %ret = call i8 @use1safecall(i8* %a)
+  call void @use1nonnull(i8* %a)
+  ret i8 %ret
+}
+
+; Make sure that an invoke works similarly to a call.
+
+declare i32 @esfp(...)
+
+define i1 @parent8(i8* %a, i8* %bogus1, i8* %b) personality i8* bitcast (i32 (...)* @esfp to i8*){
+; CHECK-LABEL: @parent8(i8* nonnull %a, i8* nocapture readnone %bogus1, i8* nonnull %b)
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    invoke void @use2nonnull(i8* %a, i8* %b)
+; CHECK-NEXT:    to label %cont unwind label %exc
+; CHECK:       cont:
+; CHECK-NEXT:    [[NULL_CHECK:%.*]] = icmp eq i8* %b, null
+; CHECK-NEXT:    ret i1 [[NULL_CHECK]]
+; CHECK:       exc:
+; CHECK-NEXT:    [[LP:%.*]] = landingpad { i8*, i32 }
+; CHECK-NEXT:    filter [0 x i8*] zeroinitializer
+; CHECK-NEXT:    unreachable
+;
+entry:
+  invoke void @use2nonnull(i8* %a, i8* %b)
+  to label %cont unwind label %exc
+
+cont:
+  %null_check = icmp eq i8* %b, null
+  ret i1 %null_check
+
+exc:
+  %lp = landingpad { i8*, i32 }
+  filter [0 x i8*] zeroinitializer
+  unreachable
+}