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

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

llvm-svn: 294998
This commit is contained in:
Sanjay Patel 2017-02-13 23:10:51 +00:00
parent f72eb8e1e7
commit 1e7c803052
2 changed files with 198 additions and 1 deletions

View File

@ -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() &&

View File

@ -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
}