[PATCH] Seh exceptions on Win64

Hi,

I’d like to submit a patch to match the clang patch on the front end.
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140414/103257.html

The front end doesn’t need this patch to work but it’s still important.

This is mostly based on work done by kai from redstar.de

Could I get some feedback on this?

I’m not sure if the emitting of the register names will effect msvc.

Many Thanks

Martell Malone

llvm-win64-exceptions.patch (16.3 KB)

llvm-emit-reg-names.patch (1.71 KB)

Hi,

if (is64Bit && T.getOS() == Triple::MinGW32 && T.getEnvironment() && Triple::GNU)

should be

if (is64Bit && Triple.isWindowsGNUEnvironment())

Yaron

Updated patch.

I’ve reordered seh patch to have includes in the correct location

Was a very sloppy edit before

llvm-win64-exceptions.patch (16.6 KB)

Hi yaron,

Good spot :slight_smile:
missed that in my conversion.

Updated patch attached

llvm-win64-exceptions.patch (16.5 KB)

Hi,

I am curious - how does clang deal with epilogue-less functions that result from _Raise_Exception being marked ‘noreturn’?
I’ve also been playing with Kai’s patch, and discovered that this tends to greatly confuse Windows stack unwinder in cases when noreturn call is at the end of a function, so execution appears to flow directly into the prologue of the next function.
It looks like MSVC solves this problem by inserting ‘int 3’ after calls to noreturns. This is actually the prime motivation for me wanting to emit code for ‘unreachable’

Vadim

Hi,

I am curious - how does clang deal with epilogue-less functions that result from _Raise_Exception being marked ‘noreturn’?
I’ve also been playing with Kai’s patch, and discovered that this tends to greatly confuse Windows stack unwinder in cases when noreturn call is at the end of a function, so execution appears to flow directly into the prologue of the next function.
It looks like MSVC solves this problem by inserting ‘int 3’ after calls to noreturns. This is actually the prime motivation for me wanting to emit code for ‘unreachable’

Vadim

Could I get some feedback on this?
I'm not sure if the emitting of the register names will effect msvc.

Emitting the register names LGTM (in fact GAS doesn't seem to be able to
parse this anyway). Since this is just GAS assembly, this does not
affect MSVC. Some tests would be nice, if possible.

Unwind emission still suffers from the same problems that the old code
did. It doesn't handle realigned stacks, and as I summarized in PR16779,
I believe it's not necessary to change the prologue emission for Win64.
Currently ilist_node::getPrevNode of MachineInstr is unusable because it
segfaults on the first node in a list. I raised this issue before on
llvm-dev and proposed a patch but nobody cared, so you have to do
something like this:

  static const MachineInstr *getPrevNode(const MachineInstr *MI) {
    if (&*MI->getParent()->instr_begin() == MI)
      return nullptr;
    return MI->getPrevNode();
  }

Some small nits:

+ if (MI->getOperand(1).getImm() != 1 || MI->getOperand(2).getImm() != 0 ||
+ MI->getOperand(4).getImm() != 0)

Spacing. Also, a comment clarifying that scale/index/segment must be
unused might be helpful here.

+ if (MI->getNextNode()) {
+ const MachineInstr *MI2 = MI->getNextNode();
+ if (!(MI2->getFlag(MachineInstr::FrameSetup) ||
+ (MI2->isCFIInstruction() && MI2->getNextNode() &&
+ MI2->getNextNode()->getFlag(MachineInstr::FrameSetup)))) {
+ OutStreamer.EmitWin64EHEndProlog();
+ }
+ }

I'd simplify this and just skip over all CFI instructions. If there's
still a node left, and it lacks the FrameSetup flag, end the prolog.

+; WIN64: callq {{__chkstk|___chkstk_ms}}

This check should really differentiate between MSVC and Mingw. You can
add additional FileCheck prefixes so the rest does not have to be
duplicated.

Overall this needs more tests covering __chkstk, alloca, manual
over-alignment and various combinations.

I am curious - how does clang deal with epilogue-less functions that result
from _Raise_Exception being marked 'noreturn'?

AFAIK it doesn't emit anything.

I've also been playing with Kai's patch, and discovered that this tends to
greatly confuse Windows stack unwinder in cases when noreturn call is at
the end of a function, so execution appears to flow directly into the
prologue of the next function.
It looks like MSVC solves this problem by inserting 'int 3' after calls to
noreturns.

The unwinder starts scanning forward looking for an optional add or lea,
then zero or more register pops, followed by a ret or a few simple jmp
variants. So you can insert anything except these. Though int3 would
probably make the most sense.

-Nico

Hi,

Hi,
I am curious - how does clang deal with epilogue-less functions that
result from _Raise_Exception being marked 'noreturn'?
I've also been playing with Kai's patch, and discovered that this tends
to greatly confuse Windows stack unwinder in cases when noreturn call is
at the end of a function, so execution appears to flow directly into the
prologue of the next function.

Yes, that's true. The patch has a problem if the epilogue is dead code and eliminated.

It looks like MSVC solves this problem by inserting 'int 3' after calls
to noreturns. This is actually the prime motivation for me wanting to
emit code for 'unreachable'
<http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-April/072145.html> ...

Vadim

Yaron also pointed out that

int main() { throw 1; }

generates two .seh_endprologue instructions. I still did not fix this bug.

Regards,
Kai

So I made a patch that fixes the “missing epilogue” problem by emitting a trap instruction for IR ‘unreachable’.

A secondary use for it would be for anyone wanting to make double-sure that ‘noreturn’ functions, indeed, do not return.

Would LLVM devs entertain the idea of committing something like this?

0001-Emit-trap-for-unreachable-IR-instruction.patch (4.46 KB)

OK. The register name patch committed in r206565 and the SEH patch is on

http://reviews.llvm.org/D3418

Nico, tests are not nice, they are absolutely required in LLVM. And it
might be good to get some of the MC maintainers to look at this.

Martell, you really cannot post patches for review or otherwise contribute
them to LLVM unless you authored the patch. If Kai authored the patch, then
Kai needs to contirbute it.

Hi Chandler,

Kai contributed the WIN64 SEH patch some time ago on llvm-commits:

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131118/196105.html

it was never completed. Kai also responded in this thread.

I opened a phabricator for the patch

http://reviews.llvm.org/D3418

Yaron

Then please, do not start new threads with new people and no reference to
the existing review. That makes accurate code review essentially impossible.

I still don't know why you claimed part of the code was implemented by Ray
Donnelly. If it was, then Kai shouldn't have contributed the patch either.

Exactly who authored this code, and can they write the emails contributing
it?

Hi Chandler,

There were five SEH releated patches posted in two threads in the last days.

Two different patches in Martell e-mail starting this thread: the win64 seh (llvm) and the register names

Three more related SEH patches in another thread: one for win64 seh clang, one for MinGW toolchain and another for unreachable prologue.

To clarify and allow proper reviews for the different patches I opened reviews for four of them (the fifth got LGTM in the discussion but it does have the ownership issue you wrote)

D3417
Emit a trap instruction for IR 'unreachable

D3418
SEH exceptions on Win64 (LLVM)

D3419
SEH exceptions on Win64 (clang part)

D3420
MinGW toolchain

Per your suggestion the link to the original discussion in D3418 was added.

Code authors are:
unreachable is by Vadim Chugunov.
win64 seh llvm is by Kai Nacke + Martell re-posting.

win64 seh clang is by Martell.

mingw toolchain is by Martell.

register names is by Ray Donnelly + Martell posting it.

seh test for “register names” ( test is r206566) by myself.

All but the “register names” patch were posted by their author on the llvm lists.

Yaron

Hi chandler,

Sorry for the confusion.
I’ve never submitted patches before.
I’ll cc Ray to get him to confirm that the patch can be merged. His header should be on the patch also so that’s how you know it is his.

Yes Yaron the names above seem and the detail seem correct to me except that the MinGW driver derived from Rubens submission to LLVM reviews some months ago.
I said that at the start of the thread.

Basically the guys including Ray were looking to have an x64 mingw64 working with clang. So I fixed Rubens patch and kai’s patch for head

I’m not concerned with having my name on any attribution I just want them all merged as this combination of patches fixes mingw64 x64 with clang.

Kind Regards
Martell

In summary we have no less than six patches required to support Win64 SEH MinGW. The first five could be committed after review and LGTM but the last one also requires Ray Donnelly approval.
Please comment in the Phabricator so the comments would be kept in context.

‘unreachable’ trap
http://reviews.llvm.org/D3417

Win64 SEH (LLVM)
http://reviews.llvm.org/D3418

Win64 SEH (clang)

http://reviews.llvm.org/D3419

MinGW toolchain
http://reviews.llvm.org/D3420

TLS (clang)

http://reviews.llvm.org/D3421

Register names instead of numbers
http://reviews.llvm.org/D3422

No, it really requires that Ray Donnelly *contribute* the patch. That is
different.

Also, folks informed us of at least one place where a patch posted to the
list by Kai in the past from the LDC / redstar.de work contained copied
copyrighted material that Kai did not hold the rights to, and was not
available under *any* license. This is really concerning, and it means that
any significant contributions from this body of work need to be very
carefully audited for other places where this has happened. I'm not sure of
any good way to do that at this point.

This isn't to say we don't want to support the win64 ABI and exception
handling stuff, we really do (and thanks for working on it!), but we need
to be careful about how we do it.

My suggestion if you want to move this forward quickly would be for folks
who are interested author their own patches independently, without any
reference to or basis on existing work, and contribute that patch. That is
what folks here are planning to do for their ABI work.

Hi Chandler,

For this patch, it is fine.