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

[Derek Schuff]

+Mark


On Thu, Jun 13, 2013 at 1:05 PM, Nick Lewycky <nicholas at mxc.ca> wrote:

> 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<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<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<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<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<http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130613/b51fc2a6/attachment.html>