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

[Davide Italiano via llvm-commits]

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.

> 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