[llvm] r183933 - Fix DeleteDeadVarargs not to crash on functions referenced by BlockAddresses

[Nick Lewycky]

Derek Schuff wrote:
> Author: dschuff
> Date: Thu Jun 13 14:51:17 2013
> New Revision: 183933
>
> URL: http://llvm.org/viewvc/llvm-project?rev=183933&view=rev
> Log:
> Fix DeleteDeadVarargs not to crash on functions referenced by BlockAddresses
>
> This pass was assuming that if hasAddressTaken() returns false for a
> function, the function's only uses are call sites.  That's not true
> because there can be references by BlockAddresses too.
>
> Fix the pass to handle this case.  Fix
> BlockAddress::replaceUsesOfWithOnConstant() to allow a function's type
> to be changed by RAUW'ing the function with a bitcast of the recreated
> function.
>
> Patch by Mark Seaborn.
>
> Added:
>      llvm/trunk/test/Transforms/DeadArgElim/2013-05-17-VarargsAndBlockAddress.ll
> Modified:
>      llvm/trunk/lib/IR/Constants.cpp
>      llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp
>
> Modified: llvm/trunk/lib/IR/Constants.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Constants.cpp?rev=183933&r1=183932&r2=183933&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/Constants.cpp (original)
> +++ llvm/trunk/lib/IR/Constants.cpp Thu Jun 13 14:51:17 2013
> @@ -1389,7 +1389,7 @@ void BlockAddress::replaceUsesOfWithOnCo
>     BasicBlock *NewBB = getBasicBlock();
>
>     if (U ==&Op<0>())
> -    NewF = cast<Function>(To);
> +    NewF = cast<Function>(To->stripPointerCasts());

This makes me extremely nervous. This function is very low level, it 
seems like it shouldn't be doing anything like calling 
stripPointerCasts(), that'd be like rigging SmallVector to call 
eraseFromParent for you or something.

What are the alternatives here, making BlockAddress hold a ConstantExpr 
instead of a Function? Because I can see why that's also very bad.

What happens if To really isn't a function? What if I create an undef of 
function type, then RAUW the function to that? Could you write a 
testcase under unittests/ for that case? (No, it's not clear to me that 
it worked before either.)

Nick

>     else
>       NewBB = cast<BasicBlock>(To);
>
>
> Modified: llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp?rev=183933&r1=183932&r2=183933&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp (original)
> +++ llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp Thu Jun 13 14:51:17 2013
> @@ -263,8 +263,10 @@ bool DAE::DeleteDeadVarargs(Function&Fn
>     // to pass in a smaller number of arguments into the new function.
>     //
>     std::vector<Value*>  Args;
> -  while (!Fn.use_empty()) {
> -    CallSite CS(Fn.use_back());
> +  for (Value::use_iterator I = Fn.use_begin(), E = Fn.use_end(); I != E; ) {
> +    CallSite CS(*I++);
> +    if (!CS)
> +      continue;
>       Instruction *Call = CS.getInstruction();
>
>       // Pass all the same arguments.
> @@ -330,6 +332,11 @@ bool DAE::DeleteDeadVarargs(Function&Fn
>     if (DI != FunctionDIs.end())
>       DI->second.replaceFunction(NF);
>
> +  // Fix up any BlockAddresses that refer to the function.
> +  Fn.replaceAllUsesWith(ConstantExpr::getBitCast(NF, Fn.getType()));
> +  // Delete the bitcast that we just created, so that NF does not
> +  // appear to be address-taken.
> +  NF->removeDeadConstantUsers();
>     // Finally, nuke the old function.
>     Fn.eraseFromParent();
>     return true;
>
> Added: llvm/trunk/test/Transforms/DeadArgElim/2013-05-17-VarargsAndBlockAddress.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadArgElim/2013-05-17-VarargsAndBlockAddress.ll?rev=183933&view=auto
> ==============================================================================
> --- llvm/trunk/test/Transforms/DeadArgElim/2013-05-17-VarargsAndBlockAddress.ll (added)
> +++ llvm/trunk/test/Transforms/DeadArgElim/2013-05-17-VarargsAndBlockAddress.ll Thu Jun 13 14:51:17 2013
> @@ -0,0 +1,25 @@
> +; RUN: opt %s -deadargelim -S | FileCheck %s
> +
> +
> + at block_addr = global i8* blockaddress(@varargs_func, %l1)
> +; CHECK: @block_addr = global i8* blockaddress(@varargs_func, %l1)
> +
> +
> +; This function is referenced by a "blockaddress" constant but it is
> +; not address-taken, so the pass should be able to remove its unused
> +; varargs.
> +
> +define internal i32 @varargs_func(i8* %addr, ...) {
> +  indirectbr i8* %addr, [ label %l1, label %l2 ]
> +l1:
> +  ret i32 1
> +l2:
> +  ret i32 2
> +}
> +; CHECK: define internal i32 @varargs_func(i8* %addr) {
> +
> +define i32 @caller(i8* %addr) {
> +  %r = call i32 (i8*, ...)* @varargs_func(i8* %addr)
> +  ret i32 %r
> +}
> +; CHECK: %r = call i32 @varargs_func(i8* %addr)
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>