Page MenuHomePhabricator

[Windows] Disable TrapUnreachable for Win64, add SEH_NoReturn
ClosedPublic

Authored by rnk on Thu, Aug 29, 3:50 PM.

Details

Summary

Users have complained llvm.trap produce two ud2 instructions on Win64,
one for the trap, and one for unreachable. This change fixes that.

TrapUnreachable was added and enabled for Win64 in r206684 (April 2014)
to avoid issues where the unwinder would disassemble forward from a
call's return address and go off the rails. Some of my experiments show
that the unwinder will sometimes use the unwind info from the next
function to unwind, if you arrange for the throwing call to end on a
16-byte aligned boundary. By trapping on unreachable, we ensured that
noreturn calls would always have some real instructions after the return
address, so the return address would be inside the bounds of the region
described by the unwind info.

Instead, this change inserts a new pseudo instruction, SEH_NoReturn,
which expands to an int3 if it happens to be the last instruction in the
current funclet after basic block layout. This approach is similar to
the approach we took with SEH_Epilogue, which expands to a nop if there
is a call immediately preceding the pseudo instruction.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Thu, Aug 29, 3:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Aug 29, 3:50 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
rnk updated this revision to Diff 217993.Thu, Aug 29, 3:53 PM
  • fix test
Harbormaster completed remote builds in B37546: Diff 217993.

Why expand to int3 instead of nop?

Can't say I'm familiar enough with these layers to give it a proper review, but what I make out from reading the code looks good at least.

hans accepted this revision.Fri, Aug 30, 12:11 AM

I'm also not familiar with the unwinder and exception handling, but the code itself and the explanation seems good to me.

llvm/lib/Target/X86/X86ISelLowering.cpp
4117 ↗(On Diff #217993)

Maybe add a comment clarifying why this gets inserted here?

This revision is now accepted and ready to land.Fri, Aug 30, 12:11 AM
rnk added a comment.Fri, Aug 30, 1:03 PM

Why expand to int3 instead of nop?

If we have to emit some code, it seems better to trap than to fall through off the end of a function into the next. MSVC uses int3, although last I checked they use it for all noreturn functions: https://gcc.godbolt.org/z/3GO_d2.

Related to this int3 stuff, I've also seen that MSVC emits nops after calls at the end of EH scopes. See the npad 1 at the end of this try block: https://gcc.godbolt.org/z/qOLw5-
LLVM instead adds one to each label used in the EH table, which assumes every call is at least one byte.

It's also possible that I've misdiagnosed the problem, the problem might not be in the unwinder, it could be in the C++ EH personality. Maybe I should rewrite the commit a bit.

rnk marked an inline comment as done.Fri, Aug 30, 1:45 PM
This revision was automatically updated to reflect the committed changes.

This broke one exception handling test case of mine. Curiously enough, it broke SEH unwinding in wine, but not on windows 10. In wine, it gives an error that looks like this:

0092:err:seh:setup_exception stack overflow 4832 bytes in thread 0092 eip 000000007bc91b16 esp 0000000000140330 stack 0x140000-0x142000-0x240000

The file that breaks is at https://martin.st/temp/seh-bug.cpp, built with clang++ -target x86_64-w64-mingw32 seh-bug.cpp -c -o test.o -fuse-cxa-atexit. The diff in generated code from this commit looks like this:

--- seh-bug-pre.s       2019-08-31 22:09:02.317179362 +0300
+++ seh-bug-post.s      2019-08-31 22:09:51.812133103 +0300
@@ -57,6 +57,8 @@
                                         # kill: def $r8 killed $r8d
        movq    %rax, %rcx
        callq   __cxa_throw
+       subq    $32, %rsp
+       int3
 .Ltmp4:
        jmp     .LBB0_8
 .LBB0_5:                                # %lpad
@@ -77,9 +79,7 @@
 .LBB0_7:                                # %eh.resume
        movq    72(%rsp), %rcx
        callq   _Unwind_Resume
-       ud2
 .LBB0_8:                                # %unreachable
-       ud2
 .Lfunc_end0:
        .seh_handlerdata
        .text

So I presume that the issue is that the new int3 isn't at the end of the function where the previous double ud2 were?

rnk added a comment.Tue, Sep 3, 3:32 PM

So I presume that the issue is that the new int3 isn't at the end of the function where the previous double ud2 were?

The old ud2 should have become int3, and there was no need to add an int3 after cxa_throw, that was undesired. I ended up reverting this change in rL370829. There were two issues:

  1. SEH_NoReturn expansion needs to look across blocks similar to what SEH_Epilogue does
  2. _Unwind_Resume is not marked noreturn

Furthermore, noreturn is supposed to be an optimization hint, not a requirement for correctness. We should really be expanding all unreachable instructions to this SEH_NoReturn pseudo, like a disappearing-ink version of TrapUnreachable. I tried to implement that, but it was hard because it interacts with target independent fast isel. And after all, I'm not sure I like this pseudo instruction that expands to something else based on some non-local analysis. I'm starting to think this should be a standalone pass that inserts int3 as needed at the end of every function and funclet that ends in a call instruction. It would be so simple, and wouldn't need 3 implementations in SDISel, FastISel, and global ISel.