[WinEH] bad codegen AllocAInst inside cleanup pad

[llvmbot]

Bugzilla Link	26251
Version	3.8
OS	Windows NT
Attachments	IR with AllocAInst inside cleanup, Asm with AllocAInst inside cleanup
Reporter	LLVM Bugzilla Contributor
CC	@majnemer,@rnk

Extended Description

Not sure if this is meant to be supported, but adding an AllocAInst inside a cleanuppad generates code that does not restore the stack correctly. This cleanup IR:

//////////////// cleanuppad: ; preds = %0 %1 = cleanuppad within none [] ; [#uses = 4] %cleanup.frame = alloca [40 x i8] ; [#uses = 1, size/byte = 40] %2 = bitcast [40 x i8] %cleanup.frame to i8 ; [#uses = 2] %3 = call i1 @​_d_enter_cleanup(i8* %2) [ "funclet"(token %1) ] ; [#uses = 1] br i1 %3, label %finally1, label %cleanupret

finally1: ; preds = %cleanuppad call x86_stdcallcc void @"\01__D7cleanup5sexitFZv"() #​0 [ "funclet"(token %1) ] br label %cleanupret

cleanupret: ; preds = %cleanuppad, %finally1 call void @​_d_leave_cleanup(i8* %2) [ "funclet"(token %1) ] cleanupret from %1 unwind to caller ////////////////

is translated to this x86-asm:

//////////////// "?dtor$2@?0?D7cleanup7cleanupFZv@4HA": LBB0_2: pushl %ebp pushl %eax addl $12, %ebp movl $40, %eax calll chkstk movl %esp, %eax movl $-1, -16(%ebp) subl $4, %esp movl %eax, (%esp) movl %eax, -32(%ebp) calll d_enter_cleanup addl $4, %esp testb $1, %al jne LBB0_3 jmp LBB0_4 LBB0_3: movl $-1, -16(%ebp) calll D7cleanup5sexitFZv LBB0_4: movl $-1, -16(%ebp) movl -32(%ebp), %eax pushl %eax calll __d_leave_cleanup addl $8, %esp popl %ebp retl ////////////////

Please note that %esp is changed by the "calll __chkstk; movl %esp, %eax" sequence, but never restored.

I'll attach the full IR and asm files.

[llvmbot]

Clang does not add the "noinline" attribute on those call sites anymore.

I'll try removing it.

Without "noinline", I get an assertion: https://llvm.org/bugs/show_bug.cgi?id=26263

[llvmbot]

2. Manually use llvm.stacksave / llvm.stackrestore on entry and exit to the cleanup. This is how VLAs are code generated.

This sounds like a good short term solution. IIUC it will still need the AllocA instruction, so it's not so great if alloca gets banned. Maybe outlining in the frontend isn't too difficult, too, though I expect some trouble with functions that are already nested...

[llvmbot]

Clang does not add the "noinline" attribute on those call sites anymore.

I don't remember well, but I think there were still some issues with removing "noinline" when I recently tried to remove it. Maybe it was in combination with inferring "noreturn", though, which should no longer be a problem. I'll try removing it.

[rnk]

Two workarounds:

1. Fully outline all the code in the cleanup so that you have a separate frame to alloca in.
2. Manually use llvm.stacksave / llvm.stackrestore on entry and exit to the cleanup. This is how VLAs are code generated.

We actually considered banning dynamic allocas in funclets, since that's what MSVC does.

We could teach LLVM to insert this ESP save/restore in a funclet with a dynamic alloca, but that would also be surprising for users who don't realize that the alloca will be dead at the funclet return.

[991901f3-cc14-4404-b340-165691b62a58]

Rainer, out of curiosity, why is the call to D7cleanup5sexitFZv in the cleanuppad marked noinline?

To avoid troubles with inlining code that might include exception handling, as "recommended" by the comment in issue 25162:

In the meantime, we slapped noinline on calls inside EH cleanups in Clang r251576.

That comment is relatively old, our inliner is capable of performing the legality checks to ensure that the personality routine will be happy with our code. Clang does not add the "noinline" attribute on those call sites anymore.

[llvmbot]

Rainer, out of curiosity, why is the call to D7cleanup5sexitFZv in the cleanuppad marked noinline?

To avoid troubles with inlining code that might include exception handling, as "recommended" by the comment in issue 25162:

In the meantime, we slapped noinline on calls inside EH cleanups in Clang r251576.

[991901f3-cc14-4404-b340-165691b62a58]

reduced IR: target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32" target triple = "i386-pc-windows-msvc"

declare void @​g()

define void @​f() personality i32 (...)* @​__CxxFrameHandler3 { invoke void @​g() to label %try.success unwind label %cleanuppad

cleanuppad: %1 = cleanuppad within none [] %cleanup.frame = alloca i8 store volatile i8 42, i8* %cleanup.frame cleanupret from %1 unwind to caller

try.success: ret void }

declare i32 @​__CxxFrameHandler3(...)

[991901f3-cc14-4404-b340-165691b62a58]

Rainer, out of curiosity, why is the call to D7cleanup5sexitFZv in the cleanuppad marked noinline?

[llvmbot]

My use case is that I want to setup an exception frame to catch exceptions that happen during execution of the cleanup code.

Allocating space for this in the original function does not work, as the VC runtime has added an exception frame on top of the function frame, and the OS detects that another one that links in the opposite direction on the stack violates the expected order of frames.

Maybe there is another way to get some stack space? I currently use the workaround that _d_leave_cleanup adjusts the stack as expected.

[991901f3-cc14-4404-b340-165691b62a58]

All allocas not contained within the entry block of the function are dynamic allocas, something which is not very preferable. I'd avoid having them in the cleanuppad.

That said, we shouldn't be miscompiling this.