[PATCH] D57254: [Outliner] Set nounwind for outlined functions

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1134
+      }))
+    F->addFnAttr(Attribute::NoUnwind);
+
----------------
yroux wrote:
> dmgreen wrote:
> > efriedma wrote:
> > > dmgreen wrote:
> > > > efriedma wrote:
> > > > > The outliner will refuse to outline calls that might unwind; you don't need the hasFnAttribute check.  (It might be possible in theory, but it would be complicated to construct the correct unwind info.)
> > > > That would certainly make this simpler.
> > > > 
> > > > I don't see any code that would stop calls that may throw from being outlined, though. And some examples seem to show it is happen at the moment (looking at aarch64, if I'm getting this right).
> > > > 
> > > > Do you think it _should_ be disabled for unwindable calls? It seems to work at the moment for the simple examples I attempted.
> > > We should never outline calls unless we can examine the callee to prove it doesn't depend on the caller's stack layout (which also implies it can't unwind).  See https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp#L5305 .
> > > 
> > > Looking at the code more carefully, it's possible it doesn't handle tail calls correctly; not sure.
> > From what I can tell, buildOutlinedFrame only needs to make stack adjustments if the function is not a tailcall, or won't become a tail call through MachineOutlinerThunk. Are there other reasons I am not thinking of that would mean the stack layout is otherwise altered?
> > 
> > Perhps put another way, what might be the problem of throwing through a tailcall or thunk, so long as it had some cfi/exidx records?
> Yes, since we are not modifying the stack for tailcall or thunk I don't see any issue with these cases
For MachineOutlinerTailCall or MachineOutlinerThunk outlining on AArch64, unwinding will never see the outlined function, so it's not an issue even if the thunk doesn't have an EH record.

The part I wasn't quite sure about glancing at the code is what would happen if callee of the call contains a tailcall that unwinds.  I guess in that case the outlined function is actually on the stack:

```
void h(int,int,int);
__attribute((noinline)) void g(int x, int y, int z) { h(x,y,z); }
void f() { g(1,2,3); g(1,2,3); }
void f2() { g(1,2,3); g(1,2,3); }
```

I have no idea if we somehow generate correct unwind info here.  If we do, that's fine, and I guess we want to make sure the function isn't marked nounwind, like the current version of this patch.  (I'm pretty sure we're missing SEH directives on Windows, but maybe the cfi markings are enough for other targets.)

Checking whether the parent is nounwind, rather than the call itself, is overly conservative, but I guess if the parent isn't nounwind, there isn't any great benefit to marking the outlined function nounwind anyway.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57254/new/

https://reviews.llvm.org/D57254