<div dir="ltr">+Mark</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jun 13, 2013 at 1:05 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nicholas@mxc.ca" target="_blank">nicholas@mxc.ca</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Derek Schuff wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
Author: dschuff<br>
Date: Thu Jun 13 14:51:17 2013<br>
New Revision: 183933<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=183933&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=183933&view=rev</a><br>
Log:<br>
Fix DeleteDeadVarargs not to crash on functions referenced by BlockAddresses<br>
<br>
This pass was assuming that if hasAddressTaken() returns false for a<br>
function, the function's only uses are call sites. That's not true<br>
because there can be references by BlockAddresses too.<br>
<br>
Fix the pass to handle this case. Fix<br>
BlockAddress::<u></u>replaceUsesOfWithOnConstant() to allow a function's type<br>
to be changed by RAUW'ing the function with a bitcast of the recreated<br>
function.<br>
<br>
Patch by Mark Seaborn.<br>
<br>
Added:<br>
llvm/trunk/test/Transforms/<u></u>DeadArgElim/2013-05-17-<u></u>VarargsAndBlockAddress.ll<br>
Modified:<br>
llvm/trunk/lib/IR/Constants.<u></u>cpp<br>
llvm/trunk/lib/Transforms/IPO/<u></u>DeadArgumentElimination.cpp<br>
<br>
Modified: llvm/trunk/lib/IR/Constants.<u></u>cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Constants.cpp?rev=183933&r1=183932&r2=183933&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/lib/IR/<u></u>Constants.cpp?rev=183933&r1=<u></u>183932&r2=183933&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/lib/IR/Constants.<u></u>cpp (original)<br>
+++ llvm/trunk/lib/IR/Constants.<u></u>cpp Thu Jun 13 14:51:17 2013<br>
@@ -1389,7 +1389,7 @@ void BlockAddress::<u></u>replaceUsesOfWithOnCo<br>
BasicBlock *NewBB = getBasicBlock();<br>
<br></div></div>
if (U ==&Op<0>())<div class="im"><br>
- NewF = cast<Function>(To);<br>
+ NewF = cast<Function>(To-><u></u>stripPointerCasts());<br>
</div></blockquote>
<br>
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.<br>
<br>
What are the alternatives here, making BlockAddress hold a ConstantExpr instead of a Function? Because I can see why that's also very bad.<br>
<br>
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.)<span class="HOEnZb"><font color="#888888"><br>
<br>
Nick</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
else<br>
NewBB = cast<BasicBlock>(To);<br>
<br>
<br>
Modified: llvm/trunk/lib/Transforms/IPO/<u></u>DeadArgumentElimination.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp?rev=183933&r1=183932&r2=183933&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/lib/<u></u>Transforms/IPO/<u></u>DeadArgumentElimination.cpp?<u></u>rev=183933&r1=183932&r2=<u></u>183933&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/lib/Transforms/IPO/<u></u>DeadArgumentElimination.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/IPO/<u></u>DeadArgumentElimination.cpp Thu Jun 13 14:51:17 2013<br>
@@ -263,8 +263,10 @@ bool DAE::DeleteDeadVarargs(<u></u>Function&Fn<br>
// to pass in a smaller number of arguments into the new function.<br>
//<br>
std::vector<Value*> Args;<br>
- while (!Fn.use_empty()) {<br>
- CallSite CS(Fn.use_back());<br>
+ for (Value::use_iterator I = Fn.use_begin(), E = Fn.use_end(); I != E; ) {<br>
+ CallSite CS(*I++);<br>
+ if (!CS)<br>
+ continue;<br>
Instruction *Call = CS.getInstruction();<br>
<br>
// Pass all the same arguments.<br>
@@ -330,6 +332,11 @@ bool DAE::DeleteDeadVarargs(<u></u>Function&Fn<br>
if (DI != FunctionDIs.end())<br>
DI->second.replaceFunction(NF)<u></u>;<br>
<br>
+ // Fix up any BlockAddresses that refer to the function.<br>
+ Fn.replaceAllUsesWith(<u></u>ConstantExpr::getBitCast(NF, Fn.getType()));<br>
+ // Delete the bitcast that we just created, so that NF does not<br>
+ // appear to be address-taken.<br>
+ NF->removeDeadConstantUsers();<br>
// Finally, nuke the old function.<br>
Fn.eraseFromParent();<br>
return true;<br>
<br>
Added: llvm/trunk/test/Transforms/<u></u>DeadArgElim/2013-05-17-<u></u>VarargsAndBlockAddress.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadArgElim/2013-05-17-VarargsAndBlockAddress.ll?rev=183933&view=auto" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/test/<u></u>Transforms/DeadArgElim/2013-<u></u>05-17-VarargsAndBlockAddress.<u></u>ll?rev=183933&view=auto</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/test/Transforms/<u></u>DeadArgElim/2013-05-17-<u></u>VarargsAndBlockAddress.ll (added)<br>
+++ llvm/trunk/test/Transforms/<u></u>DeadArgElim/2013-05-17-<u></u>VarargsAndBlockAddress.ll Thu Jun 13 14:51:17 2013<br>
@@ -0,0 +1,25 @@<br>
+; RUN: opt %s -deadargelim -S | FileCheck %s<br>
+<br>
+<br>
+@block_addr = global i8* blockaddress(@varargs_func, %l1)<br>
+; CHECK: @block_addr = global i8* blockaddress(@varargs_func, %l1)<br>
+<br>
+<br>
+; This function is referenced by a "blockaddress" constant but it is<br>
+; not address-taken, so the pass should be able to remove its unused<br>
+; varargs.<br>
+<br>
+define internal i32 @varargs_func(i8* %addr, ...) {<br>
+ indirectbr i8* %addr, [ label %l1, label %l2 ]<br>
+l1:<br>
+ ret i32 1<br>
+l2:<br>
+ ret i32 2<br>
+}<br>
+; CHECK: define internal i32 @varargs_func(i8* %addr) {<br>
+<br>
+define i32 @caller(i8* %addr) {<br>
+ %r = call i32 (i8*, ...)* @varargs_func(i8* %addr)<br>
+ ret i32 %r<br>
+}<br>
+; CHECK: %r = call i32 @varargs_func(i8* %addr)<br>
<br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote>
<br>
</div></div></blockquote></div><br></div>