[llvm] r293727 - [IPSCCP] Teach how to not propagate return values of naked functions.

[Sean Silva via llvm-commits]

On Wed, Feb 1, 2017 at 6:51 PM, Davide Italiano <davide at freebsd.org> wrote:

> On Wed, Feb 1, 2017 at 6:43 PM, Sean Silva <chisophugis at gmail.com> wrote:
> >
> >
> > On Wed, Feb 1, 2017 at 5:48 PM, Davide Italiano <davide at freebsd.org>
> wrote:
> >>
> >> On Wed, Feb 1, 2017 at 5:36 PM, Sean Silva <chisophugis at gmail.com>
> wrote:
> >> >
> >> >
> >> > On Tue, Jan 31, 2017 at 5:01 PM, Davide Italiano via llvm-commits
> >> > <llvm-commits at lists.llvm.org> wrote:
> >> >>
> >> >> Author: davide
> >> >> Date: Tue Jan 31 19:01:22 2017
> >> >> New Revision: 293727
> >> >>
> >> >> URL: http://llvm.org/viewvc/llvm-project?rev=293727&view=rev
> >> >> Log:
> >> >> [IPSCCP] Teach how to not propagate return values of naked functions.
> >> >>
> >> >> Differential Revision:  https://reviews.llvm.org/D29360
> >> >>
> >> >> Added:
> >> >>     llvm/trunk/test/Transforms/IPConstantProp/naked-return.ll
> >> >> Modified:
> >> >>     llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
> >> >>
> >> >> Modified: llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
> >> >> URL:
> >> >>
> >> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Transforms/Scalar/SCCP.cpp?rev=293727&r1=293726&r2=293727&view=diff
> >> >>
> >> >>
> >> >> ============================================================
> ==================
> >> >> --- llvm/trunk/lib/Transforms/Scalar/SCCP.cpp (original)
> >> >> +++ llvm/trunk/lib/Transforms/Scalar/SCCP.cpp Tue Jan 31 19:01:22
> 2017
> >> >> @@ -1712,7 +1712,10 @@ static bool runIPSCCP(Module &M, const D
> >> >>
> >> >>      // If this is an exact definition of this function, then we can
> >> >> propagate
> >> >>      // information about its result into callsites of it.
> >> >> -    if (F.hasExactDefinition())
> >> >> +    // Don't touch naked functions. They may contain asm returning a
> >> >> +    // value we don't see, so we may end up interprocedurally
> >> >> propagating
> >> >> +    // the return value incorrectly.
> >> >> +    if (F.hasExactDefinition() && !F.hasFnAttribute(Attribute::
> Naked))
> >> >>        Solver.AddTrackedFunction(&F);
> >> >>
> >> >>      // If this function only has direct calls that we can see, we
> can
> >> >> track its
> >> >>
> >> >> Added: llvm/trunk/test/Transforms/IPConstantProp/naked-return.ll
> >> >> URL:
> >> >>
> >> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> Transforms/IPConstantProp/naked-return.ll?rev=293727&view=auto
> >> >>
> >> >>
> >> >> ============================================================
> ==================
> >> >> --- llvm/trunk/test/Transforms/IPConstantProp/naked-return.ll
> (added)
> >> >> +++ llvm/trunk/test/Transforms/IPConstantProp/naked-return.ll Tue
> Jan
> >> >> 31
> >> >> 19:01:22 2017
> >> >> @@ -0,0 +1,28 @@
> >> >> +; RUN: opt -ipsccp -S %s | FileCheck %s
> >> >> +
> >> >> +target datalayout = "e-m:x-p:32:32-i64:64-f80:32-
> n8:16:32-a:0:32-S32"
> >> >> +target triple = "i686-pc-windows-msvc19.0.24215"
> >> >> +
> >> >> +define i32 @dipsy(i32, i32) local_unnamed_addr #0 {
> >> >> +BasicBlock0:
> >> >> +  call void asm "\0D\0Apushl %ebp\0D\0Amovl 8(%esp),%eax\0D\0Amovl
> >> >> 12(%esp), %ebp\0D\0Acalll *%eax\0D\0Apopl %ebp\0D\0Aretl\0D\0A", ""()
> >> >> +  ret i32 0
> >> >
> >> >
> >> > If this asm is going to return, shouldn't we have `unreachable`
> instead
> >> > of
> >> > `ret i32 0`? That would naturally be correct I think without requiring
> >> > special handling for naked functions. I.e., the verifier should ensure
> >> > that
> >> > naked functions always end with `unreachable`.
> >> >
> >>
> >> That would require some other code to make sure we don't propagate the
> >> unreachable to the caller (which apparently PruneEH currently does).
> WDYT?
> >
> >
> > 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.
> > 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)
> >
>
> It generally lacks 'being able to reason about inline asm'. As far as
> I can tell transformations consider asm in IR as an opaque string, and
> this causes problems in many cases.
>

We have "volatile" (whatever we turn C/C++ `asm volatile` into). Also the
in-out params of the asm.

-- Sean Silva


>
> > 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.
> >
> >
>
> I agree on the last bit (it seems this issue is more about modeling
> what inline asm should do). Also, please note a slight modification of
> this problem in DeadArgElim (where we force functions to be marked
> live because we can't make assumptions of what the asm might use).
>
> --
> Davide
>
> "There are no solved problems; there are only problems that are more
> or less solved" -- Henri Poincare
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170201/32992c9b/attachment.html>