Index: llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp =================================================================== --- llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp +++ llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp @@ -49,6 +49,14 @@ 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 EnableNonnullArgPropagation( + "enable-nonnull-arg-prop", cl::Hidden, + cl::desc("Try to propagate nonnull argument attributes from callsites to " + "caller functions.")); + namespace { typedef SmallSetVector SCCNodeSet; } @@ -531,6 +539,49 @@ 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(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 @@ 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() && Index: llvm/trunk/test/Transforms/FunctionAttrs/nonnull.ll =================================================================== --- llvm/trunk/test/Transforms/FunctionAttrs/nonnull.ll +++ llvm/trunk/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 @@ 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 +}