<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Feb 1, 2017 at 5:48 PM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Feb 1, 2017 at 5:36 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br>
><br>
><br>
> On Tue, Jan 31, 2017 at 5:01 PM, Davide Italiano via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> Author: davide<br>
>> Date: Tue Jan 31 19:01:22 2017<br>
>> New Revision: 293727<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=293727&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=293727&view=rev</a><br>
>> Log:<br>
>> [IPSCCP] Teach how to not propagate return values of naked functions.<br>
>><br>
>> Differential Revision: <a href="https://reviews.llvm.org/D29360" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D29360</a><br>
>><br>
>> Added:<br>
>> llvm/trunk/test/Transforms/<wbr>IPConstantProp/naked-return.ll<br>
>> Modified:<br>
>> llvm/trunk/lib/Transforms/<wbr>Scalar/SCCP.cpp<br>
>><br>
>> Modified: llvm/trunk/lib/Transforms/<wbr>Scalar/SCCP.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SCCP.cpp?rev=293727&r1=293726&r2=293727&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Transforms/Scalar/SCCP.cpp?<wbr>rev=293727&r1=293726&r2=<wbr>293727&view=diff</a><br>
>><br>
>> ==============================<wbr>==============================<wbr>==================<br>
>> --- llvm/trunk/lib/Transforms/<wbr>Scalar/SCCP.cpp (original)<br>
>> +++ llvm/trunk/lib/Transforms/<wbr>Scalar/SCCP.cpp Tue Jan 31 19:01:22 2017<br>
>> @@ -1712,7 +1712,10 @@ static bool runIPSCCP(Module &M, const D<br>
>><br>
>> // If this is an exact definition of this function, then we can<br>
>> propagate<br>
>> // information about its result into callsites of it.<br>
>> - if (F.hasExactDefinition())<br>
>> + // Don't touch naked functions. They may contain asm returning a<br>
>> + // value we don't see, so we may end up interprocedurally propagating<br>
>> + // the return value incorrectly.<br>
>> + if (F.hasExactDefinition() && !F.hasFnAttribute(Attribute::<wbr>Naked))<br>
>> Solver.AddTrackedFunction(&F);<br>
>><br>
>> // If this function only has direct calls that we can see, we can<br>
>> track its<br>
>><br>
>> Added: llvm/trunk/test/Transforms/<wbr>IPConstantProp/naked-return.ll<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/IPConstantProp/naked-return.ll?rev=293727&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Transforms/IPConstantProp/<wbr>naked-return.ll?rev=293727&<wbr>view=auto</a><br>
>><br>
>> ==============================<wbr>==============================<wbr>==================<br>
>> --- llvm/trunk/test/Transforms/<wbr>IPConstantProp/naked-return.ll (added)<br>
>> +++ llvm/trunk/test/Transforms/<wbr>IPConstantProp/naked-return.ll Tue Jan 31<br>
>> 19:01:22 2017<br>
>> @@ -0,0 +1,28 @@<br>
>> +; RUN: opt -ipsccp -S %s | FileCheck %s<br>
>> +<br>
>> +target datalayout = "e-m:x-p:32:32-i64:64-f80:32-<wbr>n8:16:32-a:0:32-S32"<br>
>> +target triple = "i686-pc-windows-msvc19.0.<wbr>24215"<br>
>> +<br>
>> +define i32 @dipsy(i32, i32) local_unnamed_addr #0 {<br>
>> +BasicBlock0:<br>
>> + call void asm "\0D\0Apushl %ebp\0D\0Amovl 8(%esp),%eax\0D\0Amovl<br>
>> 12(%esp), %ebp\0D\0Acalll *%eax\0D\0Apopl %ebp\0D\0Aretl\0D\0A", ""()<br>
>> + ret i32 0<br>
><br>
><br>
> If this asm is going to return, shouldn't we have `unreachable` instead of<br>
> `ret i32 0`? That would naturally be correct I think without requiring<br>
> special handling for naked functions. I.e., the verifier should ensure that<br>
> naked functions always end with `unreachable`.<br>
><br>
<br>
</div></div>That would require some other code to make sure we don't propagate the<br>
unreachable to the caller (which apparently PruneEH currently does). WDYT?<br></blockquote><div><br></div><div>It sounds like the missing thing is (at least) being able to mark an inline asm as returning (hard to do without making it a terminator?). More generally, marking inline asm's as affecting control flow is also a longstanding issue preventing us from implementing "asm goto". So this might be pretty nontrivial.</div><div>Maybe we can model it as the inline asm having a dummy out param which is the thing that is returned? (or marking the inline asm such that the optimizer thinks it might throw an exception, do a longjmp, or any other side effect that will conservatively model the asm's behavior of returning from the current function)</div><div><br></div><div>It just seems wrong for the special case check to be for naked functions since the issue seems to be more about modeling what an inline asm will do.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
$ ./opt -O3 naked-return.ll -opt-bisect-limit=85 -S<br>
<br>
[...]<br>
BISECT: running pass (83) Aggressive Dead Code Elimination on function<br>
(tinkywinky)<br>
BISECT: running pass (84) Simplify the CFG on function (tinkywinky)<br>
BISECT: running pass (85) Combine redundant instructions on function<br>
(tinkywinky)<br>
[...]<br>
<br>
; Function Attrs: naked noreturn nounwind<br>
<span class="">define i32 @dipsy(i32, i32) local_unnamed_addr #0 {<br>
</span>BasicBlock0:<br>
tail call void asm "\0D\0Apushl %ebp\0D\0Amovl<br>
<span class="">8(%esp),%eax\0D\0Amovl 12(%esp), %ebp\0D\0Acalll *%eax\0D\0Apopl<br>
</span>%ebp\0D\0Aretl\0D\0A", ""() #3<br>
unreachable<br>
}<br>
[...]<br>
define void @patatino(i32, i32, i32) local_unnamed_addr #2 {<br>
bb:<br>
%3 = tail call i32 @dipsy(i32 %0, i32 %1) #4<br>
tail call void @tinkywinky(i32 %3, i32 %2, i32 %1) #4<br>
ret void<br>
}<br>
<br>
and<br>
$ ./opt -O3 naked-return.ll -opt-bisect-limit=86 -S<br>
[...]<br>
BISECT: running pass (83) Aggressive Dead Code Elimination on function<br>
(tinkywinky)<br>
BISECT: running pass (84) Simplify the CFG on function (tinkywinky)<br>
BISECT: running pass (85) Combine redundant instructions on function<br>
(tinkywinky)<br>
BISECT: running pass (86) Remove unused exception handling info on SCC<br>
(patatino)<br>
BISECT: NOT running pass (87) Function Integration/Inlining on SCC (patatino)<br>
[...]<br>
<br>
define void @patatino(i32, i32, i32) local_unnamed_addr #2 {<br>
bb:<br>
%3 = tail call i32 @dipsy(i32 %0, i32 %1) #4<br>
unreachable<br>
}<br>
</blockquote></div><br></div></div>