Page MenuHomePhabricator

[Outliner] Set nounwind for outlined functions

Authored by dmgreen on Jan 25 2019, 12:24 PM.



This prevents the outlined functions from pulling in a lot of unnecessary code
in our downstream libraries/linker. Which stops outlining making codesize
worse in c++ code with no-exceptions.

It appears that we can outline from functions that throw, so I've made this
only apply if all the parent functions are nounwind.

Diff Detail

Event Timeline

dmgreen created this revision.Jan 25 2019, 12:24 PM
efriedma added inline comments.Jan 25 2019, 12:44 PM

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.)

dmgreen added inline comments.Jan 26 2019, 5:26 AM

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.

efriedma added inline comments.Jan 26 2019, 1:39 PM

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 .

Looking at the code more carefully, it's possible it doesn't handle tail calls correctly; not sure.

dmgreen added inline comments.Jan 27 2019, 3:10 AM

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?

yroux added inline comments.Jan 28 2019, 8:11 AM

Yes, since we are not modifying the stack for tailcall or thunk I don't see any issue with these cases

efriedma added inline comments.Jan 28 2019, 4:39 PM

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.

dmgreen updated this revision to Diff 264490.Sun, May 17, 6:37 AM

I've been trying to take another look at this. I've just rebased it here onto some new tests.

It appears that the example you gave works with -fno-omit-frame-pointer/frame-pointer=non-leaf, but gets stuck with frame-pointer=none during unwinding. I do not know enough about the required unwinding info (and am having trouble finding sources for how it works) to tell exactly what is wrong.

My gut says that it is a separate issue though, and that this patch is at least conservatively correct? Even if there are existing bugs with handing throwing outlined blocks.

Herald added a project: Restricted Project. · View Herald TranscriptSun, May 17, 6:37 AM
efriedma accepted this revision.Tue, May 19, 1:21 PM


The unwind info the outliner constructs is in fact buggy, but I guess that's orthogonal to this issue.

This revision is now accepted and ready to land.Tue, May 19, 1:21 PM