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

[Derek Schuff]

er... ok *really* +Mark


On Thu, Jun 13, 2013 at 1:07 PM, Derek Schuff <dschuff at google.com> wrote:

> +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/daaa4d6c/attachment.html>